Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Minor improvements to Windows TLS dtors #113907

Merged
merged 1 commit into from
Jul 21, 2023
Merged

Minor improvements to Windows TLS dtors #113907

merged 1 commit into from
Jul 21, 2023

Conversation

ChrisDenton
Copy link
Member

This does a few things:

  • Moves keyless dtors into the same module as the on_tls_callback function because of dylib mess. We keep the inline(never) hints as a precaution (see also the issue they link to).
  • Introduces the HAS_DTORS atomic as an optimization hint. This allows removing (most) of the TLS dtor code if no dtors are ever run. Otherwise it's always included because of a #[used].
  • Only run either keyed dtors or keyless dtors but not both. They should be mutually exclusive as keyed dtors are a fallback. I've also added an assert to make sure this is true.

@rustbot
Copy link
Collaborator

rustbot commented Jul 20, 2023

r? @thomcc

(rustbot has picked a reviewer for you, use r? to override)

@rustbot 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 Jul 20, 2023
thomcc
thomcc approved these changes Jul 21, 2023
View reviewed changes
Copy link
Member

@thomcc thomcc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me.

library/std/src/sys/windows/thread_local_key.rs
Ordering::{AcqRel, Acquire, Relaxed, Release},
};
use crate::sys::c;

#[cfg(test)]
mod tests;

/// An optimization hint. The compiler is often smart enough to know if an atomic
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised the compiler can do this! That's an improvement -- it definitely didn't used to be able to.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aye. In my tests it seems to work well for simple local-ish cases. Dunno if it'd hold up in more complex cases tho.

@thomcc
Copy link
Member

thomcc commented Jul 21, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Jul 21, 2023

📌 Commit 40e1164 has been approved by thomcc

It is now in the queue for this repository.

@bors 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 Jul 21, 2023
@matthiaskrgr matthiaskrgr mentioned this pull request Jul 21, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 21, 2023
@bors
Auto merge of rust-lang#113911 - matthiaskrgr:rollup-wk6cr7v, r=matth…
78f97c9 
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#113380 (style-guide: clean up "must"/"should"/"may")
 - rust-lang#113723 (Resurrect: rustc_llvm: Add a -Z `print-codegen-stats` option to expose LLVM statistics.)
 - rust-lang#113780 (Support `--print KIND=PATH` command line syntax)
 - rust-lang#113810 (Make {Rc,Arc}::allocator associated functions)
 - rust-lang#113907 (Minor improvements to Windows TLS dtors)

Failed merges:

 - rust-lang#113392 (style-guide: Some cleanups from the fmt-rfcs repo history)

r? `@ghost`
`@rustbot` modify labels: rollup
@joboet
Copy link
Member

joboet commented Jul 21, 2023

  • Introduces the HAS_DTORS atomic as an optimization hint. This allows removing (most) of the TLS dtor code if no dtors are ever run. Otherwise it's always included because of a #[used].

IIRC, there is always at least one destructor (for threads spawned by std) because of the local Thread handle. In other cases, this also would need LTO to work because the existence of references to the destructor registry functions cannot be known until link time.

@bors bors merged commit 8c6ef1d into rust-lang:master Jul 21, 2023
@rustbot rustbot added this to the 1.73.0 milestone Jul 21, 2023
@ChrisDenton ChrisDenton deleted the tls branch July 21, 2023 10:01
@ChrisDenton
Copy link
Member Author

  • Introduces the HAS_DTORS atomic as an optimization hint. This allows removing (most) of the TLS dtor code if no dtors are ever run. Otherwise it's always included because of a #[used].

IIRC, there is always at least one destructor (for threads spawned by std) because of the local Thread handle. In other cases, this also would need LTO to work because the existence of references to the destructor registry functions cannot be known until link time.

Well it worked in practice for a lib I built. Admittedly I was using build-std.

@joboet
Copy link
Member

joboet commented Jul 21, 2023

  • Introduces the HAS_DTORS atomic as an optimization hint. This allows removing (most) of the TLS dtor code if no dtors are ever run. Otherwise it's always included because of a #[used].

IIRC, there is always at least one destructor (for threads spawned by std) because of the local Thread handle. In other cases, this also would need LTO to work because the existence of references to the destructor registry functions cannot be known until link time.

Well it worked in practice for a lib I built. Admittedly I was using build-std.

Huh. That's cool, nevermind then!

@ChrisDenton
Copy link
Member Author

Well you're absolutely right about an exe. Or anything else that touches ThreadInfo. I did briefly look at making it so ThreadInfo didn't require dropping but I don't think it's possible atm. At the very least it requires a recent version of Windows to be able to get the OS to store the thread name itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomcc thomcc thomcc approved these changes

Assignees

@thomcc thomcc

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.73.0
Development

Successfully merging this pull request may close these issues.
5 participants
@ChrisDenton @rustbot @thomcc @bors @joboet