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

Use const initializer for LOCAL_PANIC_COUNT #92461

Merged
merged 1 commit into from
May 23, 2022
Merged

Use const initializer for LOCAL_PANIC_COUNT #92461

merged 1 commit into from
May 23, 2022

Conversation

bjorn3
Copy link
Member

@bjorn3 bjorn3 commented Dec 31, 2021

This reduces the size of the __getit function for LOCAL_PANIC_COUNT and should speed up accesses of LOCAL_PANIC_COUNT a bit.

@bjorn3 bjorn3 added the A-thread-locals Area: Thread local storage (TLS) label Dec 31, 2021
@rust-highfive
Copy link
Collaborator

r? @joshtriplett

(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 Dec 31, 2021
@Mark-Simulacrum
Copy link
Member

@bors r+ rollup=never (plausible perf effect, but should be positive or neutral)

@bors
Copy link
Contributor

bors commented Dec 31, 2021

📌 Commit 6e4c3b1 has been approved by Mark-Simulacrum

@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 Dec 31, 2021
@bors
Copy link
Contributor

bors commented Jan 1, 2022

⌛ Testing commit 6e4c3b1 with merge 64aaec50341510cde64227a2f67a73d8be26a35e...

@bors
Copy link
Contributor

bors commented Jan 1, 2022

💔 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 Jan 1, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bjorn3
Copy link
Member Author

bjorn3 commented Jan 1, 2022 

curl: (6) Could not resolve host: ci-mirrors.rust-lang.org

@bors retry spurious 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 Jan 1, 2022
@bors
Copy link
Contributor

bors commented Jan 1, 2022

⌛ Testing commit 6e4c3b1 with merge 4484eacdad2cfbdfbfa56fbcb413dac1723889ff...

@bors
Copy link
Contributor

bors commented Jan 1, 2022

💔 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 Jan 1, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bjorn3
Copy link
Member Author

bjorn3 commented Jan 1, 2022

Why does the unsafe_op_in_unsafe_fn lint trigger inside thread_local!{} here?

@bjorn3
Copy link
Member Author

bjorn3 commented Jan 10, 2022

Seems that const thread_local! was missing a couple of unsafe {} blocks.

@bjorn3
Copy link
Member Author

bjorn3 commented Jan 10, 2022

@rustbot ready

@bjorn3 bjorn3 mentioned this pull request Jan 10, 2022
Closed
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 23, 2022
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 20, 2022
@bors
Copy link
Contributor

bors commented Apr 2, 2022

☔ The latest upstream changes (presumably #95581) made this pull request unmergeable. Please resolve the merge conflicts.

@bjorn3 bjorn3 force-pushed the const_tls_local_panic_count branch from 8a0c633 to bfd45e0 Compare April 2, 2022 18:25
@bjorn3
Copy link
Member Author

bjorn3 commented Apr 2, 2022

@rust-lang/infra For some reason the x86_64-gnu-llvm-12, job got canceled. I tried rerunning it but then @github-actions canceled it again. Any idea what could be going on?

image

@bors
Copy link
Contributor

bors commented Apr 13, 2022

☔ The latest upstream changes (presumably #95727) made this pull request unmergeable. Please resolve the merge conflicts.

@bjorn3
Use const initializer for LOCAL_PANIC_COUNT
cbc0a15 
This reduces the size of the __getit function for LOCAL_PANIC_COUNT and should
speed up accesses of LOCAL_PANIC_COUNT a bit.
@bjorn3 bjorn3 force-pushed the const_tls_local_panic_count branch from bfd45e0 to cbc0a15 Compare April 23, 2022 10:10
@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Apr 23, 2022
@bjorn3
Copy link
Member Author

bjorn3 commented Apr 23, 2022

Rebased

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 22, 2022
@JohnTitor
Copy link
Member

r? @Mark-Simulacrum as you reviewed previously

@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented May 23, 2022

📌 Commit cbc0a15 has been approved by Mark-Simulacrum

@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 23, 2022
@bors
Copy link
Contributor

bors commented May 23, 2022

⌛ Testing commit cbc0a15 with merge ef9b498...

@bors
Copy link
Contributor

bors commented May 23, 2022

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing ef9b498 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 23, 2022
@bors bors merged commit ef9b498 into master May 23, 2022
@bors bors deleted the const_tls_local_panic_count branch May 23, 2022 15:45
@rustbot rustbot added this to the 1.63.0 milestone May 23, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (ef9b498): comparison url.

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results
  • Primary benchmarks: 😿 relevant regression found
  • Secondary benchmarks: mixed results
Regressions 😿
(primary)		 Regressions 😿
(secondary)		 Improvements 🎉
(primary)		 Improvements 🎉
(secondary)		 All 😿 🎉
(primary)
count1 1 2 0 1 1
mean2 2.0% 1.3% N/A -1.5% 2.0%
max 2.0% 1.5% N/A -1.5% 2.0%

Cycles

Results
  • Primary benchmarks: 😿 relevant regression found
  • Secondary benchmarks: 😿 relevant regressions found
Regressions 😿
(primary)		 Regressions 😿
(secondary)		 Improvements 🎉
(primary)		 Improvements 🎉
(secondary)		 All 😿 🎉
(primary)
count1 1 3 0 0 1
mean2 1.9% 2.5% N/A N/A 1.9%
max 1.9% 2.8% N/A N/A 1.9%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. number of relevant changes ↩ ↩2

  2. the arithmetic mean of the percent change ↩ ↩2

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

@goffrie goffrie goffrie left review comments

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
A-thread-locals Area: Thread local storage (TLS) 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. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.63.0
Development

Successfully merging this pull request may close these issues.