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

Shrink Nonterminal #95715

Merged
merged 1 commit into from
Apr 7, 2022
Merged

Shrink Nonterminal #95715

merged 1 commit into from
Apr 7, 2022

Conversation

nnethercote
Copy link
Contributor

@nnethercote nnethercote commented Apr 6, 2022 •
edited
Loading

Small consistency and performance improvements.

r? @petrochenkov

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 6, 2022
@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 6, 2022
@bors
Copy link
Contributor

bors commented Apr 6, 2022

⌛ Trying commit eeac90f18e23e5b7424093d37a35063ec6515717 with merge 3f4e6015c84c484c82557eb51dcc29431d322233...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@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

@bors
Copy link
Contributor

bors commented Apr 6, 2022

⌛ Trying commit 5a9aeb4a4491b3718709e43d32e862d3db2edd63 with merge c985d860700830a89222e7893aaab3aa247a37ba...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Apr 6, 2022

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

@rust-timer
Copy link
Collaborator

Queued c985d860700830a89222e7893aaab3aa247a37ba with parent bbe9d27, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c985d860700830a89222e7893aaab3aa247a37ba): comparison url.

Summary:

  • Primary benchmarks: changes not relevant. 4 results were found to be statistically significant but the changes were too small to be relevant.
  • Secondary benchmarks: mixed results
Regressions 😿
(primary)		 Regressions 😿
(secondary)		 Improvements 🎉
(primary)		 Improvements 🎉
(secondary)		 All 😿 🎉
(primary)
count1 3 5 1 3 4
mean2 0.2% 0.5% -0.5% -0.3% 0.1%
max 0.3% 0.5% -0.5% -0.4% -0.5%

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 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 Apr 6, 2022
@Kobzol
Copy link
Contributor

Kobzol commented Apr 6, 2022

Some max-RSS wins on deep-vector, not sure if relevant.

@nnethercote
Copy link
Contributor Author

The deep-vector wins are real, I confirmed with DHAT. It produces lots of nonterminals because it contains a vec![] call with thousands of arguments.

I've pushed a new version of the code that is less invasive, it does the minimal thing for NtStmt. Hopefully that will give the memory reductions without as much other perf impact.

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

bors commented Apr 6, 2022

⌛ Trying commit 1937012199dc0d95f70852ff893dc0794b05e0b2 with merge 209128933ec993c4bab7e0994fa2b2bc81f81f39...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Apr 7, 2022

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

@rust-timer
Copy link
Collaborator

Queued 209128933ec993c4bab7e0994fa2b2bc81f81f39 with parent 8f36334, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (209128933ec993c4bab7e0994fa2b2bc81f81f39): comparison url.

Summary:

  • Primary benchmarks: changes not relevant. 2 results were found to be statistically significant but the changes were too small to be relevant.
  • Secondary benchmarks: 🎉 relevant improvements found
Regressions 😿
(primary)		 Regressions 😿
(secondary)		 Improvements 🎉
(primary)		 Improvements 🎉
(secondary)		 All 😿 🎉
(primary)
count1 0 0 2 10 2
mean2 N/A N/A -0.4% -0.8% -0.4%
max N/A N/A -0.4% -1.3% -0.4%

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

Footnotes

  1. number of relevant changes ↩

  2. the arithmetic mean of the percent change ↩

@rustbot rustbot removed S-waiting-on-perf Status: Waiting on a perf run to be completed. perf-regression Performance regression. labels Apr 7, 2022
@nnethercote
Shrink Nonterminal.
d9592c2 
By heap allocating the argument within `NtPath`, `NtVis`, and `NtStmt`.
This slightly reduces cumulative and peak allocation amounts, most
notably on `deep-vector`.
@nnethercote
Copy link
Contributor Author

r? @davidtwco

@nnethercote
Copy link
Contributor Author

Some small but clear wins, definitely worth it for the small changes.

@davidtwco
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Apr 7, 2022

📌 Commit d9592c2 has been approved by davidtwco

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

bors commented Apr 7, 2022

⌛ Testing commit d9592c2 with merge fa72316...

@bors
Copy link
Contributor

bors commented Apr 7, 2022

☀️ Test successful - checks-actions
Approved by: davidtwco
Pushing fa72316 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 7, 2022
@bors bors merged commit fa72316 into rust-lang:master Apr 7, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 7, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (fa72316): comparison url.

Summary:

  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: mixed results
Regressions 😿
(primary)		 Regressions 😿
(secondary)		 Improvements 🎉
(primary)		 Improvements 🎉
(secondary)		 All 😿 🎉
(primary)
count1 0 1 0 4 0
mean2 N/A 0.3% N/A -0.3% N/A
max N/A 0.3% N/A -0.5% N/A

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

Footnotes

  1. number of relevant changes ↩

  2. the arithmetic mean of the percent change ↩

@rustbot rustbot added the perf-regression Performance regression. label Apr 7, 2022
@nnethercote nnethercote deleted the shrink-Nonterminal branch April 11, 2022 22:55
@c410-f3r c410-f3r mentioned this pull request Dec 21, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidtwco davidtwco davidtwco approved these changes

Assignees

@davidtwco davidtwco

Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. 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.62.0
Development

Successfully merging this pull request may close these issues.
7 participants
@nnethercote @rust-timer @bors @rust-log-analyzer @Kobzol @davidtwco @rustbot