Do not move thread-locals before dropping #141685

orlp · 2025-05-28T12:44:47Z

Fixes #140816. I also (potentially) improved the speed of get_or_init a bit by having an explicit hot/cold path.

We still move the value before dropping in the event of a recursive initialization (leading to double-initialization with one value being silently dropped). This is the old behavior, but changing this to panic instead would involve changing tests and also the other OS-specific thread_local/os.rs implementation, which is more than I'd like in this PR.

rustbot · 2025-05-28T12:44:52Z

r? @jhpratt

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

Noratrieb · 2025-05-28T17:51:18Z

can you add a test?

orlp · 2025-05-29T01:53:45Z

@Noratrieb Done.

Kobzol · 2025-05-29T06:16:59Z

@bors try @rust-timer queue

Do not move thread-locals before dropping Fixes #140816. I also (potentially) improved the speed of `get_or_init` a bit by having an explicit hot/cold path. We still move the value before dropping in the event of a recursive initialization (leading to double-initialization with one value being silently dropped). This is the old behavior, but changing this to panic instead would involve changing tests and also the other OS-specific `thread_local/os.rs` implementation, which is more than I'd like in this PR.

bors · 2025-05-29T07:27:25Z

⌛ Trying commit aff29df with merge 71827ba...

bors · 2025-05-29T09:43:42Z

☀️ Try build successful - checks-actions
Build commit: 71827ba (71827ba6edcfe824077acb1cbfd0f3167800826d)

rust-timer · 2025-05-29T12:31:10Z

Finished benchmarking commit (71827ba): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.7%	[-2.0%, -0.2%]	25
Improvements ✅ (secondary)	-1.1%	[-1.6%, -0.2%]	7
All ❌✅ (primary)	-0.7%	[-2.0%, -0.2%]	25

Max RSS (memory usage)

Results (primary 4.3%)

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)	7.2%	[4.7%, 9.7%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.5%	[-1.5%, -1.5%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	4.3%	[-1.5%, 9.7%]	3

Cycles

Results (primary -1.4%, secondary -14.6%)

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)	-1.4%	[-1.4%, -1.4%]	1
Improvements ✅ (secondary)	-14.6%	[-16.3%, -13.5%]	6
All ❌✅ (primary)	-1.4%	[-1.4%, -1.4%]	1

Binary size

Results (primary -0.2%, 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.2%	[-0.5%, -0.1%]	11
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.1%]	3
All ❌✅ (primary)	-0.2%	[-0.5%, -0.1%]	11

Bootstrap: 777.949s -> 778.459s (0.07%)
Artifact size: 368.50 MiB -> 368.44 MiB (-0.02%)

jhpratt · 2025-05-30T03:40:51Z

I don't have the necessary context here; reassigning. Though I will note the performance looks great!

r? libs

joboet

Well, the performance improvements speak for themselves...

When I wrote this code, I was hoping that storing the value in an enum would allow for niche-optimisations. It'd be interesting to see whether doing that helps in the case of variables without destructors – but that shouldn't block this PR.

r=me with the nits addressed.

joboet · 2025-05-30T08:35:26Z

library/std/src/sys/thread_local/native/lazy.rs

+        // access to self.value and may replace it.
+        let mut old_value = unsafe { self.value.get().replace(MaybeUninit::new(v)) };
+        match self.state.replace(State::Alive) {
+            State::Uninitialized => D::register_dtor(self),


Could you reintroduce the comments from the old version? I was confused for a second about why the destructor is only registered in this arm, the comment helped to avoid that.

joboet · 2025-05-30T08:36:35Z

tests/ui/threads-sendsync/tls-dont-move-after-init.rs

@@ -0,0 +1,38 @@
+//@ run-pass
+#![allow(stable_features)]


What's this for?

Nothing, it was left over after copying tls-init-on-init.rs as a template.

orlp · 2025-05-30T10:24:01Z

Well, the performance improvements speak for themselves...

I believe the most important part of that isn't the change in layout, it's separating get_or_init into a fast path (already initialized) with a single well-predicted branch followed by an immediate return and a #[cold] slow path where all the other stuff needed for initialization can be.

joboet · 2025-05-30T16:48:16Z

Thank you!
@bors r+

bors · 2025-05-30T16:48:18Z

📌 Commit b374adc has been approved by joboet

It is now in the queue for this repository.

Do not move thread-locals before dropping

b0f6b69

rustbot assigned jhpratt May 28, 2025

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 May 28, 2025

Improve safety comment, double-drop is not relevant here

f70cf59

This comment has been minimized.

Sign in to view

orlp added 2 commits May 28, 2025 16:56

Do not panic, maintain old behavior

13bce27

Add same unsafe bound on get_or_init_slow

8785f7b

This comment has been minimized.

Sign in to view

orlp changed the title ~~Do not move thread-locals before dropping and panic on recursive initialization~~ Do not move thread-locals before dropping May 28, 2025

When replacing an old value we may not drop it in place

9ffbc62

orlp added 2 commits May 29, 2025 02:47

Add test

22c5e1d

Remove unneeded feature

aff29df

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 29, 2025

orlp mentioned this pull request May 29, 2025

Static thread_local! declarations are moved before Drop is called #140816

Open

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 29, 2025

rustbot assigned joboet and unassigned jhpratt May 30, 2025

joboet approved these changes May 30, 2025

View reviewed changes

Address review comments.

b374adc

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 30, 2025

Do not move thread-locals before dropping #141685

Are you sure you want to change the base?

Do not move thread-locals before dropping #141685

Uh oh!

Conversation

orlp commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented May 28, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

Noratrieb commented May 28, 2025

Uh oh!

orlp commented May 29, 2025

Uh oh!

Kobzol commented May 29, 2025

Uh oh!

This comment has been minimized.

bors commented May 29, 2025

Uh oh!

bors commented May 29, 2025

Uh oh!

This comment has been minimized.

rust-timer commented May 29, 2025

Overall result: ✅ improvements - no action needed

Uh oh!

jhpratt commented May 30, 2025

Uh oh!

joboet 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orlp commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joboet commented May 30, 2025

Uh oh!

bors commented May 30, 2025

Uh oh!

Uh oh!

orlp commented May 28, 2025 •

edited

Loading

orlp commented May 30, 2025 •

edited

Loading