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

Remove CrateNum::ReservedForIncrCompCache #85829

Merged
merged 2 commits into from
Jun 1, 2021
Merged

Remove CrateNum::ReservedForIncrCompCache #85829

merged 2 commits into from
Jun 1, 2021

Conversation

bjorn3
Copy link
Member

@bjorn3 bjorn3 commented May 30, 2021 •
edited
Loading

It's only use is easily replaceable with Option<CrateNum>.

@bjorn3 bjorn3 added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 30, 2021
@rust-highfive
Copy link
Collaborator

r? @jackh726

(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 30, 2021
@bjorn3
Copy link
Member Author

bjorn3 commented May 30, 2021

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

bors commented May 30, 2021

⌛ Trying commit fdfdd08ec59ae3fb47418f2310df75f309cc79df with merge 27edab0bf18b4e2d6617cc3ef49435e245632939...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@cjgillot
Copy link
Contributor

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

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

@bors
Copy link
Contributor

bors commented May 30, 2021

⌛ Trying commit 1ef9885 with merge b45d930d4f71d9f1f424a759cf6e02d3c790eb7c...

@bors
Copy link
Contributor

bors commented May 30, 2021

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

@rust-timer
Copy link
Collaborator

Queued b45d930d4f71d9f1f424a759cf6e02d3c790eb7c with parent 2023cc3, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (b45d930d4f71d9f1f424a759cf6e02d3c790eb7c): 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 30, 2021
@bjorn3
Copy link
Member Author

bjorn3 commented May 30, 2021

Wow! Up to 11.3% improvements for such a simple change! Bootstrap improved by 1.0%. No regressions at all. ❤️

@jackh726
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented May 31, 2021

📌 Commit 1ef9885 has been approved by jackh726

@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 31, 2021
@Aaron1011
Copy link
Member

The perf impact is pretty stunning. I think we should try look into exactly why this is such a large improvement - I suspect that there could be similar missed opportunities within rustc.

@klensy
Copy link
Contributor

klensy commented May 31, 2021

No data for rustc_parse_format in perf run. Other question: why that big change?

@bjorn3
Copy link
Member Author

bjorn3 commented May 31, 2021

I would expect that part of the perf change is from the Hash impl now only needing to hash an u32 instead of a tag and maybe an u32. Another part could be that there are no more panics in the encodable impl. Some could be caused by the from_* and as_* methods previously not being #[inline]. Another part of the perf improvement could be the rest of the derives optimizing better.

No data for rustc_parse_format in perf run.

Huh, strange.

@Mark-Simulacrum
Copy link
Member

rustc_parse_format has a runtime of ~1 second, and we don't currently record measurements less than 1 second.

@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 Jun 1, 2021
@jackh726
Copy link
Member

jackh726 commented Jun 1, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Jun 1, 2021

📌 Commit 5ab25ab has been approved by jackh726

@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 Jun 1, 2021
@bors
Copy link
Contributor

bors commented Jun 1, 2021

⌛ Testing commit 5ab25ab with merge f80975701db776f6a437e4b1a144cba8168ca6b4...

@bors
Copy link
Contributor

bors commented Jun 1, 2021

💥 Test timed out

@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 Jun 1, 2021
@bjorn3
Copy link
Member Author

bjorn3 commented Jun 1, 2021

x86_64-apple has been running for over 4 hours. All other jobs passed. Likely spurious.

@bors retry

@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 Jun 1, 2021
@bors
Copy link
Contributor

bors commented Jun 1, 2021

⌛ Testing commit 5ab25ab with merge 625d5a6...

@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@bors
Copy link
Contributor

bors commented Jun 1, 2021

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing 625d5a6 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 1, 2021
@bors bors merged commit 625d5a6 into rust-lang:master Jun 1, 2021
@rustbot rustbot added this to the 1.54.0 milestone Jun 1, 2021
This was referenced Jun 1, 2021
Merged
Merged
@bjorn3 bjorn3 deleted the simplify_crate_num branch June 2, 2021 08:03
@bjorn3
Copy link
Member Author

bjorn3 commented Jun 2, 2021

Post merge perf results: https://perf.rust-lang.org/compare.html?start=7f9ab0300cd66f6f616e03ea90b2d71af474bf28&end=625d5a693e4697bcafdd34fd1a38c281acabb8e9&stat=instructions%3Au

Still a huge win!

@Mark-Simulacrum
Copy link
Member

Do we have some idea for why this was such a large win? It seems like it may be worth investigating as there's likely some more general improvements to be made - the enum previously defined does not seem like it should add overheads, ideally. Maybe we're hashing enums that are size-optimized by unpacking and hashing the discriminant first, even though that's not necessary in practice in at least some cases (e.g., this one, where the hash of the raw bytes would be sort of fine?).

@bjorn3
Copy link
Member Author

bjorn3 commented Jun 8, 2021

This is my best guess: #85829 (comment)

Maybe we're hashing enums that are size-optimized by unpacking and hashing the discriminant first

Yeah, I am pretty sure this is the case.

JohnTitor added a commit to JohnTitor/rust-semverver that referenced this pull request Jun 14, 2021
@JohnTitor
Rustup to rust-lang/rust#85829
ed27151
JohnTitor added a commit to JohnTitor/rust-semverver that referenced this pull request Jun 14, 2021
@JohnTitor
Rustup to rust-lang/rust#85829
f84cb3c
@JohnTitor JohnTitor mentioned this pull request Jun 19, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jackh726 jackh726

Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. 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-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.54.0
Development

Successfully merging this pull request may close these issues.