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

Deduplicate obligations in opt_normalize_projection_type #90913

Closed

Conversation

the8472
Copy link
Member

@the8472 the8472 commented Nov 14, 2021

This adds a second deduplication site (in addition to the original one at impl_or_trait_obligations) in opt_normalize_projection_type since that's where the OOMing allocations occured in #74456.

Fixes an OOM and compile time blowup in the reduced test-case on that issue.

Since it only touches one place where obligations are processed it might not fix all of these blowups. A more general fix would require replacing most uses of Vec<Obligation> with some OrderedSet<Obligation>.

@the8472
Copy link
Member Author

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

bors commented Nov 14, 2021

⌛ Trying commit 78b5f2d with merge 223f5e877fe93b5f437c2d703f883797913cd2b7...

@bors
Copy link
Contributor

bors commented Nov 15, 2021

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

@rust-timer
Copy link
Collaborator

Queued 223f5e877fe93b5f437c2d703f883797913cd2b7 with parent ad44239, future comparison URL.

@rust-timer

This comment has been minimized.

@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. labels Nov 15, 2021
@the8472
Copy link
Member Author

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

bors commented Nov 15, 2021

⌛ Trying commit dd05c32256545056356f5547af03eb17e173d889 with merge d1b7285495d8ea2363fd7d7efc2b6989f0029424...

@bors
Copy link
Contributor

bors commented Nov 15, 2021

💔 Test failed - checks-actions

@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-review Status: Awaiting review from the assignee but also interested parties. labels Nov 15, 2021
@the8472 the8472 force-pushed the simplify-obligation-cause-hash branch from dd05c32 to 0330941 Compare November 15, 2021 15:46
@the8472
Copy link
Member Author

the8472 commented Nov 15, 2021

@bors try

@bors
Copy link
Contributor

bors commented Nov 15, 2021

⌛ Trying commit 033094176cdafa8b165a658604ecf2ddff3e53b1 with merge 39d7399cd1e666e48aecc980ee352b3daf8c420b...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Nov 15, 2021

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

@rust-timer
Copy link
Collaborator

Queued 39d7399cd1e666e48aecc980ee352b3daf8c420b with parent eab2d75, future comparison URL.

@rust-timer

This comment has been minimized.

@rustbot rustbot added perf-regression Performance regression. 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. labels Nov 15, 2021
@the8472
Copy link
Member Author

the8472 commented Nov 15, 2021

Well, not as rosy as the first commit, but now it fixes the OOM reported in #74456.

@bors
Copy link
Contributor

bors commented Nov 23, 2021

⌛ Trying commit 307a2b675b94d53a765894b0a169225e8a831772 with merge 92da4f029ba76f78f916fa529c0c6add294fbb04...

@bors
Copy link
Contributor

bors commented Nov 23, 2021

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

@rust-timer
Copy link
Collaborator

Queued 92da4f029ba76f78f916fa529c0c6add294fbb04 with parent 311fa1f, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (92da4f029ba76f78f916fa529c0c6add294fbb04): comparison url.

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

  • Very large improvement in instruction counts (up to -7.2% on full builds of deeply-nested)
  • Moderate regression in instruction counts (up to 0.9% on full builds of wg-grammar)

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 Nov 23, 2021
@the8472 the8472 force-pushed the simplify-obligation-cause-hash branch from 307a2b6 to b6c2cf0 Compare November 23, 2021 22:24
@the8472
Copy link
Member Author

the8472 commented Nov 23, 2021

Previous run looks much better, let's see if we can squeeze a bit more out of it.

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

bors commented Nov 23, 2021

⌛ Trying commit b6c2cf00116d7097d36ae67f593f36e1c7b74903 with merge 2d385d312cf7a0119d96d63907bb119e2b83923c...

@the8472
Copy link
Member Author

the8472 commented Nov 23, 2021

@bors cancel

@the8472 the8472 force-pushed the simplify-obligation-cause-hash branch from b6c2cf0 to 6972c6e Compare November 23, 2021 22:33
@the8472
Copy link
Member Author

the8472 commented Nov 23, 2021

@bors try

@bors
Copy link
Contributor

bors commented Nov 23, 2021

⌛ Trying commit 6972c6e with merge a5fcd75b334a8bb8bc3e870f0e2c1173c48e0712...

@bors
Copy link
Contributor

bors commented Nov 24, 2021

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

@rust-timer
Copy link
Collaborator

Queued a5fcd75b334a8bb8bc3e870f0e2c1173c48e0712 with parent 7b3cd07, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (a5fcd75b334a8bb8bc3e870f0e2c1173c48e0712): comparison url.

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

  • Very large improvement in instruction counts (up to -7.5% on full builds of deeply-nested)
  • Large regression in instruction counts (up to 4.1% on incr-unchanged builds of deep-vector)

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 Nov 24, 2021
@the8472
Copy link
Member Author

the8472 commented Nov 24, 2021

If we ignore deep-vector debug incr-unchanged which probably is incremental verification noise the results are still looking good. I'll go with that.

Sorry for all the noise from the experiments. I'll create a new PR to make it easier to review.

@the8472
Copy link
Member Author

the8472 commented Nov 24, 2021

new pr: #91186

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
perf-regression Performance regression. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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.

7 participants