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

Run various queries from other queries instead of explicitly in phases #108118

Merged
merged 4 commits into from
Apr 23, 2023

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Feb 16, 2023

These are just legacy leftovers from when rustc didn't have a query system. While there are more cleanups of this sort that can be done here, I want to land them in smaller steps.

This phased order of query invocations was already a lie, as any query that looks at types (e.g. the wf checks run before) can invoke e.g. const eval which invokes borrowck, which invokes typeck, ...

@rustbot
Copy link
Collaborator

rustbot commented Feb 16, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @wesleywiser (or someone else) soon.

Please see the contribution instructions for more information.

@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 16, 2023

@bors try @rust-timer queue

@rustbot rustbot added 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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Feb 16, 2023
@rust-timer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Feb 16, 2023

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 16, 2023
@bors
Copy link
Contributor

bors commented Feb 16, 2023

⌛ Trying commit a1bf293aea5384252c0d7227f46dcf534667aeb1 with merge 41c8d3a20cad3d4de552339500551ada93f0d722...

@@ -529,8 +529,6 @@ pub fn check_crate(tcx: TyCtxt<'_>) -> Result<(), ErrorGuaranteed> {
tcx.hir().for_each_module(|module| tcx.ensure().check_mod_item_types(module))
});

tcx.sess.time("item_bodies_checking", || tcx.typeck_item_bodies(()));
Copy link
Member

Choose a reason for hiding this comment

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

What is going to ensure that typecheck errors for all functions are reported without this call?

Copy link
Member

Choose a reason for hiding this comment

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

This also regresses the quality of the output of -Ztime-passes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as mentioned in the main post

This phased order of query invocations was already a lie, as any query that looks at types (e.g. the wf checks run before) can cause e.g. const eval which invokes borrowck, which invokes typeck, ...

we already didn't produce correct time tracking for it

@jackh726
Copy link
Member

I think this is a good change, but maybe a safer intermediate step would be to "assert" that all the queries were run in the "passes" for now. This would ensure that we don't accidentally make a query more lazy, then skip something (accidentally).

@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 16, 2023

Hmm... we don't have the setup for doing that (also doing it at that phase won't work, as it may get run later). I'll see if I can add something to the query system for asserting that a query has been run

@bors
Copy link
Contributor

bors commented Feb 16, 2023

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

@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 16, 2023

This would ensure that we don't accidentally make a query more lazy, then skip something (accidentally).

I looked into it, and we're already relying on the mir_built query to run thir_check_unsafety. So we're already assuming that mir_built gets run for all things with bodies. I agree some assertions would be neat, but generating a check-function per query and calling it is a lot. Limiting it to certain queries can be done with more query flags, but that seems like a lot of infrastructure for something that we can be certain won't happen (as we'd already be missing checks in that case).

@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 16, 2023

@rust-timer build 41c8d3a20cad3d4de552339500551ada93f0d722

@rust-timer

This comment has been minimized.

@cjgillot
Copy link
Contributor

I'm not sure I agree with the general direction. I'd rather suggest doing the opposite:

  • remove useless dependencies from queries -> avoid useless recomputations;
  • invoke all the required "check" queries from analysis -> ensure correctness-related stuff are in a single place.

The fact that we run thir_check_unsafety from mir_built is more accidental, due to the fact that mir_built steals THIR. We probably shouldn't really rely on it for correctness. Even if having ensure().mir_borrowck in analysis is enough to make sure that mir_built is computed and thir_check_unsafety too.

@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 16, 2023

The problem with those useless dependencies is that e.g. const eval can run on code that doesn't satisfy various checks, leading to surprising bugs and workarounds in const eval. I agree that mir_built may be too early, but at the very least the mir_drops_elaborated_and_const_checked query should not finish before the various checks on the same def id are done.

I guess my assumption was that at some point all we have to do is to either call the optimized_mir query for the main function and can compile from just the information from that, or call the analysis query to do a check build.

If changing any of this negatively affects incremental, then we should not do it, but I don't see that being an issue in general with this scheme, just with the details of where to add edges.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (41c8d3a20cad3d4de552339500551ada93f0d722): 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-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.

mean range count
Regressions ❌
(primary)
0.7% [0.7%, 0.7%] 1
Regressions ❌
(secondary)
0.7% [0.7%, 0.7%] 1
Improvements ✅
(primary)
-0.5% [-0.6%, -0.5%] 6
Improvements ✅
(secondary)
-1.6% [-1.6%, -1.6%] 1
All ❌✅ (primary) -0.4% [-0.6%, 0.7%] 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.

mean range count
Regressions ❌
(primary)
1.1% [1.1%, 1.1%] 1
Regressions ❌
(secondary)
2.0% [1.4%, 2.8%] 10
Improvements ✅
(primary)
-1.6% [-2.5%, -1.0%] 3
Improvements ✅
(secondary)
-3.9% [-7.1%, -2.5%] 5
All ❌✅ (primary) -0.9% [-2.5%, 1.1%] 4

Cycles

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.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.6% [-1.7%, -1.6%] 2
Improvements ✅
(secondary)
-2.2% [-2.5%, -2.0%] 2
All ❌✅ (primary) -1.6% [-1.7%, -1.6%] 2

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Feb 16, 2023
@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 16, 2023

Everything but assoc_many_items in doc mode is noise.

@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 16, 2023

  • remove useless dependencies from queries -> avoid useless recomputations;

about that, do we really add more dependencies with ensure? Doesn't ensure just mean that the ensured query must be run before the current one without any change in the dep-graph otherwise?

@cjgillot
Copy link
Contributor

Yes, ensure adds a dependency edge. And the calls to ensure just before stealing a value bother me a bit: they introduce a dependency to a value that will not be used, and don't even make sure the code will not be called later. ensure may "just" mark the query as green, without loading any value, to be recomputed later when the value is actually required.

The problem with those useless dependencies is that e.g. const eval can run on code that doesn't satisfy various checks, leading to surprising bugs and workarounds in const eval. I agree that mir_built may be too early, but at the very least the mir_drops_elaborated_and_const_checked query should not finish before the various checks on the same def id are done.

In those cases, we should rely on the trainted_by_errors fields on typeck and MIR body.

One other symptom is the calls to track_errors in rustc_hir_analysis::check_crate that shouldn't exist: typeck should rely on propagated error state via ty::Error and ty::ReError. But that FIXME exists since 2020, so that's not high priority either.

If changing any of this negatively affects incremental, then we should not do it, but I don't see that being an issue in general with this scheme, just with the details of where to add edges.

From the perf results, I have no proof that it does negatively affect incremental, so no serious grounds on which to block this PR. That's also why I frame it as a design question, more than an operational question.

That said, we should merge that PR, as it does not lock anything in a way or the other. I'll have to make my case with an actual PR demonstrating the direction I have in mind 😄.

@rust-log-analyzer

This comment has been minimized.

@cjgillot cjgillot self-assigned this Apr 20, 2023
@wesleywiser
Copy link
Member

r=me when @cjgillot is happy 🙂

@bors
Copy link
Contributor

bors commented Apr 21, 2023

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

…voking it eagerly.

Later queries that are run on all body owners will invoke typeck as they need information from its result to perform their own logic
@cjgillot
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Apr 23, 2023

📌 Commit 3344232 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 Apr 23, 2023
@bors
Copy link
Contributor

bors commented Apr 23, 2023

⌛ Testing commit 3344232 with merge 3462f79...

@bors
Copy link
Contributor

bors commented Apr 23, 2023

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

1 similar comment
@bors
Copy link
Contributor

bors commented Apr 23, 2023

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

@bors bors added merged-by-bors This PR was explicitly merged by bors. labels Apr 23, 2023
@bors bors merged commit 3462f79 into rust-lang:master Apr 23, 2023
@rustbot rustbot added this to the 1.71.0 milestone Apr 23, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (3462f79): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

@rustbot label: -perf-regression

Instruction count

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

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.5% [0.5%, 0.5%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.3% [-0.3%, -0.2%] 4
All ❌✅ (primary) - - 0

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.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.7% [-4.4%, -1.6%] 6
Improvements ✅
(secondary)
-1.4% [-1.4%, -1.4%] 1
All ❌✅ (primary) -2.7% [-4.4%, -1.6%] 6

Cycles

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.

mean range count
Regressions ❌
(primary)
1.6% [1.6%, 1.6%] 1
Regressions ❌
(secondary)
3.8% [3.8%, 3.8%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.6% [1.6%, 1.6%] 1

@rustbot rustbot removed the perf-regression Performance regression. label Apr 23, 2023
flip1995 pushed a commit to flip1995/rust that referenced this pull request May 5, 2023
Run various queries from other queries instead of explicitly in phases

These are just legacy leftovers from when rustc didn't have a query system. While there are more cleanups of this sort that can be done here, I want to land them in smaller steps.

This phased order of query invocations was already a lie, as any query that looks at types (e.g. the wf checks run before) can invoke e.g. const eval which invokes borrowck, which invokes typeck, ...
@SparrowLii SparrowLii mentioned this pull request Jul 10, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 14, 2023
typeck in parallel

rust-lang#108118 caused `typeck` to be transferred to the serial part (`check_unused`), which made the performance of parallel rustc significantly reduced.

This pr re-parallelize this part, which increases the average performance improvement of parallel rustc in `full` and `incr-full` scenarios from [14.4%](rust-lang#110284 (comment)) to [23.2%](rust-lang#110284 (comment)).

r? `@cjgillot`
cc `@oli-obk` `@Zoxc`
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. 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.

10 participants