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

Simplify caching and storage for queries #101307

Merged
merged 8 commits into from
Sep 14, 2022
Merged

Simplify caching and storage for queries #101307

merged 8 commits into from
Sep 14, 2022

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Sep 2, 2022

I highly recommend reviewing commit-by-commit; each individual commit is quite small but it can be hard to see looking at the overall diff that the behavior is the same. Each commit depends on the previous.

r? @cjgillot

@jyn514 jyn514 added the A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) label Sep 2, 2022
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 2, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 2, 2022
@jyn514
Copy link
Member Author

jyn514 commented Sep 2, 2022

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

bors commented Sep 2, 2022

⌛ Trying commit 1b32d6190360f1e2e0a199705ce352d4bb34767d with merge b8b3b08be266b740bb07752f836e0adbb20f5243...

@bors
Copy link
Contributor

bors commented Sep 2, 2022

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

@rust-timer
Copy link
Collaborator

Queued b8b3b08be266b740bb07752f836e0adbb20f5243 with parent e4f0493, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (b8b3b08be266b740bb07752f836e0adbb20f5243): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 1.4% [1.4%, 1.4%] 1
Improvements ✅
(primary)		 -0.3% [-0.3%, -0.3%] 2
Improvements ✅
(secondary)		 -0.2% [-0.3%, -0.2%] 6
All ❌✅ (primary) -0.3% [-0.3%, -0.3%] 2

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)		 2.4% [2.4%, 2.4%] 1
Regressions ❌
(secondary)		 3.5% [2.1%, 4.4%] 3
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -2.9% [-2.9%, -2.9%] 1
All ❌✅ (primary) 2.4% [2.4%, 2.4%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

Footnotes

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

  2. number of relevant changes ↩ ↩2

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Sep 2, 2022
@jyn514
Copy link
Member Author

jyn514 commented Sep 2, 2022

The single large regression is in llvm, and there are many smaller improvements. I think this is just a change in codegen units that will go away on any further small change.

@cjgillot cjgillot 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 Sep 4, 2022
@jyn514 jyn514 force-pushed the simplify-storage branch 2 times, most recently from 3608906 to 0339a00 Compare September 5, 2022 11:49
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jyn514 jyn514 force-pushed the simplify-storage branch 2 times, most recently from 468f1ec to 69ce855 Compare September 5, 2022 12:15
@jyn514 jyn514 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 7, 2022
@jyn514 jyn514 mentioned this pull request Sep 7, 2022
Merged
@bors
Copy link
Contributor

bors commented Sep 8, 2022

☔ The latest upstream changes (presumably #101303) made this pull request unmergeable. Please resolve the merge conflicts.

@jyn514
Make the storage query modifier less general
fb0c36a 
In practice, it was only ever used with `ArenaCacheSelector`. Change it to a single boolean
`arena_cache` rather than allowing queries to specify an arbitrary type.
@jyn514
Simplify the implementation of rustc_queries
b7c9687
@jyn514
Don't use a custom disk loader for diagnostic_only_typeck
8da754e 
This uses exactly the same types for query results as `typeck`, which doesn't have custom code.
It's not clear to me why this code exists (it goes back even before queries used a proc macro),
but it compiles fine without the custom loader. Remove it for simplicity.
@jyn514
Remove dead load_cached code in rustc_query
112419c
@jyn514
Don't create a new try_load_from_disk closure for each query
b164dbc 
Instead, define a single function, parameterized only by the return type.
@jyn514
Remove cache_on_disk from QueryVTable
7208bde 
This is not only simpler, but removes a generic function and unwrap.
I have hope it will see compile time and bootstrap time improvements.
@jyn514
Move TRY_LOAD_FROM_DISK out of rustc_queries to rustc_query_impl
9273782 
We want to refer to `crate::plumbing::try_load_from_disk` in the const, but hard-coding it in
rustc_queries, where we don't yet know the crate this macro will be called in, seems kind of hacky.
Do it in query_impl instead.
@jyn514
Remove unnecessary TRY_LOAD_FROM_DISK constant
0a9d7db
@cjgillot
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Sep 13, 2022

📌 Commit 0a9d7db has been approved by cjgillot

It is now in the queue for this repository.

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

bors commented Sep 14, 2022

⌛ Testing commit 0a9d7db with merge a5b58ad...

@bors
Copy link
Contributor

bors commented Sep 14, 2022

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing a5b58ad to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 14, 2022
@bors bors merged commit a5b58ad into rust-lang:master Sep 14, 2022
@rustbot rustbot added this to the 1.65.0 milestone Sep 14, 2022
@bors bors mentioned this pull request Sep 14, 2022
Merged
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (a5b58ad): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

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
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 1.7% [1.7%, 1.7%] 1
Improvements ✅
(primary)		 -0.2% [-0.3%, -0.2%] 7
Improvements ✅
(secondary)		 -0.3% [-0.3%, -0.3%] 3
All ❌✅ (primary) -0.2% [-0.3%, -0.2%] 7

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 1.4% [1.4%, 1.4%] 1
Improvements ✅
(primary)		 -3.2% [-3.2%, -3.2%] 1
Improvements ✅
(secondary)		 -3.4% [-3.6%, -3.2%] 2
All ❌✅ (primary) -3.2% [-3.2%, -3.2%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

Footnotes

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

  2. number of relevant changes ↩ ↩2

@nnethercote
Copy link
Contributor

The deep-vector debug full regression could be noise. Even if it's not, a single run of a secondary benchmark doesn't matter much, esp. given there are many small improvements across many other benchmarks.

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Sep 14, 2022
@jyn514 jyn514 deleted the simplify-storage branch September 14, 2022 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjgillot cjgillot cjgillot left review comments

Assignees

@cjgillot cjgillot

Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) 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.
Projects
None yet
Milestone
1.65.0
Development

Successfully merging this pull request may close these issues.
8 participants
@jyn514 @rust-timer @bors @rust-log-analyzer @cjgillot @nnethercote @rust-highfive @rustbot