Tracking issue for cleaning up std's thread_local implementation details #110897

m-ou-se · 2023-04-27T13:15:41Z

joboet · 2023-04-27T13:33:45Z

LazyKeyInner unfortunately can't just be OnceCell because of reentrant initialization: Because OnceCell::get_or_init returns a shared reference to the data that lives for the duration of self, it cannot change the value after the first inner initialization. LazyKeyInner can do that (and does) because references cannot outlive LocalKey::with.

m-ou-se · 2023-04-27T13:43:13Z

@joboet In what situation would that mem::replace call replace a Some instead of a None? Is that the right behaviour? Do we have a test for that situation?

joboet · 2023-04-27T13:54:36Z

It's very cursed, but this works. I don't think it's currently tested, however.

m-ou-se · 2023-04-27T13:56:25Z

That just seems like a bug to me. A (thread local) static &'static str (not mut, not a cell) shouldn't be able to have different values at different times (on the same thread).

joboet · 2023-04-27T14:02:31Z

Maybe 😄? It's sound however, as there can be no references to the data from the first initialization when the new value is written.

m-ou-se · 2023-04-27T14:06:40Z

It's sound

Sure, but so would OnceCell be. ^^ And I think users should be able to safely assume that an immutable static never changes. (And as a bonus we get to delete code.)

m-ou-se · 2023-04-27T14:31:00Z

I did some software archeology and found the original issue and PR that introduced that mem::replace call: #30228 and #30267.

The issue was originally about thread locals, but then most of the discussion was about drop on assignment in general. The issue description suggests:

It should either drop one of the two values (perhaps after putting the TLS slot in "destructor being called" mode) or panic, if a multiple initialization condition is detected.

Without a preference for any of these solutions. The PR also doesn't include a reason for picking one solution over the other.

So I don't think the mem::replace behaviour was purposely picked over another solution (like dropping the other value or panicking).

workingjubilee · 2023-04-27T22:55:10Z

…-key, r=cuviper Remove unused std::sys_common::thread_local_key::Key Part of rust-lang#110897 This `Key` type seems unused. Let's remove it and see if anything explodes. :)

m-ou-se · 2023-04-28T08:36:01Z

In this case I don't think anyone actually relies on LazyKeyInner being a OnceOrMaybeTwiceIfYouTryReallyHardCell instead of just a OnceCell. ^^

joboet · 2023-04-28T08:46:14Z

I'd advocate for a crater run, just in case, but yes.

workingjubilee · 2023-04-29T06:02:34Z

It is extremely unlikely someone is going to have tests that stress this exact point sufficiently for it to be revealed.

joboet · 2023-04-29T07:27:38Z

Ok, yes, it's actually part of the documentation. Nevermind, then.

thomcc · 2023-06-05T16:45:50Z

Thanks for this, I've been very unhappy with the thread_local! impl for a long time (@ChrisDenton has had to listen to me gripe about it many times). It's something I was hoping to get to back last fall, but then got a dayjob that takes up much of my time. I think you hit the big issues. Here are some other ones

Fixing our (mis)use of __dso_handle/__cxa_thread_atexit_impl (thread_local! dtor registration can pass wrong __dso_handle if dynamically linked #88737). That issue has a lot of writing and some of it is made with incomplete understanding of the issue (I think), so I've summarized in a gist: https://gist.github.com/thomcc/8c4a8003cf1a282949a539e899cb45aa (initially it was going to be part of this comment).
Lots of code does unnecessary reads from #[thread_local]s, which, while generally fast, can be very slow in cases like dynamic libraries (where it has to call a function that sometimes takes a lock, walks linked lists, etc).

Some of this is just a quirk of the compiler (https://doc.rust-lang.org/nightly/src/std/sys/common/thread_local/fast_local.rs.html#182-198), but our "fast" impl actually uses two #[thread_local] variables now, which guarantees such behavior (that said, I only realized this yesterday and intend to fix it today).
Related to that code, the non-const version ends up pessimizing things (especially the !needs_drop case) to avoid extra TLS reads on macOS:

rust/library/std/src/sys/common/thread_local/fast_local.rs

Line 182 in 7452822

// `try_initialize` is only called once per fast thread local variable,

.

This might be fixed in LLVM now, and if not could be worked around by using either compiler_fence or (if all else fails) black_box.

(Alternatively, it's possible for us to do semi-dubious things with the fact that std is allowed to unstabley use needs_drop<T>() as a const fn...)

There are a few others I had written down (multiple implementations of the manual tls-dtor list) but you touch on them and it might be outdated given #109858.

…uviper Remove unused std::sys_common::thread_local_key::Key Part of rust-lang/rust#110897 This `Key` type seems unused. Let's remove it and see if anything explodes. :)

…n, r=dtolnay Broaden the consequences of recursive TLS initialization This PR updates the documentation of `LocalKey` to clearly disallow the behaviour described in [this comment](rust-lang#110897 (comment)). This allows using `OnceCell` for the lazy initialization of TLS variables, which panics on reentrant initialization instead of updating the value like TLS variables currently do. `@rustbot` label +T-libs-api r? `@m-ou-se`

…n, r=dtolnay Broaden the consequences of recursive TLS initialization This PR updates the documentation of `LocalKey` to clearly disallow the behaviour described in [this comment](rust-lang#110897 (comment)). This allows using `OnceCell` for the lazy initialization of TLS variables, which panics on reentrant initialization instead of updating the value like TLS variables currently do. ``@rustbot`` label +T-libs-api r? ``@m-ou-se``

Rollup merge of rust-lang#116172 - joboet:recursive_tls_initialization, r=dtolnay Broaden the consequences of recursive TLS initialization This PR updates the documentation of `LocalKey` to clearly disallow the behaviour described in [this comment](rust-lang#110897 (comment)). This allows using `OnceCell` for the lazy initialization of TLS variables, which panics on reentrant initialization instead of updating the value like TLS variables currently do. ``@rustbot`` label +T-libs-api r? ``@m-ou-se``

Rewrite native thread-local storage (part of rust-lang#110897) The current native thread-local storage implementation has become quite messy, uses indescriptive names and unnecessarily adds code to the macro expansion. This PR tries to fix that by using a new implementation that also allows more layout optimizations and potentially increases performance by eliminating unnecessary TLS accesses. This does not change the recursive initialization behaviour I described in [this comment](rust-lang#110897 (comment)), so it should be a library-only change. Changing that behaviour should be quite easy now, however. r? `@m-ou-se` `@rustbot` label +T-libs

Simplify key-based thread locals This PR simplifies key-based thread-locals by: * unifying the macro expansion of `const` and non-`const` initializers * reducing the amount of code in the expansion * simply reallocating on recursive initialization instead of going through `LazyKeyInner` * replacing `catch_unwind` with the shared `abort_on_dtor_unwind` It does not change the initialization behaviour described in rust-lang#110897.

Rewrite native thread-local storage (part of rust-lang#110897) The current native thread-local storage implementation has become quite messy, uses indescriptive names and unnecessarily adds code to the macro expansion. This PR tries to fix that by using a new implementation that also allows more layout optimizations and potentially increases performance by eliminating unnecessary TLS accesses. This does not change the recursive initialization behaviour I described in [this comment](rust-lang#110897 (comment)), so it should be a library-only change. Changing that behaviour should be quite easy now, however. r? `@m-ou-se` `@rustbot` label +T-libs

Rewrite TLS on platforms without threads The saga of rust-lang#110897 continues! r? `@m-ou-se` if you have time

Rewrite native thread-local storage (part of #110897) The current native thread-local storage implementation has become quite messy, uses indescriptive names and unnecessarily adds code to the macro expansion. This PR tries to fix that by using a new implementation that also allows more layout optimizations and potentially increases performance by eliminating unnecessary TLS accesses. This does not change the recursive initialization behaviour I described in [this comment](rust-lang/rust#110897 (comment)), so it should be a library-only change. Changing that behaviour should be quite easy now, however. r? `@m-ou-se` `@rustbot` label +T-libs

Simplify key-based thread locals This PR simplifies key-based thread-locals by: * unifying the macro expansion of `const` and non-`const` initializers * reducing the amount of code in the expansion * simply reallocating on recursive initialization instead of going through `LazyKeyInner` * replacing `catch_unwind` with the shared `abort_on_dtor_unwind` It does not change the initialization behaviour described in rust-lang#110897.

Rewrite native thread-local storage (part of #110897) The current native thread-local storage implementation has become quite messy, uses indescriptive names and unnecessarily adds code to the macro expansion. This PR tries to fix that by using a new implementation that also allows more layout optimizations and potentially increases performance by eliminating unnecessary TLS accesses. This does not change the recursive initialization behaviour I described in [this comment](rust-lang/rust#110897 (comment)), so it should be a library-only change. Changing that behaviour should be quite easy now, however. r? `@m-ou-se` `@rustbot` label +T-libs

@rustbot

As discovered by Mara in rust-lang#110897, our TLS implementation is a total mess. In the past months, I have simplified the actual macros and their expansions, but the majority of the complexity comes from the platform-specific support code needed to create keys and register destructors. In keeping with rust-lang#117276, I have therefore moved all of the `thread_local_key`/`thread_local_dtor` functions to the `thread_local` module in `sys` and given them a new structure, so that future porters of `std` can simply mix-and-match the existing code instead of having to copy the same (bad) implementation everywhere. The new structure should become obvious when looking at `sys/thread_local/mod.rs`. Unfortunately, the documentation changes associated with the refactoring have made this PR rather large. That said, this contains no functional changes except for two small ones: * the key-based destructor fallback now, by virtue of sharing the implementation used by macOS and others, stores its list in a `#[thread_local]` static instead of in the key, eliminating one indirection layer and drastically simplifying its code. * I've switched over ZKVM (tier 3) to use the same implementation as WebAssembly, as the implementation was just a way worse version of that Please let me know if I can make this easier to review! I know these large PRs aren't optimal, but I couldn't think of any good intermediate steps. @rustbot label +A-thread-locals

@rustbot

As discovered by Mara in rust-lang#110897, our TLS implementation is a total mess. In the past months, I have simplified the actual macros and their expansions, but the majority of the complexity comes from the platform-specific support code needed to create keys and register destructors. In keeping with rust-lang#117276, I have therefore moved all of the `thread_local_key`/`thread_local_dtor` modules to the `thread_local` module in `sys` and merged them into a new structure, so that future porters of `std` can simply mix-and-match the existing code instead of having to copy the same (bad) implementation everywhere. The new structure should become obvious when looking at `sys/thread_local/mod.rs`. Unfortunately, the documentation changes associated with the refactoring have made this PR rather large. That said, this contains no functional changes except for two small ones: * the key-based destructor fallback now, by virtue of sharing the implementation used by macOS and others, stores its list in a `#[thread_local]` static instead of in the key, eliminating one indirection layer and drastically simplifying its code. * I've switched over ZKVM (tier 3) to use the same implementation as WebAssembly, as the implementation was just a way worse version of that Please let me know if I can make this easier to review! I know these large PRs aren't optimal, but I couldn't think of any good intermediate steps. @rustbot label +A-thread-locals

… r=Mark-Simulacrum std: refactor the TLS implementation As discovered by Mara in rust-lang#110897, our TLS implementation is a total mess. In the past months, I have simplified the actual macros and their expansions, but the majority of the complexity comes from the platform-specific support code needed to create keys and register destructors. In keeping with rust-lang#117276, I have therefore moved all of the `thread_local_key`/`thread_local_dtor` modules to the `thread_local` module in `sys` and merged them into a new structure, so that future porters of `std` can simply mix-and-match the existing code instead of having to copy the same (bad) implementation everywhere. The new structure should become obvious when looking at `sys/thread_local/mod.rs`. Unfortunately, the documentation changes associated with the refactoring have made this PR rather large. That said, this contains no functional changes except for two small ones: * the key-based destructor fallback now, by virtue of sharing the implementation used by macOS and others, stores its list in a `#[thread_local]` static instead of in the key, eliminating one indirection layer and drastically simplifying its code. * I've switched over ZKVM (tier 3) to use the same implementation as WebAssembly, as the implementation was just a way worse version of that Please let me know if I can make this easier to review! I know these large PRs aren't optimal, but I couldn't think of any good intermediate steps. `@rustbot` label +A-thread-locals

…=Mark-Simulacrum std: refactor the TLS implementation As discovered by Mara in rust-lang#110897, our TLS implementation is a total mess. In the past months, I have simplified the actual macros and their expansions, but the majority of the complexity comes from the platform-specific support code needed to create keys and register destructors. In keeping with rust-lang#117276, I have therefore moved all of the `thread_local_key`/`thread_local_dtor` modules to the `thread_local` module in `sys` and merged them into a new structure, so that future porters of `std` can simply mix-and-match the existing code instead of having to copy the same (bad) implementation everywhere. The new structure should become obvious when looking at `sys/thread_local/mod.rs`. Unfortunately, the documentation changes associated with the refactoring have made this PR rather large. That said, this contains no functional changes except for two small ones: * the key-based destructor fallback now, by virtue of sharing the implementation used by macOS and others, stores its list in a `#[thread_local]` static instead of in the key, eliminating one indirection layer and drastically simplifying its code. * I've switched over ZKVM (tier 3) to use the same implementation as WebAssembly, as the implementation was just a way worse version of that Please let me know if I can make this easier to review! I know these large PRs aren't optimal, but I couldn't think of any good intermediate steps. `@rustbot` label +A-thread-locals

robertbastian · 2024-06-26T14:30:42Z

This might have caused #126992

std: separate TLS key creation from TLS access Currently, `std` performs an atomic load to get the OS key on every access to `StaticKey` even when the key is already known. This PR thus replaces `StaticKey` with the platform-specific `get` and `set` function and a new `LazyKey` type that acts as a `LazyLock<Key>`, allowing the reuse of the retreived key for multiple accesses. Related to rust-lang#110897.

Rollup merge of rust-lang#126953 - joboet:lazy_key, r=jhpratt std: separate TLS key creation from TLS access Currently, `std` performs an atomic load to get the OS key on every access to `StaticKey` even when the key is already known. This PR thus replaces `StaticKey` with the platform-specific `get` and `set` function and a new `LazyKey` type that acts as a `LazyLock<Key>`, allowing the reuse of the retreived key for multiple accesses. Related to rust-lang#110897.

m-ou-se added C-cleanup Category: PRs that clean code up or issues documenting cleanup. A-thread-locals Area: Thread local storage (TLS) T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 27, 2023

m-ou-se self-assigned this Apr 27, 2023

m-ou-se mentioned this issue Apr 27, 2023

Remove unused std::sys_common::thread_local_key::Key #110898

Merged

m-ou-se mentioned this issue Jun 5, 2023

Avoid unwind across extern "C" in thread_local::fast_local #112292

Merged

saethlin mentioned this issue Jun 7, 2023

Data reachable from thread-local storage of the main thread should not be considered leaked rust-lang/miri#2881

Closed

This was referenced Sep 24, 2023

Rewrite native thread-local storage #116123

Merged

std::thread::local internals allow race conditions in safe but unstable code. #43733

Closed

Broaden the consequences of recursive TLS initialization #116172

Merged

joboet mentioned this issue Oct 17, 2023

Unify TLS destructor list implementations #116850

Closed

joboet mentioned this issue Dec 28, 2023

New Lint: [thread_local_initializer_can_be_made_const] rust-lang/rust-clippy#12026

Merged

joboet mentioned this issue Feb 26, 2024

Use volatile access instead of #[used] for on_tls_callback #121596

Merged

joboet mentioned this issue Mar 14, 2024

Simplify key-based thread locals #122494

Merged

ChrisDenton mentioned this issue Apr 7, 2024

Miri sometimes reports memory leak in thread-local storage on *-pc-windows-gnu #123583

Closed

joboet mentioned this issue Apr 10, 2024

Rewrite TLS on platforms without threads #123724

Merged

bors added a commit to rust-lang-ci/rust that referenced this issue May 24, 2024

Auto merge of rust-lang#123724 - joboet:static_tls, r=m-ou-se

78dd504

Rewrite TLS on platforms without threads The saga of rust-lang#110897 continues! r? `@m-ou-se` if you have time

joboet mentioned this issue Jun 15, 2024

std: refactor the TLS implementation #126523

Merged

joboet mentioned this issue Jun 25, 2024

std: separate TLS key creation from TLS access #126953

Merged

correabuscar mentioned this issue Jul 17, 2024

LocalKey dropped after calling std::process::exit() #127637

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tracking issue for cleaning up std's thread_local implementation details #110897

Tracking issue for cleaning up std's thread_local implementation details #110897

m-ou-se commented Apr 27, 2023 •

edited by joboet

Loading

joboet commented Apr 27, 2023 •

edited

Loading

m-ou-se commented Apr 27, 2023

joboet commented Apr 27, 2023

m-ou-se commented Apr 27, 2023

joboet commented Apr 27, 2023

m-ou-se commented Apr 27, 2023

m-ou-se commented Apr 27, 2023

workingjubilee commented Apr 27, 2023

m-ou-se commented Apr 28, 2023

joboet commented Apr 28, 2023

workingjubilee commented Apr 29, 2023

joboet commented Apr 29, 2023

thomcc commented Jun 5, 2023 •

edited

Loading

robertbastian commented Jun 26, 2024

Tracking issue for cleaning up std's thread_local implementation details #110897

Tracking issue for cleaning up std's thread_local implementation details #110897

Comments

m-ou-se commented Apr 27, 2023 • edited by joboet Loading

joboet commented Apr 27, 2023 • edited Loading

m-ou-se commented Apr 27, 2023

joboet commented Apr 27, 2023

m-ou-se commented Apr 27, 2023

joboet commented Apr 27, 2023

m-ou-se commented Apr 27, 2023

m-ou-se commented Apr 27, 2023

workingjubilee commented Apr 27, 2023

m-ou-se commented Apr 28, 2023

joboet commented Apr 28, 2023

workingjubilee commented Apr 29, 2023

joboet commented Apr 29, 2023

thomcc commented Jun 5, 2023 • edited Loading

robertbastian commented Jun 26, 2024

m-ou-se commented Apr 27, 2023 •

edited by joboet

Loading

joboet commented Apr 27, 2023 •

edited

Loading

thomcc commented Jun 5, 2023 •

edited

Loading