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

Optimize obligation gathering. #92017

Closed
wants to merge 1 commit into from
Closed

Optimize obligation gathering. #92017

wants to merge 1 commit into from

Conversation

nnethercote
Copy link
Contributor

By passing in a &mut Vec in various places and appending to it, rather
than building many tiny Vec's and then combining them.

This removes about 20% of the allocations occurred in a check build of
a test program from #87012, making it roughly 3-5% faster.

r? @Mark-Simulacrum

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

@bors try @rust-timer queue

r=me presuming perf looks good on other benchmarks

@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 32d95de905055f1cce7e3b82de039bca7cf78fec with merge 17753d3bec6170d9cf3bf2d3da5ce97d8350dda6...

@bors
Copy link
Contributor

bors commented Dec 17, 2021

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

@rust-timer
Copy link
Collaborator

Queued 17753d3bec6170d9cf3bf2d3da5ce97d8350dda6 with parent 23c2723, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (17753d3bec6170d9cf3bf2d3da5ce97d8350dda6): 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 32.5% on incr-unchanged builds of clap-rs)

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
@nnethercote
Copy link
Contributor Author

Some weirdly large changes that I'll have to look at next week. Note that this change isn't expected to have much effect on existing benchmarks.

@jyn514
Copy link
Member

jyn514 commented Dec 17, 2021

FWIW a bunch of rustdoc PRs were also getting nonsensical results like this so I think it might just be a bug in the perf suite somehow.

@Mark-Simulacrum
Copy link
Member

@bors try @rust-timer queue

rerunning now that we've landed rust-lang/rustc-perf#1123

@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 32d95de905055f1cce7e3b82de039bca7cf78fec with merge 2a37f5b6ca4b0180260dbbb2c5801176ddc67b77...

@bors
Copy link
Contributor

bors commented Dec 18, 2021

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

@rust-timer
Copy link
Collaborator

Queued 2a37f5b6ca4b0180260dbbb2c5801176ddc67b77 with parent 7abab1e, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (2a37f5b6ca4b0180260dbbb2c5801176ddc67b77): comparison url.

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

  • Large improvement in instruction counts (up to -2.4% on full builds of syn)
  • Moderate regression in instruction counts (up to 0.8% on incr-unchanged builds of externs)

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
@nnethercote
Copy link
Contributor Author

I wasn't expecting much change on the rustc-perf benchmarks, because this change is aimed at the example from #87012. But there are more improvements than regressions, which seems fine.

@nnethercote
Copy link
Contributor Author

nnethercote commented Dec 24, 2021 •
edited
Loading

Note: this might not be worthwhile once #92044 lands. We should re-evaluate then.

@Mark-Simulacrum
Copy link
Member

Blocked on #92044, will mark as waiting on author.

@Mark-Simulacrum Mark-Simulacrum 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 Dec 28, 2021
@JohnCSimon JohnCSimon removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 23, 2022
@JohnCSimon JohnCSimon added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 23, 2022
@JohnCSimon JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 12, 2022
@JohnCSimon JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 20, 2022
@nnethercote
Optimize obligation gathering.
528b6a4 
By passing in a `&mut Vec` in various places and appending to it, rather
than building many tiny `Vec`'s and then combining them.

This removes about 20% of the allocations occurred in a `check` build of
a test program from rust-lang#87012, making it roughly 2% faster.
@nnethercote nnethercote force-pushed the opt-obligation-gathering branch from 32d95de to 528b6a4 Compare April 5, 2022 03:38
@nnethercote
Copy link
Contributor Author

@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 Apr 5, 2022
@bors
Copy link
Contributor

bors commented Apr 5, 2022

⌛ Trying commit 528b6a4 with merge 87208aad822ca9fc68eadabdc1fb00df5f862801...

@bors
Copy link
Contributor

bors commented Apr 5, 2022

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

@rust-timer
Copy link
Collaborator

Queued 87208aad822ca9fc68eadabdc1fb00df5f862801 with parent 60e50fc, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (87208aad822ca9fc68eadabdc1fb00df5f862801): comparison url.

Summary:

  • Primary benchmarks: mixed results
  • Secondary benchmarks: mixed results
Regressions 😿
(primary)		 Regressions 😿
(secondary)		 Improvements 🎉
(primary)		 Improvements 🎉
(secondary)		 All 😿 🎉
(primary)
count1 20 8 10 8 30
mean2 0.7% 0.4% -0.8% -0.6% 0.2%
max 2.0% 0.6% -1.7% -1.5% 2.0%

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

Footnotes

  1. number of relevant changes ↩

  2. the arithmetic mean of the percent change ↩

@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 Apr 5, 2022
@nnethercote
Copy link
Contributor Author

I was seeing consistent small wins locally, in a non-PGO build, but the CI results are not good. Time to give up on this one.

@nnethercote nnethercote closed this Apr 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Mark-Simulacrum Mark-Simulacrum

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@nnethercote @Mark-Simulacrum @rust-timer @bors @jyn514 @JohnCSimon @rust-highfive @rustbot