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

Make DefId repr(C), optimize big-endian field order #92012

Merged
merged 1 commit into from
Jan 11, 2022
Merged

Make DefId repr(C), optimize big-endian field order #92012

merged 1 commit into from
Jan 11, 2022

Conversation

llogiq
Copy link
Contributor

@llogiq llogiq commented Dec 16, 2021

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 16, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 16, 2021
@jackh726
Copy link
Member

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

bors commented Dec 17, 2021

⌛ Trying commit 95a3e6adc5e724e93a62915b22380b6137be08db with merge 6de93a5f3c58289159da3d736b0361ca414bd34f...

@bors
Copy link
Contributor

bors commented Dec 17, 2021

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

@rust-timer
Copy link
Collaborator

Queued 6de93a5f3c58289159da3d736b0361ca414bd34f with parent 5531927, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (6de93a5f3c58289159da3d736b0361ca414bd34f): comparison url.

Summary: This change led to very large relevant mixed results 🤷 in compiler performance.

  • Very large improvement in instruction counts (up to -6.8% on incr-unchanged builds of ctfe-stress-4)
  • Large regression in instruction counts (up to 3.5% on incr-unchanged builds of unicode_normalization)

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

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Dec 17, 2021
@llogiq
Copy link
Contributor Author

llogiq commented Dec 17, 2021

That's weird. Even if I did something wrong, it shouldn't change the outcome by that much.

@michaelwoerister
Copy link
Member

Yeah, that is weird. We'd expect the outcome to not change at all...

@jackh726
Copy link
Member

I wonder if something is going wrong with perf: #91672 (comment)

@llogiq
Copy link
Contributor Author

llogiq commented Dec 17, 2021

Yeah, that looks really fishy. Perhaps someone with access to perf can have a look?

@llogiq
Copy link
Contributor Author

llogiq commented Dec 17, 2021

Ah it's likely because of rust-lang/rustc-perf#1123

@Mark-Simulacrum
Copy link
Member

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

bors commented Dec 17, 2021

⌛ Trying commit 95a3e6adc5e724e93a62915b22380b6137be08db with merge 6a6e67427da0929979dc434855e205efe90bd2a3...

@bors
Copy link
Contributor

bors commented Dec 18, 2021

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

@rust-timer
Copy link
Collaborator

Queued 6a6e67427da0929979dc434855e205efe90bd2a3 with parent 7abab1e, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (6a6e67427da0929979dc434855e205efe90bd2a3): comparison url.

Summary: This change led to moderate relevant regressions 😿 in compiler performance.

  • Moderate regression in instruction counts (up to 3.2% on incr-patched: println builds of regression-31157)

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

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 18, 2021
@llogiq
Copy link
Contributor Author

llogiq commented Dec 18, 2021

So there is something wrong. I'll have a look.

@llogiq
Copy link
Contributor Author

llogiq commented Dec 20, 2021 •
edited
Loading

Can we get another perf run? I wonder if removing the repr(C) fixes the regression. Otherwise it must be because of the #[cfg(..)]s. I also wonder if at this point it would make sense to change DefId to a single u64 and make index and krate methods instead, but that change would be far larger. The upside would be that we could freely change the internal representation should we ever desire it.

@michaelwoerister
Copy link
Member

perf.rlo seems to have problems with increased noise at the moment: rust-lang/rustc-perf#1126

Let's wait for that to be fixed and then revisit this.

@rust-timer
Copy link
Collaborator

Queued 586b3d2e1d80845457c6700c7e3deee19006521b with parent ddabe07, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (586b3d2e1d80845457c6700c7e3deee19006521b): comparison url.

Summary: This benchmark run did not return any relevant changes.

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

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. perf-regression Performance regression. labels Jan 4, 2022
@llogiq
Copy link
Contributor Author

llogiq commented Jan 4, 2022

Ok, that's what we were expecting. Now I'll add the #[repr(C)] back.

@Mark-Simulacrum
Copy link
Member

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

bors commented Jan 4, 2022

⌛ Trying commit 7f7b551 with merge a5535ef48e9bb30104929abcdb9f1a6eec3d532f...

@bors
Copy link
Contributor

bors commented Jan 4, 2022

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

@rust-timer
Copy link
Collaborator

Queued a5535ef48e9bb30104929abcdb9f1a6eec3d532f with parent 2b681ac, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (a5535ef48e9bb30104929abcdb9f1a6eec3d532f): comparison url.

Summary: This benchmark run did not return any relevant changes.

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

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 4, 2022
@llogiq
Copy link
Contributor Author

llogiq commented Jan 4, 2022

Cool. This means we haven't lost any perf on any tier-1 systems and may have gained a little on big-endian 64-bit machines, not that there would be a lot of them around.

@michaelwoerister
Copy link
Member

Looks good now!
@bors r+

@bors
Copy link
Contributor

bors commented Jan 10, 2022

📌 Commit 7f7b551 has been approved by michaelwoerister

@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. S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Jan 10, 2022
@bors
Copy link
Contributor

bors commented Jan 11, 2022

⌛ Testing commit 7f7b551 with merge e4b1d58...

@bors bors mentioned this pull request Jan 11, 2022
Merged
@bors
Copy link
Contributor

bors commented Jan 11, 2022

☀️ Test successful - checks-actions
Approved by: michaelwoerister
Pushing e4b1d58 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 11, 2022
@bors bors merged commit e4b1d58 into rust-lang:master Jan 11, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 11, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e4b1d58): comparison url.

Summary: This benchmark run did not return any relevant changes.

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

@rustbot label: -perf-regression

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

@michaelwoerister michaelwoerister

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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.60.0
Development

Successfully merging this pull request may close these issues.
8 participants
@llogiq @jackh726 @rust-timer @bors @michaelwoerister @Mark-Simulacrum @rust-highfive @rustbot