Merge mir query analysis invocations #140856
Conversation
rustbot
added
S-waiting-on-review
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
Merge mir query analysis invocations r? `@ghost` same thing as rust-lang#140854 just a different set of queries
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (bddede8): comparison URL. Overall result: ❌ regressions - no action neededBenchmarking 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 Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary -0.9%, secondary -2.2%)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.
CyclesResults (primary 1.5%, secondary 0.2%)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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 773.559s -> 774.531s (0.13%) |
rustbot
removed
the
S-waiting-on-perf
|
@Zoxc do you think changes like this are detrimental to parallel rustc? |
|
Merging parallel sections is generally good, but actual measurements are also nice. |
|
Right. I'll figure out how to do that locally. But until then r? @nnethercote |
|
Ah no I forgot the regression only shows in cycles, not in instructions... |
|
Branch misses and cache misses are also really badly regressed. So my original analysis still stands, even if wall time itself is not really affected |
| @@ -233,6 +222,17 @@ pub fn check_crate(tcx: TyCtxt<'_>) { | |||
| } | |||
| }); | |||
|
|
|||
| if tcx.features().rustc_attrs() { | |||
There was a problem hiding this comment.
Why dump attrs stuff later? I'm not opposed, just there's no motivation given.
There was a problem hiding this comment.
I wanted to make space there to merge the parallel blocks before and after
Can you remind me what the original analysis is? I've lost track of the motivation for this PR. Is it for code simplification, performance, both/something else? Thanks. |
|
|
The reasons I'm doing it is that
|
Aha, that helps a lot. Please put that explanation in the commit message for the third commit, and then r=me, thanks. |
The reasons I'm doing it is that * merging those blocks allows for more parallelism as you don't run parallel blocks in sequence * merging blocks allows merging analysis queries shrinking the dep graph * should allow us to do more early aborting in case of errors and/or moving query calls from the analysis query into others that allow early aborting the others (and doing more tainting and stuff)
|
@bors r=nnethercote |
bors
removed
the
S-waiting-on-review
bors
added
the
S-waiting-on-bors
Merge mir query analysis invocations r? `@ghost` same thing as rust-lang#140854 just a different set of queries Doing this in general has some bad cache coherence issues because the query caches are laid out in Vec<QueryResult> lists per query where each index refers to a DefId in the same order as we're iterating. Iterating two or more lists at the same time does have cache issues, so I want to poke a bit at it to see if we can't merge just a few of them at a time.
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
@bors retry |
bors
added
S-waiting-on-bors
|
@bors rollup=never |
Merge mir query analysis invocations r? `@ghost` same thing as rust-lang#140854 just a different set of queries Doing this in general has some bad cache coherence issues because the query caches are laid out in Vec<QueryResult> lists per query where each index refers to a DefId in the same order as we're iterating. Iterating two or more lists at the same time does have cache issues, so I want to poke a bit at it to see if we can't merge just a few of them at a time.
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
|
bors
added
S-waiting-on-bors
Merge mir query analysis invocations r? `@ghost` same thing as rust-lang#140854 just a different set of queries Doing this in general has some bad cache coherence issues because the query caches are laid out in Vec<QueryResult> lists per query where each index refers to a DefId in the same order as we're iterating. Iterating two or more lists at the same time does have cache issues, so I want to poke a bit at it to see if we can't merge just a few of them at a time.
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
r? @ghost
same thing as #140854 just a different set of queries
Doing this in general has some bad cache coherence issues because the query caches are laid out in Vec lists per query where each index refers to a DefId in the same order as we're iterating. Iterating two or more lists at the same time does have cache issues, so I want to poke a bit at it to see if we can't merge just a few of them at a time.