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

remove obligation dedup from impl_or_trait_obligations #84944

Merged
merged 1 commit into from
Mar 3, 2022

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented May 5, 2021

Looking at the examples from #38528 they all seem to compile fine even without this and it seems like this might be unnecessary effort

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(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 5, 2021
@lcnr
Copy link
Contributor Author

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

bors commented May 5, 2021

⌛ Trying commit 57a1c9887f7a07660872d1ffad6eaba7fc193359 with merge baa153cea1f4237cf0f42cc231044925ed4616c6...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented May 5, 2021

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

@rust-timer
Copy link
Collaborator

Queued baa153cea1f4237cf0f42cc231044925ed4616c6 with parent 2d11e25, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (baa153cea1f4237cf0f42cc231044925ed4616c6): 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 5, 2021
@lcnr lcnr changed the title remove dedup in impl_or_trait_obligations move obligation dedup from impl_or_trait_obligations to project caching May 5, 2021
@lcnr
Copy link
Contributor Author

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

bors commented May 5, 2021

⌛ Trying commit 8a7ae4b17d1e9b669d2df0dab5a7b51322f4f879 with merge 97f6956b10598cea5032a815cee5224be2243d23...

@rust-log-analyzer

This comment has been minimized.

@lcnr lcnr force-pushed the obligation-dedup branch from 8a7ae4b to fddcc01 Compare May 5, 2021 13:06
@lcnr
Copy link
Contributor Author

lcnr commented May 5, 2021

@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 5, 2021

⌛ Trying commit fddcc01cae17efb417e88d0d356c873f1880edfc with merge cfbeb503faad5f1b68441b15f8f2660bbe95bfa8...

@bors
Copy link
Contributor

bors commented May 5, 2021

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

@rust-timer
Copy link
Collaborator

Queued cfbeb503faad5f1b68441b15f8f2660bbe95bfa8 with parent 2d11e25, future comparison URL.

@jackh726
Copy link
Member

jackh726 commented May 5, 2021

Related: #77325

@lcnr
Copy link
Contributor Author

lcnr commented May 5, 2021

that PR is very related 😅

From what I can tell #77325 looks sound and is more general than this PR, so it's probably easier to just merge that PR

@jackh726
Copy link
Member

jackh726 commented May 5, 2021

Agreed. Let's see if we can poke @nikomatsakis to dedicate some time to look at that PR :P

@Aaron1011
Copy link
Member

Aaron1011 commented Dec 22, 2021

I'm concerned that there might be code relying on this to avoid exponential blowup (similar to the 'projection caching' issue), but none of our benchmarks cover it.

@jackh726
Copy link
Member

We should look at the cases found by the previous crater run. There didn't seem to be perf issues (though I don't know it that was specifically looked at). But there were some regressions (I don't remember if this was checked).

Also, was the original issue that prompted this added as a perf benchmark? Probably should be if it wasn't.

@Aaron1011
Copy link
Member

Also, was the original issue that prompted this added as a perf benchmark? Probably should be if it wasn't.

Not yet - see rust-lang/rustc-perf#1124

@lcnr
Copy link
Contributor Author

lcnr commented Jan 27, 2022

rust-lang/rustc-perf#1124 is now closed, does it still makes sense to merge this PR? I am really not up to date here 😅

@lqd
Copy link
Member

lqd commented Jan 27, 2022

Maybe we could just re-run perf one final time, now that the projection-caching benchmark exists, to see if this PR doesn't affect it ?

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

bors commented Jan 27, 2022

⌛ Trying commit b941485 with merge e5dfa4b135ad18263acae18b00193d608bf71d85...

@bors
Copy link
Contributor

bors commented Jan 27, 2022

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

@rust-timer
Copy link
Collaborator

Queued e5dfa4b135ad18263acae18b00193d608bf71d85 with parent 21b4a9c, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e5dfa4b135ad18263acae18b00193d608bf71d85): comparison url.

Summary: This benchmark run shows 30 relevant improvements 🎉 to instruction counts.

  • Average relevant improvement: -1.5%
  • Largest improvement in instruction counts: -3.9% on full builds of deeply-nested check

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 Jan 27, 2022
@jackh726
Copy link
Member

Solid wins.

I'm good to merge this, but I want to r? @Aaron1011, since they had concerns.

@lcnr lcnr changed the title move obligation dedup from impl_or_trait_obligations to project caching remove obligation dedup from impl_or_trait_obligations Jan 28, 2022
@jackh726
Copy link
Member

jackh726 commented Mar 3, 2022

Discussed in today's compiler meeting. Decided to go ahead and r+ this, given that the original blowup this code was added for has a benchmark.

@bors r+

@bors
Copy link
Contributor

bors commented Mar 3, 2022

📌 Commit b941485 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 Mar 3, 2022
@bors
Copy link
Contributor

bors commented Mar 3, 2022

⌛ Testing commit b941485 with merge 32cbc76...

@bors
Copy link
Contributor

bors commented Mar 3, 2022

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing 32cbc76 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 3, 2022
@bors bors merged commit 32cbc76 into rust-lang:master Mar 3, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 3, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (32cbc76): comparison url.

Summary: This benchmark run shows 34 relevant improvements 🎉 to instruction counts.

  • Arithmetic mean of relevant improvements: -1.4%
  • Largest improvement in instruction counts: -4.0% on full builds of deeply-nested check

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

@rustbot label: -perf-regression

@lcnr lcnr deleted the obligation-dedup branch March 7, 2022 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-trait-system Area: Trait system 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
Development

Successfully merging this pull request may close these issues.