std: lazily allocate the main thread handle #132654

joboet · 2024-11-05T17:27:12Z

#123550 eliminated the allocation of the main thread handle, but at the cost of greatly increased complexity. This PR proposes another approach: Instead of creating the main thread handle itself, the runtime simply remembers the thread ID of the main thread. The main thread handle is then only allocated when it is used, using the same lazy-initialization mechanism as for non-runtime use of thread::current, and the name method uses the thread ID to identify the main thread handle and return the correct name ("main") for it.

Thereby, we also allow accessing thread::current before main: as the runtime no longer tries to install its own handle, this will no longer trigger an abort. Rather, the name returned from name will only be "main" after the runtime initialization code has run, but I think that is acceptable.

This new approach also requires some changes to the signal handling code, as calling thread::current would now allocate when called on the main thread, which is not acceptable. I fixed this by adding a new function (with_current_name) that performs all the naming logic without allocation or without initializing the thread ID (which could allocate on some platforms).

Reverts #123550, CC @GnomedDev

rustbot · 2024-11-05T17:27:20Z

r? @ChrisDenton

rustbot has assigned @ChrisDenton.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

workingjubilee · 2024-11-05T18:44:59Z

I've also moved the Thread code into its own module, the size of thread/mod.rs is just getting out of hand.

std::thread::thread here we come?

EDIT: I was right!

ChrisDenton

I'd be happy with this approach but it would be nice to have some consensus on it to (hopefully) avoid further churn. I won't insist on it though.

ChrisDenton · 2024-11-05T18:40:24Z

library/std/src/thread/mod.rs

-    #[cfg(not(target_thread_local))]
+    #[allow(unused)]


Why is this change is needed?

Because this is also used when 64-bit atomics are unavailable.

Is there any more specific cfg that can be used? #[allow(unused)], while sometimes necessary, makes me nervous because it could silently become actual dead code in the future.

ChrisDenton · 2024-11-05T18:41:42Z

library/std/src/thread/thread.rs

+    // INVARIANT: must be valid UTF-8
+    name: Option<CString>,


This change seems worse. Having an invariant that can be violated in safe code is not great.

I also wondered if it made sense to have a new-type that held both invariants (UTF-8 & c-string \0 ending) ?

Utf8CStr is a fairly common type in the ecosystem, it would make sense for us to start tinkering with providing it.

ChrisDenton · 2024-11-05T18:59:21Z

Also could you separate the file move into a different commit? It's easier to diff that way.

joboet · 2024-11-07T16:15:42Z

Blocked on the efficiency fix in #132730, without that, we'd be creating a new Thread for every blocking RwLock call on macOS.

@rustbot label +S-blocked

bors · 2024-11-20T02:14:07Z

☔ The latest upstream changes (presumably #133219) made this pull request unmergeable. Please resolve the merge conflicts.

joboet · 2024-11-25T10:48:13Z

I've undone the Thread move (that's probably worth another PR) and fixed some other things related to signal handling (thread::current now allocates on the main thread, so we mustn't call it in the signal handler).

@rustbot ready

If the variable does not need a destructor, `std` uses racy initialization for creating TLS keys on Windows. With just the right timing, this can lead to `TlsFree` being called. Unfortunately, with rust-lang#132654 this is hit quite often, so miri should definitely support `TlsFree` ([documentation](https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-tlsfree)). I'm filing this here instead of in the miri repo so that rust-lang#132654 isn't blocked for so long.

joboet · 2024-11-25T15:29:45Z

Blocked on #133457. This PR times everything just right so that the TlsFree codepath is hit in the tests.

RalfJung · 2024-11-25T17:17:50Z

library/std/src/thread/mod.rs

-                // 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 ()
+pub(crate) fn with_current_name<F, R>(f: F) -> R


If this is called from signal handler context, I think there should be a doc comment here explicitly saying that the function may be called from signal handler context. It seems fairly easily to accidentally break this property in the future.

RalfJung · 2024-11-25T17:20:15Z

library/std/src/thread/mod.rs

    use crate::ffi::{CStr, CString};
+    use crate::str;

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


If this was a Cow<'static, CString>, could the main thread reference a "main\0" string literal and thus avoid a few else if main_thread::get() == Some(self.inner.id)?

The thread ID method has the advantage of working inside TLS destructors, so e.g. panicking in a TLS destructor on main now prints the correct name. And since we need the infrastructure anyway to detect the main thread in the first place, I figured this was better.

I find the duplication of logic unfortunate, and IMO it'd be worth reducing.

But I am just commenting as a bystander here, hopefully I never have to dig into this code and figure out how it holds together. ;)

Yeah, I'm planning to do a cleanup PR for this module next... it's just way to large to understand, even though the actual logic is mostly quite local.

The thread ID method has the advantage of working inside TLS destructors, so e.g. panicking in a TLS destructor on main now prints the correct name. And since we need the infrastructure anyway to detect the main thread in the first place, I figured this was better.

Sorry, that scenario wasn't the issue. The actual reason why this mustn't change is that there is now nothing stopping thread::current from being called before main, which would lead to an incorrect name being stored. The ID-based detection doesn't have this issue, because the name will be correct as soon as the runtime has initialized the ID.

Ah, that is subtle. More comments would be good. :)

RalfJung · 2024-11-25T17:28:23Z

library/std/src/thread/mod.rs

+                return f(Some("main"));
            }
+        } else if let Some(main) = main_thread::get()
+            && let Some(id) = current::id::get()
+            && id == main
+        {
+            return f(Some("main"));


This seems worth some comments explaining why "main" has two separate cases.

miri: implement `TlsFree` If the variable does not need a destructor, `std` uses racy initialization for creating TLS keys on Windows. With just the right timing, this can lead to `TlsFree` being called. Unfortunately, with rust-lang#132654 this is hit quite often, so miri should definitely support `TlsFree` ([documentation](https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-tlsfree)). I'm filing this here instead of in the miri repo so that rust-lang#132654 isn't blocked for so long.

Rollup merge of rust-lang#133457 - joboet:miri-tlsfree, r=saethlin miri: implement `TlsFree` If the variable does not need a destructor, `std` uses racy initialization for creating TLS keys on Windows. With just the right timing, this can lead to `TlsFree` being called. Unfortunately, with rust-lang#132654 this is hit quite often, so miri should definitely support `TlsFree` ([documentation](https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-tlsfree)). I'm filing this here instead of in the miri repo so that rust-lang#132654 isn't blocked for so long.

This reverts commit 0747f28.

rust-lang#123550 eliminated the allocation of the main thread handle, but at the cost of greatly increased complexity. This PR proposes another approach: Instead of creating the main thread handle itself, the runtime simply remembers the thread ID of the main thread. The main thread handle is then only allocated when it is used, using the same lazy-initialization mechanism as for non-runtime use of `thread::current`, and the `name` method uses the thread ID to identify the main thread handle and return the correct name ("main") for it. Thereby, we also allow accessing thread::current before main: as the runtime no longer tries to install its own handle, this will no longer trigger an abort. Rather, the name returned from name will only be "main" after the runtime initialization code has run, but I think that is acceptable. This new approach also requires some changes to the signal handling code, as calling `thread::current` would now allocate when called on the main thread, which is not acceptable. I fixed this by adding a new function (`with_current_name`) that performs all the naming logic without allocation or without initializing the thread ID (which could allocate on some platforms).

Originally authored by GnomedDev

joboet · 2024-11-27T13:41:10Z

I've rebased this on top of #133457 and added some comments explaining everything.

rustbot assigned ChrisDenton Nov 5, 2024

joboet changed the title ~~Lazy main~~ std: lazily allocate the main thread handle Nov 5, 2024

This comment has been minimized.

Sign in to view

joboet force-pushed the lazy_main branch from d26cacf to ee6cd82 Compare November 5, 2024 18:28

ChrisDenton reviewed Nov 5, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

rustbot added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Nov 7, 2024

joboet added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 7, 2024

joboet linked an issue Nov 12, 2024 that may be closed by this pull request

std::thread::Thread grew from one to two pointer sizes #132619

Open

joboet force-pushed the lazy_main branch from ee6cd82 to 8f94c51 Compare November 25, 2024 10:46

rustbot added O-unix Operating system: Unix-like O-windows Operating system: Windows labels Nov 25, 2024

This comment has been minimized.

Sign in to view

joboet force-pushed the lazy_main branch from 8f94c51 to f13ff4b Compare November 25, 2024 12:01

This comment has been minimized.

Sign in to view

joboet mentioned this pull request Nov 25, 2024

miri: implement TlsFree #133457

Merged

joboet added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Nov 25, 2024

RalfJung reviewed Nov 25, 2024

View reviewed changes

joboet added 3 commits November 27, 2024 14:07

Revert "Remove the Arc rt::init allocation for thread info"

6e0725e

This reverts commit 0747f28.

make sure that the allocator is actually called in allocator test

1861a0c

Originally authored by GnomedDev

joboet removed the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Nov 27, 2024

add comments explaining main thread identification

4617692

joboet force-pushed the lazy_main branch from 5e8caa0 to 4617692 Compare November 27, 2024 13:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

std: lazily allocate the main thread handle #132654

std: lazily allocate the main thread handle #132654

joboet commented Nov 5, 2024 •

edited

Loading

rustbot commented Nov 5, 2024

This comment has been minimized.

workingjubilee commented Nov 5, 2024 •

edited

Loading

ChrisDenton left a comment

ChrisDenton Nov 5, 2024

joboet Nov 6, 2024

ChrisDenton Nov 6, 2024

ChrisDenton Nov 5, 2024

fbstj Nov 5, 2024

workingjubilee Nov 6, 2024

ChrisDenton commented Nov 5, 2024

This comment has been minimized.

joboet commented Nov 7, 2024

bors commented Nov 20, 2024

joboet commented Nov 25, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

joboet commented Nov 25, 2024

RalfJung Nov 25, 2024

RalfJung Nov 25, 2024

joboet Nov 25, 2024 •

edited

Loading

RalfJung Nov 25, 2024

joboet Nov 25, 2024

joboet Nov 25, 2024

RalfJung Nov 25, 2024

RalfJung Nov 25, 2024

joboet commented Nov 27, 2024

std: lazily allocate the main thread handle #132654

Are you sure you want to change the base?

std: lazily allocate the main thread handle #132654

Conversation

joboet commented Nov 5, 2024 • edited Loading

rustbot commented Nov 5, 2024

This comment has been minimized.

workingjubilee commented Nov 5, 2024 • edited Loading

ChrisDenton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisDenton commented Nov 5, 2024

This comment has been minimized.

joboet commented Nov 7, 2024

bors commented Nov 20, 2024

joboet commented Nov 25, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

joboet commented Nov 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joboet Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joboet commented Nov 27, 2024

joboet commented Nov 5, 2024 •

edited

Loading

workingjubilee commented Nov 5, 2024 •

edited

Loading

joboet Nov 25, 2024 •

edited

Loading