DepNode use improvements#154076
Conversation
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
`DepNode` use improvements
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (bc93a15): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (secondary 1.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 481.003s -> 479.861s (-0.24%) |
de1e5b3 to
153cb38
Compare
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
`DepNode` use improvements
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (72bc4f3): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 480.842s -> 480.293s (-0.11%) |
153cb38 to
dcb3776
Compare
|
|
|
@bors p=4 |
This comment has been minimized.
This comment has been minimized.
`DepNode` use improvements We can simplify `DepNode` construction during query execution. Details in individual commits. r? @Zalathar
This comment has been minimized.
This comment has been minimized.
|
💔 Test for dd56731 failed: CI. Failed job:
|
This comment has been minimized.
This comment has been minimized.
The call chain for a non-incremental query includes the following
functions:
- execute_query_non_incr_inner (assert!)
- try_execute_query (assert!)
- execute_job_non_incr (assert!)
And likewise for an incremental query:
- execute_query_incr_inner (assert!)
- try_execute_query (assert!)
- execute_job_incr (expect)
That is five distinct functions. Every one of them has an `assert!` or
`expect` call that checks that the dep-graph is/is not enabled as
expected. Three cheers for defensive programming but this feels like
overkill, particularly when `execute_job{,_non_incr,_incr}` each have a
single call site.
This commit removes the assertions in `execute_query_*` and
`try_execute_query`, leaving a check in each of the `execute_job_*`
functions.
Prior to rust-lang#154122 it wasn't used on all paths, so we only computed it when necessary -- sometimes in `check_if_ensure_can_skip_execution`, sometimes in one of two places in `execute_job_incr` -- and pass around `Option<DepNode>` to allow this. But now it's always used, so this commit makes us compute it earlier, which simplifies things. - `check_if_ensure_can_skip_execution` can be made simpler, returning a bool and eliminating the need for `EnsureCanSkip`. - `execute_job_incr` no longer uses two slightly different methods to create a `DepNode` (`get_or_insert_with` vs `unwrap_or_else`).
df76ef0 to
3c7ee98
Compare
|
I rebased. The CI failure seems spurious? @bors r=Zalathar |
The call chain for a non-incremental query includes the following
functions:
- execute_query_non_incr_inner (assert!)
- try_execute_query (assert!)
- execute_job_non_incr (assert!)
And likewise for an incremental query:
- execute_query_incr_inner (assert!)
- try_execute_query (assert!)
- execute_job_incr (expect)
That is five distinct functions. Every one of them has an `assert!` or
`expect` call that checks that the dep-graph is/is not enabled as
expected. Three cheers for defensive programming but this feels like
overkill, particularly when `execute_job{,_non_incr,_incr}` each have a
single call site.
This commit removes the assertions in `execute_query_*` and
`try_execute_query`, leaving a check in each of the `execute_job_*`
functions.
Prior to rust-lang#154122 it wasn't used on all paths, so we only computed it when necessary -- sometimes in `check_if_ensure_can_skip_execution`, sometimes in one of two places in `execute_job_incr` -- and pass around `Option<DepNode>` to allow this. But now it's always used, so this commit makes us compute it earlier, which simplifies things. - `check_if_ensure_can_skip_execution` can be made simpler, returning a bool and eliminating the need for `EnsureCanSkip`. - `execute_job_incr` no longer uses two slightly different methods to create a `DepNode` (`get_or_insert_with` vs `unwrap_or_else`).
This comment has been minimized.
This comment has been minimized.
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 6f22f61 (parent) -> 212b0d4 (this PR) Test differencesShow 2 test diffs2 doctest diffs were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 212b0d480f337082bbe1132d2b62be20e7e61f8a --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (212b0d4): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (secondary -0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -2.4%, secondary 0.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 485.277s -> 495.299s (2.07%) |
View all comments
We can simplify
DepNodeconstruction during query execution. Details in individual commits.r? @Zalathar