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

Try to cache region_scope_tree as a query #97383

Merged
merged 3 commits into from
May 28, 2022
Merged

Try to cache region_scope_tree as a query #97383

merged 3 commits into from
May 28, 2022

Conversation

dingxiangfei2009
Copy link
Contributor

This PR will attempt to restore region_scope_tree as a query so that caching works again. It seems that region_scope_tree could be re-computed for nested items after all, which could explain the performance regression introduced by #95563.

cc @Mark-Simulacrum @pnkfelix I will try to trigger a perf run here.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 25, 2022
@rust-highfive
Copy link
Collaborator

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(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 25, 2022
@dingxiangfei2009
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Insufficient permissions to issue commands to rust-timer.

@bors
Copy link
Contributor

bors commented May 25, 2022

@dingxiangfei2009: 🔑 Insufficient privileges: not in try users

@dingxiangfei2009
Copy link
Contributor Author

😢 Okay. May I request for a test run with perf.rust-lang.org? 🙏

@matthiaskrgr
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 May 25, 2022
@bors
Copy link
Contributor

bors commented May 25, 2022

⌛ Trying commit 4c6074f with merge 209f91e1787d0d29a0e566fa93f35d52e60ea84a...

@bors
Copy link
Contributor

bors commented May 25, 2022

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

@rust-timer
Copy link
Collaborator

Queued 209f91e1787d0d29a0e566fa93f35d52e60ea84a with parent 6ac8ada, future comparison URL.

@petrochenkov
Copy link
Contributor

r? rust-lang/compiler

@rust-highfive rust-highfive assigned nagisa and unassigned petrochenkov May 25, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (209f91e1787d0d29a0e566fa93f35d52e60ea84a): comparison url.

Instruction count

  • Primary benchmarks: 🎉 relevant improvements found
  • Secondary benchmarks: mixed results
mean1 max count2
Regressions 😿
(primary)		 0.4% 0.4% 1
Regressions 😿
(secondary)		 0.6% 1.0% 9
Improvements 🎉
(primary)		 -1.7% -5.4% 100
Improvements 🎉
(secondary)		 -2.8% -8.1% 42
All 😿🎉 (primary) -1.7% -5.4% 101

Max RSS (memory usage)

Results
  • Primary benchmarks: 🎉 relevant improvements found
  • Secondary benchmarks: mixed results
mean1 max count2
Regressions 😿
(primary)		 1.4% 2.1% 3
Regressions 😿
(secondary)		 2.5% 3.3% 4
Improvements 🎉
(primary)		 -3.7% -10.6% 42
Improvements 🎉
(secondary)		 -3.9% -7.4% 21
All 😿🎉 (primary) -3.3% -10.6% 45

Cycles

Results
  • Primary benchmarks: 🎉 relevant improvements found
  • Secondary benchmarks: 🎉 relevant improvements found
mean1 max count2
Regressions 😿
(primary)		 N/A N/A 0
Regressions 😿
(secondary)		 2.2% 2.2% 1
Improvements 🎉
(primary)		 -2.4% -3.6% 20
Improvements 🎉
(secondary)		 -3.5% -5.0% 21
All 😿🎉 (primary) -2.4% -3.6% 20

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 may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

Footnotes

  1. the arithmetic mean of the percent change ↩ ↩2 ↩3

  2. number of relevant changes ↩ ↩2 ↩3

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 25, 2022
@nikomatsakis nikomatsakis mentioned this pull request May 26, 2022
Open
@nikomatsakis
Copy link
Contributor

nikomatsakis commented May 26, 2022 •
edited
Loading

I dug into this with @dingxiangfei2009 -- this PR seems like a good next step. I hadn't anticipated constructing the region-scope-tree to be a significant cost, but it makes sense, particularly in incremental builds, where we are doing less work overall, and so it pays to avoid it.

I think we could probably optimize things further by removing the region_scope_tree entirely, as I described in #97426, but we shouldn't block on that, as it will be somewhat involved.

@nikomatsakis
Copy link
Contributor

r=me with that change

@nikomatsakis
Copy link
Contributor

@bors delegate+

@bors
Copy link
Contributor

bors commented May 26, 2022

✌️ @dingxiangfei2009 can now approve this pull request

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented May 26, 2022

⌛ Testing commit 5ced891 with merge 35463d158c63c4b4585f5038c3659fa1eca061bd...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented May 26, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 26, 2022
@cjgillot
Copy link
Contributor

@bors retry

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

bors commented May 26, 2022

⌛ Testing commit 5ced891 with merge f65e03b283bd1d4f2c8c9280635280236aad50cf...

@bors
Copy link
Contributor

bors commented May 26, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 26, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@dingxiangfei2009 @lqd
remove trailing space
cd76f2e 
Co-authored-by: Rémy Rakic <remy.rakic+github@gmail.com>
@dingxiangfei2009
Copy link
Contributor Author

@bors r+

@bors
Copy link
Contributor

bors commented May 28, 2022

📌 Commit cd76f2e has been approved by dingxiangfei2009

@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 May 28, 2022
@dingxiangfei2009
Copy link
Contributor Author

@bors retry

@bors
Copy link
Contributor

bors commented May 28, 2022

⌛ Testing commit cd76f2e with merge 4f39fb1...

@bors
Copy link
Contributor

bors commented May 28, 2022

☀️ Test successful - checks-actions
Approved by: dingxiangfei2009
Pushing 4f39fb1 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 28, 2022
@bors bors merged commit 4f39fb1 into rust-lang:master May 28, 2022
@rustbot rustbot added this to the 1.63.0 milestone May 28, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (4f39fb1): comparison url.

Instruction count

  • Primary benchmarks: 🎉 relevant improvements found
  • Secondary benchmarks: mixed results
mean1 max count2
Regressions 😿
(primary)		 N/A N/A 0
Regressions 😿
(secondary)		 1.0% 1.0% 3
Improvements 🎉
(primary)		 -1.7% -4.7% 98
Improvements 🎉
(secondary)		 -2.4% -7.7% 43
All 😿🎉 (primary) -1.7% -4.7% 98

Max RSS (memory usage)

Results
  • Primary benchmarks: 🎉 relevant improvements found
  • Secondary benchmarks: mixed results
mean1 max count2
Regressions 😿
(primary)		 3.7% 6.8% 4
Regressions 😿
(secondary)		 2.3% 3.6% 3
Improvements 🎉
(primary)		 -3.8% -10.4% 52
Improvements 🎉
(secondary)		 -4.3% -8.4% 23
All 😿🎉 (primary) -3.3% -10.4% 56

Cycles

Results
  • Primary benchmarks: 🎉 relevant improvements found
  • Secondary benchmarks: 🎉 relevant improvements found
mean1 max count2
Regressions 😿
(primary)		 N/A N/A 0
Regressions 😿
(secondary)		 2.5% 2.5% 1
Improvements 🎉
(primary)		 -2.6% -4.3% 21
Improvements 🎉
(secondary)		 -3.5% -6.6% 19
All 😿🎉 (primary) -2.6% -4.3% 21

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

@rustbot label: -perf-regression

Footnotes

  1. the arithmetic mean of the percent change ↩ ↩2 ↩3

  2. number of relevant changes ↩ ↩2 ↩3

@dingxiangfei2009 dingxiangfei2009 deleted the restore-region-scope-tree-query branch June 3, 2022 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lqd lqd lqd left review comments

@nikomatsakis nikomatsakis nikomatsakis approved these changes

Assignees

@nagisa nagisa

Labels
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
Milestone
1.63.0
Development

Successfully merging this pull request may close these issues.