std: refactor the TLS implementation by joboet · Pull Request #126523 · rust-lang/rust

joboet · 2024-06-15T15:39:58Z

As discovered by Mara in #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 #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

rustbot · 2024-06-15T15:40:06Z

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
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

rustbot · 2024-06-15T15:40:10Z

The Miri subtree was changed

cc @rust-lang/miri

@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

rustbot · 2024-06-15T15:47:52Z

The Miri subtree was changed

cc @rust-lang/miri

Mark-Simulacrum

I didn't review the implementations in detail (I think they are just moved, not actually changed, right?) -- left some comments inline, but most are non-blocking, just curiosity/slight improvements.

In general I'm happy to r+ with some of the nits fixed/questions answered.

Mark-Simulacrum · 2024-06-16T20:31:38Z

+            pub(super) use linux::register;
+            pub(super) use list::run;
+        } else if #[cfg(all(
+            target_thread_local,


Since this is present in both lists, should we outline it and just #[cfg] the whole destructors module? Or are we glob-importing it or something?

Sure, that makes sense.

Mark-Simulacrum · 2024-06-16T20:42:22Z

+    #[cfi_encoding = "i"]
+    #[repr(transparent)]
+    #[allow(non_camel_case_types)]
+    pub struct c_int(#[allow(dead_code)] pub libc::c_int);


Is there a reason this doesn't use the same c_int? (libc vs core::ffi)

I don't think so.

Mark-Simulacrum · 2024-06-16T20:49:45Z

+//! Instead, we use one TLS key to register a callback which will run
+//! iterate through the destructor list.


Suggested change

//! Instead, we use one TLS key to register a callback which will run

//! iterate through the destructor list.

//! Instead, we use one TLS key to register a callback which will

//! iterate through the destructor list.

Mark-Simulacrum · 2024-06-16T20:51:29Z

+    static DTORS: StaticKey = StaticKey::new(Some(run));
+
+    // Setting the key value to something other than NULL will result in the
+    // destructor being run at thread exit.


Maybe StaticKey is doing something I don't really understand (yet), but the idea that we don't have a way to register TLS destructors and yet can register a callback on thread exit doesn't really connect for me. Maybe we want to say that we can't register more than one destructor or something? (But then, how does this work when there's non-Rust code in play)

joboet · 2024-06-17T14:00:38Z

I updated the documentation, it should be a bit clearer now.
@rustbot ready

Mark-Simulacrum · 2024-06-23T18:46:29Z

@bors r+

bors · 2024-06-23T18:46:32Z

📌 Commit a2f078d has been approved by Mark-Simulacrum

It is now in the queue for this repository.

workingjubilee · 2024-06-24T01:59:36Z

@bors rollup=never

Big refactor of TLS, don't want to have to guess when bisecting to this.

bors · 2024-06-24T04:18:59Z

⌛ Testing commit a2f078d with merge 45edc40...

bors · 2024-06-24T05:05:24Z

💔 Test failed - checks-actions

joboet · 2024-06-24T14:41:03Z

I forgot that Emscripten is also in the UNIX family...
@bors r=@Mark-Simulacrum

bors · 2024-06-24T14:41:05Z

📌 Commit 50a02ed has been approved by Mark-Simulacrum

It is now in the queue for this repository.

bors · 2024-06-24T15:55:32Z

⌛ Testing commit 50a02ed with merge 5a3e2a4...

bors · 2024-06-24T18:34:47Z

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 5a3e2a4 to master...

rust-timer · 2024-06-24T19:49:58Z

Finished benchmarking commit (5a3e2a4): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

Results (primary -0.1%, secondary -0.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.1%	[-0.1%, -0.1%]	4
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.0%]	35
All ❌✅ (primary)	-0.1%	[-0.1%, -0.1%]	4

Bootstrap: 692.546s -> 692.589s (0.01%)
Artifact size: 326.77 MiB -> 326.77 MiB (-0.00%)

rustbot assigned Mark-Simulacrum Jun 15, 2024

joboet mentioned this pull request Jun 15, 2024

Unify TLS destructor list implementations #116850

Closed

joboet added the A-thread-locals Area: Thread local storage (TLS) label Jun 15, 2024

This comment has been minimized.

Sign in to view

joboet force-pushed the the_great_big_tls_refactor branch from ff3e202 to f3facf1 Compare June 15, 2024 15:47

This was referenced Jun 15, 2024

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

Closed

Tracking Issue for restructuring std::sys #117276

Open

Mark-Simulacrum requested changes Jun 16, 2024

View reviewed changes

rustbot 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 Jun 16, 2024

joboet added 4 commits June 17, 2024 12:41

std: simplify #[cfg]s for TLS

d70f071

std: use the c_int from core::ffi instead of libc

b2f29ed

std: update TLS module documentation

35f050b

std: rename module for clarity

32f9b8b

rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 17, 2024

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

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 23, 2024

This comment has been minimized.

Sign in to view

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 24, 2024

std: fix wasm builds

50a02ed

joboet force-pushed the the_great_big_tls_refactor branch from a2f078d to 50a02ed Compare June 24, 2024 14:37

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 24, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 24, 2024

bors merged commit 5a3e2a4 into rust-lang:master Jun 24, 2024

rustbot added this to the 1.81.0 milestone Jun 24, 2024

seritools mentioned this pull request Jul 1, 2024

Notes for future upgrade to Rust 1.84 rust9x/rust#19

Closed

1 task

		//! Instead, we use one TLS key to register a callback which will run
		//! iterate through the destructor list.

Uh oh!

Conversation

joboet commented Jun 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Jun 15, 2024

Uh oh!

rustbot commented Jun 15, 2024

Uh oh!

This comment has been minimized.

rustbot commented Jun 15, 2024

Uh oh!

Mark-Simulacrum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

joboet commented Jun 17, 2024

Uh oh!

Mark-Simulacrum commented Jun 23, 2024

Uh oh!

bors commented Jun 23, 2024

Uh oh!

workingjubilee commented Jun 24, 2024

Uh oh!

bors commented Jun 24, 2024

Uh oh!

This comment has been minimized.

bors commented Jun 24, 2024

Uh oh!

joboet commented Jun 24, 2024

Uh oh!

bors commented Jun 24, 2024

Uh oh!

bors commented Jun 24, 2024

Uh oh!

bors commented Jun 24, 2024

Uh oh!

rust-timer commented Jun 24, 2024

Overall result: no relevant changes - no action needed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

joboet commented Jun 15, 2024 •

edited

Loading