std: Attempt again to inline thread-local-init across crates #84876
Conversation
|
r? @dtolnay (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
@bors r+ rollup=never -- it may make sense I guess regardless to cfg_attr these inlines to work on e.g. unix or platforms where they're "known good" even if this doesn't end up landing. |
|
📌 Commit 75311b41b4e822ca715155861cb7afa8ecb6f618 has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 75311b41b4e822ca715155861cb7afa8ecb6f618 with merge d83e8b8afad8a3c3b98038e5664add57726fc2ec... |
This comment has been minimized.
This comment has been minimized.
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
Hm ok that got suspiciously far compiling rustc in stage1. It's faulting on MSVC, though, like before. I would personally like to spend some time investigating this. I still feel the same that this shouldn't be landed with |
|
Sounds good to me. It may be worth switching to cfg_attr and running a perf build, just so we can get some sense of how the performance improvement looks - if it ends up being a regression, for example, we might need some alternative approach. |
|
Nah that's a good idea! I was surprised rereading the results of #59720 of how things regressed slightly. If that's still the case then I would imagine that, at least for the compiler itself, this would need to be coupled with #84833 to show better results. In any case, mind kicking off the perf build? |
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit f6afc646da3992bb7182b15b5d41d93ae1a1bd62 with merge 628cb1d71c8850f01c33ec3ec594fde62784a7e2... |
|
Happy to run it with a cherry pick of the const init PR as well |
|
☀️ Try build successful - checks-actions |
|
Queued 628cb1d71c8850f01c33ec3ec594fde62784a7e2 with parent 716394d, future comparison URL. |
|
Finished benchmarking try commit (628cb1d71c8850f01c33ec3ec594fde62784a7e2): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
rustbot
removed
the
S-waiting-on-perf
|
Looks like a net win. |
|
I'm making some progress investigating this crash on Windows. It'll take some more time. I have no idea how thread locals work on Windows so I'm throwing a lot of stuff at the wall right now. |
alexcrichton
force-pushed
the
inline-thread-locals-cross-crate
branch
from
f6afc64
to
8bd9035
Compare
|
⌛ Testing commit 0e3f77947de5a81d6e787dee2658721e69d63875 with merge 6dd6a850b05bb650bbcc0539a3551edae014a9a0... |
This comment has been minimized.
This comment has been minimized.
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
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
alexcrichton
force-pushed
the
inline-thread-locals-cross-crate
branch
from
0e3f779
to
641d3b0
Compare
|
@bors: r=Mark-Simulacrum |
|
📌 Commit 641d3b0 has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 641d3b0 with merge 583eb5d0735d15042e52530947033ff8b39cf99d... |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
@bors: retry Hm looks like things were just cancelled, I couldn't find any actual failures, so gonna retry. |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
| @@ -0,0 +1,50 @@ | |||
| // compile-flags: -O | |||
| // aux-build:thread_local_aux.rs | |||
| // ignore-windows FIXME(#84933) | |||
There was a problem hiding this comment.
This should have been ignore-msvc because now we enable it for windows-gnu without testing.
Issue #25088 has been part of
thread_local!for quite some time now.Historical attempts have been made to add
#[inline]to__getitin #43931, #50252, and #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 thisis the only remaining bug which is why you might want to use
#[thread_local]overthread_local!. As a result I figured it wastime to resubmit this and see how it fares on CI and if I can help
debugging any issues that crop up.
Closes #25088