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

Implement normalize_erasing_regions queries in terms of 'try' version #91672

Merged
merged 2 commits into from
Dec 17, 2021
Merged

Implement normalize_erasing_regions queries in terms of 'try' version #91672

merged 2 commits into from
Dec 17, 2021

Conversation

b-naber
Copy link
Contributor

@b-naber b-naber commented Dec 8, 2021

Attempt to lessen performance regression caused by #91255

r? @jackh726

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 8, 2021
b-naber
b-naber commented Dec 8, 2021
View reviewed changes
src/test/ui/associated-consts/issue-24949-assoc-const-static-recursion-trait-default.stderr Outdated
@@ -36,6 +37,7 @@ note: ...which requires caching mir of `FooDefault::BAR` for CTFE...
LL | const BAR: u32 = DEFAULT_REF_BAR;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
= note: ...which requires normalizing `DEFAULT_REF_BAR`...
= note: ...which requires normalizing `DEFAULT_REF_BAR`...
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What can we do about this duplication? We can't just remove the query description of normalize_erasing... queries.

b-naber
b-naber commented Dec 8, 2021
View reviewed changes
compiler/rustc_traits/src/normalize_erasing_regions.rs Outdated
normalize_after_erasing_regions(tcx, goal)

let (param_env, goal) = goal.into_parts();
tcx.try_normalize_erasing_regions(param_env, goal).unwrap_or_else(|_| bug!(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't sure whether it's better to use try_normalize_generic_arg_after_erasing_regions here directly or implement this as here (and go through the visitor). The latter would lead to more caching, which might be beneficial when calling normalize_erasing_regions in later stages of compilation, or we might just cache stuff we don't need and also have the cost of traversing the visitor on each call, not sure.

@bors
Copy link
Contributor

bors commented Dec 8, 2021

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

@b-naber b-naber force-pushed the merge-normalize-erasing-regions-queries branch from 28bb188 to 59a33c2 Compare December 9, 2021 08:35
@the8472
Copy link
Member

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

bors commented Dec 9, 2021

⌛ Trying commit 59a33c262b0ff37963f1f55b2008f3f4c716c601 with merge a16ff5c167cd59dbef8e0d6e84cf32aeb4a97a51...

@bors
Copy link
Contributor

bors commented Dec 9, 2021

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

@rust-timer
Copy link
Collaborator

Queued a16ff5c167cd59dbef8e0d6e84cf32aeb4a97a51 with parent 600820d, future comparison URL.

@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 9, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (a16ff5c167cd59dbef8e0d6e84cf32aeb4a97a51): comparison url.

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

  • Small improvement in instruction counts (up to -0.7% on full builds of deeply-nested-closures)
  • Small regression in instruction counts (up to 0.7% on incr-unchanged builds of keccak)

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 9, 2021
jackh726
jackh726 requested changes Dec 9, 2021
View reviewed changes
Copy link
Member

@jackh726 jackh726 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought: Do we even need the normalize_generic_arg_after_erasing_regions and normalize_mir_const_after_erasing_regions queries? Seems like we can just make them either inherent methods on TyCtxt<'tcx> or, better, just call try_* with a bug!() in the one they're called each...

@b-naber
Copy link
Contributor Author

b-naber commented Dec 10, 2021

@jackh726 Can you request another perf-run, please?

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

bors commented Dec 10, 2021

⌛ Trying commit 009e83488716f90c1ec7d9d8a635d2e6bc2e94c9 with merge 763e1db0840ce350629924afc881ea361b2192a2...

@bors
Copy link
Contributor

bors commented Dec 10, 2021

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

@rust-timer
Copy link
Collaborator

Queued 763e1db0840ce350629924afc881ea361b2192a2 with parent 0b42dea, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (763e1db0840ce350629924afc881ea361b2192a2): 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 S-waiting-on-perf Status: Waiting on a perf run to be completed. perf-regression Performance regression. labels Dec 10, 2021
@jackh726
Copy link
Member

Mild perf win but simpler API regardless.

@bors r+

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 13, 2021
@b-naber
Copy link
Contributor Author

b-naber commented Dec 13, 2021

@jackh726 Small problem here, I deleted the branch on my machine once it was r+'ed. I'm unable to recover the branch from my fork on github. Are there other options besides opening a new PR?

@rustbot rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Dec 13, 2021
@b-naber
Copy link
Contributor Author

b-naber commented Dec 13, 2021

nvm, fortunately you can also resolve conflicts directly in github.

@the8472
Copy link
Member

the8472 commented Dec 13, 2021

Github says the branch still exists on your remote. You should be able to fetch it it and then create a branch from the remote ref.

@b-naber
Copy link
Contributor Author

b-naber commented Dec 13, 2021

@the8472 I cloned the remote repo, but it only had a single master branch for some reason. What do you mean by fetching the remote branch?

@b-naber
Copy link
Contributor Author

b-naber commented Dec 13, 2021

Ok nvm, thats a git command I didnt know about.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@the8472
Copy link
Member

the8472 commented Dec 13, 2021

Merge branch 'master' into merge-normalize-erasing-regions-queries

That breaks the no-merge policy, you have to rebase onto current master instead.

@b-naber b-naber force-pushed the merge-normalize-erasing-regions-queries branch from f84942c to 399ab40 Compare December 13, 2021 22:09
@b-naber
Copy link
Contributor Author

b-naber commented Dec 13, 2021

Merge branch 'master' into merge-normalize-erasing-regions-queries

That breaks the no-merge policy, you have to rebase onto current master instead.

Thanks.

@b-naber
Copy link
Contributor Author

b-naber commented Dec 16, 2021

@jackh726 Can you r+ this again, please?

@jackh726
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Dec 16, 2021

📌 Commit 399ab40 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 16, 2021
@bors
Copy link
Contributor

bors commented Dec 17, 2021

⌛ Testing commit 399ab40 with merge 9b45f04...

@bors
Copy link
Contributor

bors commented Dec 17, 2021

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing 9b45f04 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 17, 2021
@bors bors merged commit 9b45f04 into rust-lang:master Dec 17, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 17, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (9b45f04): comparison url.

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

  • Very large improvement in instruction counts (up to -11.6% on incr-unchanged builds of tuple-stress)
  • Very large regression in instruction counts (up to 7.4% on incr-unchanged builds of ctfe-stress-4)

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

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

@rustbot rustbot added the perf-regression Performance regression. label Dec 17, 2021
@jackh726
Copy link
Member

jackh726 commented Dec 17, 2021 •
edited
Loading

That's... weird

@jackh726 jackh726 mentioned this pull request Dec 17, 2021
Merged
@rylev
Copy link
Member

rylev commented Dec 21, 2021

This seems likely to be noise as a result of the issue tracked in rust-lang/rustc-perf#1126 -- marking as triaged, not something we should address directly.

@rustbot label +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Dec 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackh726 jackh726 jackh726 requested changes

Assignees

@jackh726 jackh726

Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. 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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.59.0
Development

Successfully merging this pull request may close these issues.
10 participants
@b-naber @bors @the8472 @rust-timer @jackh726 @rust-log-analyzer @rylev @apiraino @rust-highfive @rustbot