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

Cache more queries on disk #95418

Merged
merged 2 commits into from
May 20, 2022
Merged

Cache more queries on disk #95418

merged 2 commits into from
May 20, 2022

Conversation

cjgillot
Copy link
Contributor

@cjgillot cjgillot commented Mar 28, 2022

One of the principles of incremental compilation is to allow saving results on disk to avoid recomputing them.
This PR investigates persisting a lot of queries whose result are to be saved into metadata.
Some of the queries are cheap reads from HIR, but we may also want to get rid of these reads for incremental lowering.

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Mar 28, 2022
@cjgillot

This comment was marked as outdated.

@rust-timer

This comment was marked as outdated.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 28, 2022
@bors

This comment was marked as outdated.

@rust-log-analyzer

This comment has been minimized.

@bors

This comment was marked as outdated.

@rust-timer

This comment was marked as outdated.

@rust-timer

This comment was marked as outdated.

@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 Mar 29, 2022
@cjgillot
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 Mar 29, 2022
@bors
Copy link
Contributor

bors commented Mar 29, 2022

⌛ Trying commit ae9af5f539ba1eca44562ff9a400d8b46cdc66c6 with merge 36ace5e9edc63601569a7ef878ded180015f85d3...

separate_provide_extern
}

query impl_constness(def_id: DefId) -> hir::Constness {
desc { |tcx| "looking up whether `{}` is a const impl", tcx.def_path_str(def_id) }
cache_on_disk_if { def_id.is_local() }
Copy link
Member

@bjorn3 bjorn3 Mar 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't those two just a cheap hir lookup?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. For the moment, I'm trying some heuristic like "cache everything that will be stored in metadata (roughly if is has separate_provide_extern)". I plan to refine this from perf measurements.

@bors
Copy link
Contributor

bors commented Mar 29, 2022

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

@rust-timer
Copy link
Collaborator

Queued 36ace5e9edc63601569a7ef878ded180015f85d3 with parent e2301ca, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (36ace5e9edc63601569a7ef878ded180015f85d3): comparison url.

Summary: This benchmark run shows 83 relevant improvements 🎉 but 43 relevant regressions 😿 to instruction counts.

  • Arithmetic mean of relevant regressions: 1.3%
  • Arithmetic mean of relevant improvements: -2.1%
  • Arithmetic mean of all relevant changes: -0.9%
  • Largest improvement in instruction counts: -34.3% on incr-unchanged builds of externs debug
  • Largest regression in instruction counts: 4.5% on incr-unchanged builds of issue-58319 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.

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 Mar 29, 2022
@cjgillot
Copy link
Contributor Author

r? rust-lang/compiler

@rust-timer

This comment was marked as outdated.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 29, 2022
@cjgillot cjgillot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 29, 2022
@cjgillot
Copy link
Contributor Author

After investigation, I'm not sure how I can optimize the disk effect. One of the key effects is to avoid recomputing resolve_lifetimes (for instance here). This is done by skipping all "astconv" queries which access it indirectly. I'd rather avoid caching resolve_lifetimes, preferring queries whose results will be written into metadata.

@davidtwco
Copy link
Member

If this is waiting on another review from me then all the changes in this PR still look good to me, so r=me if you don't want to do further changes.

After investigation, I'm not sure how I can optimize the disk effect. One of the key effects is to avoid recomputing resolve_lifetimes (for instance here). This is done by skipping all "astconv" queries which access it indirectly. I'd rather avoid caching resolve_lifetimes, preferring queries whose results will be written into metadata.

I'm not familiar enough with the relevant code to have any suggestions here unfortunately.

@cjgillot
Copy link
Contributor Author

Let's confirm there are no unforeseen regressions, and I think we'll go ahead.

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

bors commented May 17, 2022

⌛ Trying commit 29f3b3f with merge 439d2588a96abfba733f7cef6e0f998d18042636...

@bors
Copy link
Contributor

bors commented May 17, 2022

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

@rust-timer
Copy link
Collaborator

Queued 439d2588a96abfba733f7cef6e0f998d18042636 with parent 3655175, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (439d2588a96abfba733f7cef6e0f998d18042636): comparison url.

Summary:

  • Primary benchmarks: mixed results
  • Secondary benchmarks: mixed results
Regressions 😿
(primary)
Regressions 😿
(secondary)
Improvements 🎉
(primary)
Improvements 🎉
(secondary)
All 😿 🎉
(primary)
count1 27 28 65 27 92
mean2 0.9% 1.7% -1.0% -4.8% -0.5%
max 2.0% 4.1% -3.7% -36.4% -3.7%

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.

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 removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 17, 2022
@cjgillot
Copy link
Contributor Author

@bors r=davidtwco

@bors
Copy link
Contributor

bors commented May 20, 2022

📌 Commit 29f3b3f 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 May 20, 2022
@cjgillot
Copy link
Contributor Author

cjgillot commented May 20, 2022

This PR caches many more queries' results on disk. The queries have been chosen to be: (1) outputs of typechecking, and (2) parts of metadata. This caching allows for wide and large compile time savings, up to 36%. The tradeoff is a compile time increase (4%) when these caches cannot be usefully used, and a increased on-disk size around 30%.
@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label May 20, 2022
@bors
Copy link
Contributor

bors commented May 20, 2022

⌛ Testing commit 29f3b3f with merge e6a4afc...

@bors
Copy link
Contributor

bors commented May 20, 2022

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

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

Finished benchmarking commit (e6a4afc): comparison url.

Instruction count

  • Primary benchmarks: mixed results
  • Secondary benchmarks: mixed results
Regressions 😿
(primary)
Regressions 😿
(secondary)
Improvements 🎉
(primary)
Improvements 🎉
(secondary)
All 😿 🎉
(primary)
count1 33 32 65 28 98
mean2 0.8% 1.6% -1.0% -4.4% -0.4%
max 2.1% 4.0% -3.6% -33.0% -3.6%

Max RSS (memory usage)

Results
  • Primary benchmarks: 😿 relevant regressions found
  • Secondary benchmarks: mixed results
Regressions 😿
(primary)
Regressions 😿
(secondary)
Improvements 🎉
(primary)
Improvements 🎉
(secondary)
All 😿 🎉
(primary)
count1 17 5 2 8 19
mean2 1.3% 1.6% -1.1% -1.9% 1.1%
max 2.4% 1.9% -1.2% -2.5% 2.4%

Cycles

Results
  • Primary benchmarks: mixed results
  • Secondary benchmarks: mixed results
Regressions 😿
(primary)
Regressions 😿
(secondary)
Improvements 🎉
(primary)
Improvements 🎉
(secondary)
All 😿 🎉
(primary)
count1 11 16 8 9 19
mean2 4.0% 4.6% -3.0% -7.4% 1.1%
max 6.0% 8.9% -4.4% -25.0% 6.0%

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 3

  2. the arithmetic mean of the percent change 2 3

@cjgillot cjgillot deleted the more-disk branch May 21, 2022 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. 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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants