Simplify key-based thread locals #122494

joboet · 2024-03-14T13:40:11Z

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 #110897.

rustbot · 2024-03-14T13:40:18Z

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

cuviper · 2024-03-19T23:09:33Z

r? @m-ou-se

^ since you hopefully remember more about this code from #110897.

m-ou-se · 2024-03-20T10:09:25Z

library/std/src/sys/thread_local/os_local.rs

+            // If the variable was recursively initialized, drop the old value.
+            // SAFETY: We cannot be inside a `LocalKey::with` scope, as the
+            // initializer has already returned and the next scope only starts
+            // after we return the pointer. Therefore, there can be no references
+            // to the old value.
+            drop(unsafe { Box::from_raw(old) });


I think it'd be good to swap this around (such that it behaves like OnceCell) in a future PR. (That PR will need an FCP and perhaps a crater run though.)

m-ou-se · 2024-03-20T10:15:28Z

This looks great.

@bors r+

bors · 2024-03-20T10:15:31Z

📌 Commit 5fd4a84 has been approved by m-ou-se

It is now in the queue for this repository.

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.

…llaumeGomez Rollup of 8 pull requests Successful merges: - rust-lang#122494 (Simplify key-based thread locals) - rust-lang#122644 (pattern analysis: add a custom test harness) - rust-lang#122723 (Use same file permissions for ar_archive_writer as the LLVM archive writer) - rust-lang#122729 (Relax SeqCst ordering in standard library.) - rust-lang#122740 (use more accurate terminology) - rust-lang#122764 (coverage: Remove incorrect assertions from counter allocation) - rust-lang#122765 (Add `usize::MAX` arg tests for Vec) - rust-lang#122776 (Rename `hir::Let` into `hir::LetExpr`) r? `@ghost` `@rustbot` modify labels: rollup

GuillaumeGomez · 2024-03-20T18:47:14Z

I think this is the PR which failed in #122778.

m-ou-se · 2024-03-21T07:09:11Z

@bors r-

joboet · 2024-03-21T21:12:23Z

The failing UI tests only check internal perma-unstable implementations, so I think they are fine to remove.

@rustbot ready

bors · 2024-04-06T17:49:18Z

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

joboet · 2024-05-23T10:44:16Z

r? libs

m-ou-se · 2024-05-23T11:25:16Z

@bors r+

bors · 2024-05-23T11:25:19Z

📌 Commit cef00ea has been approved by m-ou-se

It is now in the queue for this repository.

bors · 2024-05-23T17:39:23Z

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

joboet · 2024-05-24T09:47:59Z

Rebased to resolve the conflict with the native TLS rewrite. I've also added a second commit that makes the #[allow(dead_code)] more local and removed the unnecessary imports that are now detected.

m-ou-se · 2024-05-24T15:07:53Z

@bors r+

bors · 2024-05-24T15:07:55Z

📌 Commit 0e7e75e has been approved by m-ou-se

It is now in the queue for this repository.

bors · 2024-05-24T15:34:09Z

⌛ Testing commit 0e7e75e with merge 9e297bf...

bors · 2024-05-24T17:39:40Z

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing 9e297bf to master...

rust-timer · 2024-05-24T18:54:55Z

Finished benchmarking commit (9e297bf): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.6%	[0.4%, 1.2%]	8
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -2.2%, secondary -5.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)	-2.2%	[-2.2%, -2.2%]	1
Improvements ✅ (secondary)	-5.1%	[-5.1%, -5.1%]	1
All ❌✅ (primary)	-2.2%	[-2.2%, -2.2%]	1

Cycles

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

Binary size

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

Bootstrap: 673.59s -> 674.001s (0.06%)
Artifact size: 315.74 MiB -> 315.67 MiB (-0.02%)

rustbot assigned cuviper Mar 14, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 14, 2024

joboet force-pushed the simplify_key_tls branch from 3150859 to 5fd4a84 Compare March 18, 2024 18:15

rustbot assigned m-ou-se and unassigned cuviper Mar 19, 2024

m-ou-se reviewed Mar 20, 2024

View reviewed changes

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 Mar 20, 2024

m-ou-se added the A-thread-locals Area: Thread local storage (TLS) label Mar 20, 2024

m-ou-se mentioned this pull request Mar 20, 2024

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

Open

23 tasks

GuillaumeGomez mentioned this pull request Mar 20, 2024

Rollup of 8 pull requests #122778

Closed

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 21, 2024

rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Mar 21, 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 Mar 21, 2024

joboet force-pushed the simplify_key_tls branch from 5c40e2a to cef00ea Compare April 8, 2024 10:24

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 May 23, 2024

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 23, 2024

std: simplify key-based thread locals

5f0531d

joboet force-pushed the simplify_key_tls branch from cef00ea to 90ef597 Compare May 24, 2024 09:46

joboet 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 May 24, 2024

joboet assigned m-ou-se and unassigned workingjubilee May 24, 2024

This comment has been minimized.

Sign in to view

joboet force-pushed the simplify_key_tls branch from 90ef597 to 5a7e50e Compare May 24, 2024 10:05

This comment has been minimized.

Sign in to view

std: clean up the TLS implementation

0e7e75e

joboet force-pushed the simplify_key_tls branch from 5a7e50e to 0e7e75e Compare May 24, 2024 10:28

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 May 24, 2024

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

bors merged commit 9e297bf into rust-lang:master May 24, 2024
7 checks passed

rustbot added this to the 1.80.0 milestone May 24, 2024

joboet deleted the simplify_key_tls branch May 24, 2024 18:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify key-based thread locals #122494

Simplify key-based thread locals #122494

joboet commented Mar 14, 2024 •

edited

rustbot commented Mar 14, 2024

cuviper commented Mar 19, 2024

m-ou-se Mar 20, 2024

m-ou-se commented Mar 20, 2024

bors commented Mar 20, 2024

GuillaumeGomez commented Mar 20, 2024

m-ou-se commented Mar 21, 2024

joboet commented Mar 21, 2024

bors commented Apr 6, 2024

joboet commented May 23, 2024

m-ou-se commented May 23, 2024

bors commented May 23, 2024

bors commented May 23, 2024

joboet commented May 24, 2024

This comment has been minimized.

This comment has been minimized.

m-ou-se commented May 24, 2024

bors commented May 24, 2024

bors commented May 24, 2024

bors commented May 24, 2024

rust-timer commented May 24, 2024

Simplify key-based thread locals #122494

Simplify key-based thread locals #122494

Conversation

joboet commented Mar 14, 2024 • edited

rustbot commented Mar 14, 2024

cuviper commented Mar 19, 2024

m-ou-se Mar 20, 2024

Choose a reason for hiding this comment

m-ou-se commented Mar 20, 2024

bors commented Mar 20, 2024

GuillaumeGomez commented Mar 20, 2024

m-ou-se commented Mar 21, 2024

joboet commented Mar 21, 2024

bors commented Apr 6, 2024

joboet commented May 23, 2024

m-ou-se commented May 23, 2024

bors commented May 23, 2024

bors commented May 23, 2024

joboet commented May 24, 2024

This comment has been minimized.

This comment has been minimized.

m-ou-se commented May 24, 2024

bors commented May 24, 2024

bors commented May 24, 2024

bors commented May 24, 2024

rust-timer commented May 24, 2024

Overall result: ❌ regressions - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

joboet commented Mar 14, 2024 •

edited