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

"const" initialized thread locals in rustc #84833

Merged
merged 1 commit into from
May 4, 2021

Conversation

Mark-Simulacrum
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum commented May 2, 2021

This appears to give a slight speedup on many of our benchmarks.

Let's see if this gives us any speedup - some of the TLS state modified in this
commit *is* pretty heavily accessed, so we can hope!
@rust-highfive
Copy link
Collaborator

r? @varkor

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 2, 2021
@Mark-Simulacrum
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 2, 2021
@bors
Copy link
Contributor

bors commented May 2, 2021

⌛ Trying commit 5065144 with merge d001198a19b8608565f98c76c5700d69176acd3e...

@bors
Copy link
Contributor

bors commented May 2, 2021

☀️ Try build successful - checks-actions
Build commit: d001198a19b8608565f98c76c5700d69176acd3e (d001198a19b8608565f98c76c5700d69176acd3e)

@rust-timer
Copy link
Collaborator

Queued d001198a19b8608565f98c76c5700d69176acd3e with parent e10cbc3, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (d001198a19b8608565f98c76c5700d69176acd3e): 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 rollup- to bors.

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 label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 3, 2021
@Mark-Simulacrum
Copy link
Member Author

Looks like a pretty solid improvement to me; this generates essentially strictly less code so I'm not too surprised.

@varkor
Copy link
Member

varkor commented May 3, 2021

@bors r+

@bors
Copy link
Contributor

bors commented May 3, 2021

📌 Commit 5065144 has been approved by varkor

@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 May 3, 2021
@alexcrichton
Copy link
Member

This is a comparison of the codegen FWIW -- https://godbolt.org/z/5MEjMMocE

Also in general the major cost of TLS with Rust is when it comes to cross-crate usage. We've never been great at this, largely due to a missing feature in the compiler (inlining more directives). If rustc winds up using cross-crate TLS (e.g. via monomorphization and such) that's the biggest cost to overcome.

@Mark-Simulacrum
Copy link
Member Author

FWIW for rustc itself I suspect our codegen is a bit better as we pass -Ztls-model=initial-exec - https://godbolt.org/z/deq5jMhsj - still an improvement for sure!

@bors
Copy link
Contributor

bors commented May 3, 2021

⌛ Testing commit 5065144 with merge 11959306b3008b2476b40157ed49c6fd7ee579c8...

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-msvc-cargo failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
fatal: Could not parse object 'caac107ae8145ef2fd20365e2b8fadaf09c2eb3b'.
From https://github.com/servo/servo
 * branch                master     -> FETCH_HEAD
fatal: Could not parse object 'caac107ae8145ef2fd20365e2b8fadaf09c2eb3b'.
error: RPC failed; curl 56 OpenSSL SSL_read: Connection was reset, errno 10054
error: 3981 bytes of body are still expected
fetch-pack: unexpected disconnect while reading sideband packet
fatal: early EOF
fatal: fetch-pack: invalid index-pack output
thread 'main' panicked at 'assertion failed: status.success()', src\tools\cargotest\main.rs:123:13


command did not execute successfully: "D:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage0-tools-bin\\cargotest.exe" "D:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage2-tools-bin\\cargo.exe" "D:\\a\\rust\\rust\\build\\ct"
expected success, got: exit code: 101

@bors
Copy link
Contributor

bors commented May 3, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 3, 2021
@Mark-Simulacrum
Copy link
Member Author

@bors retry network error

@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 May 3, 2021
@bors
Copy link
Contributor

bors commented May 4, 2021

⌛ Testing commit 5065144 with merge 0309953...

@Aaron1011
Copy link
Member

As a follow-up, it might be useful to look at adding a const-thread-local feature to the scoped-tls crate for use in the compiler. This would allow us to const-initialize things like SESSION_GLOBALS, which are probably very hot.

@bors
Copy link
Contributor

bors commented May 4, 2021

☀️ Test successful - checks-actions
Approved by: varkor
Pushing 0309953 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 4, 2021
@bors bors merged commit 0309953 into rust-lang:master May 4, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants