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.

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

Inline __getit #59720

Closed
wants to merge 1 commit into from
Closed

Inline __getit #59720

wants to merge 1 commit into from

Conversation

mati865
Copy link
Contributor

@mati865 mati865 commented Apr 5, 2019

Trying to land #50252 (originally part of #43931) for platforms where it doesn't fail.
cc #25088

r? @eddyb

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 5, 2019
@eddyb
Copy link
Member

eddyb commented Apr 7, 2019

r? @alexcrichton

@rust-highfive rust-highfive assigned alexcrichton and unassigned eddyb Apr 7, 2019
@alexcrichton
Copy link
Member

@mati865
Copy link
Contributor Author

mati865 commented Apr 9, 2019

I don't know how much windows-gnu depends on GCC but mingw-w64 GCC provides emulated TLS, that could be why only MSVC was broken.

I'll try to run perf to see if it's even worth pushing for. It could be changed from blacklisting msvc to whitelist only platfroms known to work (linux* and bsd*?).

Assuming the perf looks good it should be codegen test, right?

@alexcrichton
Copy link
Member

Indeed!

@mati865
Copy link
Contributor Author

mati865 commented Apr 12, 2019

On Ryzen platform running openSUSE Tumbleweed (Linux 5.0.6, glibc 2.29) this results in slightly red noise:
Screenshot_2019-04-12 rustc performance data

Rust perf seems to be running on Debian Stretch with significantly older packages so it may yield different results but you can go ahead and close this if you want.

@alexcrichton
Copy link
Member

Ok, sounds like we may not be quite ready for this yet!

@eddyb
Copy link
Member

eddyb commented Apr 13, 2019

My guess is that if you want to see improvements, you need to use bare #[thread_local] directly, on supported platforms.
And maybe switch to static linking.

alexcrichton added a commit to alexcrichton/rust that referenced this pull request May 18, 2021
Issue rust-lang#25088 has been part of `thread_local!` for quite some time now.
Historical attempts have been made to add `#[inline]` to `__getit`
in rust-lang#43931, rust-lang#50252, and rust-lang#59720, but these attempts ended up not landing
at the time due to segfaults on Windows.

In the interim though with `const`-initialized thread locals AFAIK this
is the only remaining bug which is why you might want to use
`#[thread_local]` over `thread_local!`. As a result I figured it was
time to resubmit this and see how it fares on CI and if I can help
debugging any issues that crop up.

Closes rust-lang#25088
bors added a commit to rust-lang-ci/rust that referenced this pull request May 19, 2021
…ss-crate, r=Mark-Simulacrum

std: Attempt again to inline thread-local-init across crates

Issue rust-lang#25088 has been part of `thread_local!` for quite some time now.
Historical attempts have been made to add `#[inline]` to `__getit`
in rust-lang#43931, rust-lang#50252, and rust-lang#59720, but these attempts ended up not landing
at the time due to segfaults on Windows.

In the interim though with `const`-initialized thread locals AFAIK this
is the only remaining bug which is why you might want to use
`#[thread_local]` over `thread_local!`. As a result I figured it was
time to resubmit this and see how it fares on CI and if I can help
debugging any issues that crop up.

Closes rust-lang#25088
@mati865 mati865 deleted the getit branch May 20, 2021 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants