Skip to content

Remove the Arc rt::init allocation for thread info #123550

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions library/std/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,7 @@
#![feature(str_internals)]
#![feature(strict_provenance)]
#![feature(strict_provenance_atomic_ptr)]
#![feature(sync_unsafe_cell)]
#![feature(ub_checks)]
// tidy-alphabetical-end
//
Expand Down
2 changes: 1 addition & 1 deletion library/std/src/rt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ unsafe fn init(argc: isize, argv: *const *const u8, sigpipe: u8) {
// handle does not match the current ID, we should attempt to use the
// current thread ID here instead of unconditionally creating a new
// one. Also see #130210.
let thread = Thread::new_main(thread::current_id());
let thread = unsafe { Thread::new_main(thread::current_id()) };
if let Err(_thread) = thread::set_current(thread) {
// `thread::current` will create a new handle if none has been set yet.
// Thus, if someone uses it before main, this call will fail. That's a
Expand Down
170 changes: 117 additions & 53 deletions library/std/src/thread/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -158,9 +158,12 @@
#[cfg(all(test, not(any(target_os = "emscripten", target_os = "wasi"))))]
mod tests;

use core::cell::SyncUnsafeCell;
use core::ffi::CStr;
use core::mem::MaybeUninit;

use crate::any::Any;
use crate::cell::UnsafeCell;
use crate::ffi::CStr;
use crate::marker::PhantomData;
use crate::mem::{self, ManuallyDrop, forget};
use crate::num::NonZero;
Expand Down Expand Up @@ -1125,7 +1128,7 @@ pub fn park_timeout(dur: Duration) {
let guard = PanicGuard;
// SAFETY: park_timeout is called on the parker owned by this thread.
unsafe {
current().inner.as_ref().parker().park_timeout(dur);
current().0.parker().park_timeout(dur);
}
// No panic occurred, do not abort.
forget(guard);
Expand Down Expand Up @@ -1232,65 +1235,114 @@ impl ThreadId {
// Thread
////////////////////////////////////////////////////////////////////////////////

/// The internal representation of a `Thread`'s name.
enum ThreadName {
Main,
Other(ThreadNameString),
Unnamed,
}

// This module ensures private fields are kept private, which is necessary to enforce the safety requirements.
mod thread_name_string {
use core::str;

use super::ThreadName;
use crate::ffi::{CStr, CString};

/// Like a `String` it's guaranteed UTF-8 and like a `CString` it's null terminated.
pub(crate) struct ThreadNameString {
inner: CString,
}

impl ThreadNameString {
pub fn as_str(&self) -> &str {
// SAFETY: `self.inner` is only initialised via `String`, which upholds the validity invariant of `str`.
unsafe { str::from_utf8_unchecked(self.inner.to_bytes()) }
}
}

impl core::ops::Deref for ThreadNameString {
type Target = CStr;
fn deref(&self) -> &CStr {
&self.inner
}
}

impl From<String> for ThreadNameString {
fn from(s: String) -> Self {
Self {
inner: CString::new(s).expect("thread name may not contain interior null bytes"),
}
}
}
impl ThreadName {
pub fn as_cstr(&self) -> Option<&CStr> {
match self {
ThreadName::Main => Some(c"main"),
ThreadName::Other(other) => Some(other),
ThreadName::Unnamed => None,
}
}

pub fn as_str(&self) -> Option<&str> {
// SAFETY: `as_cstr` can only return `Some` for a fixed CStr or a `ThreadNameString`,
// which is guaranteed to be UTF-8.
self.as_cstr().map(|s| unsafe { str::from_utf8_unchecked(s.to_bytes()) })
}
}
}
pub(crate) use thread_name_string::ThreadNameString;

/// The internal representation of a `Thread` handle
struct Inner {
name: ThreadName, // Guaranteed to be UTF-8
static MAIN_THREAD_INFO: SyncUnsafeCell<(MaybeUninit<ThreadId>, MaybeUninit<Parker>)> =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could now move this into new_main, but I really don't want to continue letting you suffer with this so we'll leave it as-is

SyncUnsafeCell::new((MaybeUninit::uninit(), MaybeUninit::uninit()));

/// The internal representation of a `Thread` that is not the main thread.
struct OtherInner {
name: Option<ThreadNameString>,
id: ThreadId,
parker: Parker,
}

/// The internal representation of a `Thread` handle.
#[derive(Clone)]
enum Inner {
/// Represents the main thread. May only be constructed by Thread::new_main.
Main(&'static (ThreadId, Parker)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that the main thread is a static reference, why not just have this be an Option<Pin<Arc<OtherInner>>>? Every None match can refer to the static.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/// Represents any other thread.
Other(Pin<Arc<OtherInner>>),
}

impl Inner {
fn parker(self: Pin<&Self>) -> Pin<&Parker> {
unsafe { Pin::map_unchecked(self, |inner| &inner.parker) }
fn id(&self) -> ThreadId {
match self {
Self::Main((thread_id, _)) => *thread_id,
Self::Other(other) => other.id,
}
}

fn cname(&self) -> Option<&CStr> {
match self {
Self::Main(_) => Some(c"main"),
Self::Other(other) => other.name.as_deref(),
}
}

fn name(&self) -> Option<&str> {
match self {
Self::Main(_) => Some("main"),
Self::Other(other) => other.name.as_ref().map(ThreadNameString::as_str),
}
}

fn into_raw(self) -> *const () {
match self {
// Just return the pointer to `MAIN_THREAD_INFO`.
Self::Main(ptr) => crate::ptr::from_ref(ptr).cast(),
Self::Other(arc) => {
// Safety: We only expose an opaque pointer, which maintains the `Pin` invariant.
let inner = unsafe { Pin::into_inner_unchecked(arc) };
Arc::into_raw(inner) as *const ()
}
}
}

/// # Safety
///
/// See [`Thread::from_raw`].
unsafe fn from_raw(ptr: *const ()) -> Self {
// If the pointer is to `MAIN_THREAD_INFO`, we know it is the `Main` variant.
if crate::ptr::eq(ptr.cast(), &MAIN_THREAD_INFO) {
Self::Main(unsafe { &*ptr.cast() })
} else {
// Safety: Upheld by caller
Self::Other(unsafe { Pin::new_unchecked(Arc::from_raw(ptr as *const OtherInner)) })
}
}

fn parker(&self) -> Pin<&Parker> {
match self {
Self::Main((_, parker_ref)) => Pin::static_ref(parker_ref),
Self::Other(inner) => unsafe {
Pin::map_unchecked(inner.as_ref(), |inner| &inner.parker)
},
}
}
}

Expand All @@ -1314,41 +1366,55 @@ impl Inner {
/// docs of [`Builder`] and [`spawn`] for more details.
///
/// [`thread::current`]: current::current
pub struct Thread {
inner: Pin<Arc<Inner>>,
}
pub struct Thread(Inner);

impl Thread {
/// Used only internally to construct a thread object without spawning.
pub(crate) fn new(id: ThreadId, name: String) -> Thread {
Self::new_inner(id, ThreadName::Other(name.into()))
Self::new_inner(id, Some(ThreadNameString::from(name)))
}

pub(crate) fn new_unnamed(id: ThreadId) -> Thread {
Self::new_inner(id, ThreadName::Unnamed)
Self::new_inner(id, None)
}

/// Constructs the thread handle for the main thread.
pub(crate) fn new_main(id: ThreadId) -> Thread {
Self::new_inner(id, ThreadName::Main)
/// Used in runtime to construct main thread
///
/// # Safety
///
/// This must only ever be called once, and must be called on the main thread.
pub(crate) unsafe fn new_main(thread_id: ThreadId) -> Thread {
// Safety: As this is only called once and on the main thread, nothing else is accessing MAIN_THREAD_INFO
// as the only other read occurs in `main_thread_info` *after* the main thread has been constructed,
// and this function is the only one that constructs the main thread.
//
// Pre-main thread spawning cannot hit this either, as the caller promises that this is only called on the main thread.
let main_thread_info = unsafe { &mut *MAIN_THREAD_INFO.get() };

unsafe { Parker::new_in_place((&raw mut main_thread_info.1).cast()) };
main_thread_info.0.write(thread_id);

// Store a `'static` ref to the initialised ThreadId and Parker,
// to avoid having to repeatedly prove initialisation.
Self(Inner::Main(unsafe { &*MAIN_THREAD_INFO.get().cast() }))
}

fn new_inner(id: ThreadId, name: ThreadName) -> Thread {
fn new_inner(id: ThreadId, name: Option<ThreadNameString>) -> Thread {
// We have to use `unsafe` here to construct the `Parker` in-place,
// which is required for the UNIX implementation.
//
// SAFETY: We pin the Arc immediately after creation, so its address never
// changes.
let inner = unsafe {
let mut arc = Arc::<Inner>::new_uninit();
let mut arc = Arc::<OtherInner>::new_uninit();
let ptr = Arc::get_mut_unchecked(&mut arc).as_mut_ptr();
(&raw mut (*ptr).name).write(name);
(&raw mut (*ptr).id).write(id);
Parker::new_in_place(&raw mut (*ptr).parker);
Pin::new_unchecked(arc.assume_init())
};

Thread { inner }
Self(Inner::Other(inner))
}

/// Like the public [`park`], but callable on any handle. This is used to
Expand All @@ -1357,7 +1423,7 @@ impl Thread {
/// # Safety
/// May only be called from the thread to which this handle belongs.
pub(crate) unsafe fn park(&self) {
unsafe { self.inner.as_ref().parker().park() }
unsafe { self.0.parker().park() }
}

/// Atomically makes the handle's token available if it is not already.
Expand Down Expand Up @@ -1393,7 +1459,7 @@ impl Thread {
#[stable(feature = "rust1", since = "1.0.0")]
#[inline]
pub fn unpark(&self) {
self.inner.as_ref().parker().unpark();
self.0.parker().unpark();
}

/// Gets the thread's unique identifier.
Expand All @@ -1413,7 +1479,7 @@ impl Thread {
#[stable(feature = "thread_id", since = "1.19.0")]
#[must_use]
pub fn id(&self) -> ThreadId {
self.inner.id
self.0.id()
}

/// Gets the thread's name.
Expand Down Expand Up @@ -1456,7 +1522,11 @@ impl Thread {
#[stable(feature = "rust1", since = "1.0.0")]
#[must_use]
pub fn name(&self) -> Option<&str> {
self.inner.name.as_str()
self.0.name()
}

fn cname(&self) -> Option<&CStr> {
self.0.cname()
}

/// Consumes the `Thread`, returning a raw pointer.
Expand All @@ -1480,9 +1550,7 @@ impl Thread {
/// ```
#[unstable(feature = "thread_raw", issue = "97523")]
pub fn into_raw(self) -> *const () {
// Safety: We only expose an opaque pointer, which maintains the `Pin` invariant.
let inner = unsafe { Pin::into_inner_unchecked(self.inner) };
Arc::into_raw(inner) as *const ()
self.0.into_raw()
}

/// Constructs a `Thread` from a raw pointer.
Expand All @@ -1504,11 +1572,7 @@ impl Thread {
#[unstable(feature = "thread_raw", issue = "97523")]
pub unsafe fn from_raw(ptr: *const ()) -> Thread {
// Safety: Upheld by caller.
unsafe { Thread { inner: Pin::new_unchecked(Arc::from_raw(ptr as *const Inner)) } }
}

fn cname(&self) -> Option<&CStr> {
self.inner.name.as_cstr()
unsafe { Thread(Inner::from_raw(ptr)) }
}
}

Expand Down
8 changes: 4 additions & 4 deletions tests/debuginfo/thread.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,15 @@
// cdb-check:join_handle,d [Type: std::thread::JoinHandle<tuple$<> >]
// cdb-check: [...] __0 [Type: std::thread::JoinInner<tuple$<> >]
//
// cdb-command:dx t,d
// cdb-command:dx -r3 t,d
// cdb-check:t,d : [...] [Type: std::thread::Thread *]
// cdb-check:[...] inner [...][Type: core::pin::Pin<alloc::sync::Arc<std::thread::Inner,alloc::alloc::Global> >]
// cdb-check: [...] __0 : Other [Type: enum2$<std::thread::Inner>]
// cdb-check: [...] __0 [Type: core::pin::Pin<alloc::sync::Arc<std::thread::OtherInner,[...]> >]

use std::thread;

#[allow(unused_variables)]
fn main()
{
fn main() {
let join_handle = thread::spawn(|| {
println!("Initialize a thread");
});
Expand Down
1 change: 1 addition & 0 deletions tests/rustdoc/demo-allocator-54478.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
//! }
//!
//! fn main() {
//! drop(String::from("An allocation"));
//! assert!(unsafe { HIT });
//! }
//! ```
Loading