Streamline rustc_span::HashStableContext.#152318
Streamline rustc_span::HashStableContext.#152318rust-bors[bot] merged 1 commit intorust-lang:mainfrom
rustc_span::HashStableContext.#152318Conversation
|
@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.
…try> Streamline `rustc_span::HashStableContext`.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (8d7aeb5): comparison URL. Overall result: ❌ regressions - 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)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 sizeResults (primary -0.0%, secondary -0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 476.152s -> 476.94s (0.17%) |
|
This is the part of #152086 that I pulled out because of performance regressions. Locally I don't see regressions, I see very small improvements, which suggests it's a minor thing like an inlining choice. Also, this PR affects incremental hashing, so rustc-perf's enabling of |
Currently this trait has five methods. But it only really needs three. For example, currently stable hashing of spans is implemented in `rustc_span`, except a couple of sub-operations are delegated to `rustc_query_system`: `def_span` and `span_data_to_lines_and_cols`. These two delegated sub-operations can be reduced to a single delegated operation that does the full hash computation. Likewise, `assert_default_hashing_controls` depends on two delegated sub-operations, `hashing_controls` and `unstable_opts_incremental_ignore_spans`, and can be simplified. I find the resulting code simpler and clearer -- when necessary, we do a whole operation in `rustc_query_system` instead of doing it partly in `rustc_span` and partly in `rustc_query_system`.
01a9f26 to
0f31083
Compare
|
Let's try a slightly different @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.
…try> Streamline `rustc_span::HashStableContext`.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (1381776): comparison URL. Overall result: no relevant changes - 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 countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesResults (secondary -0.7%)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: 471.737s -> 475.18s (0.73%) |
The inlining tweak fixed the perf regression, and @cjgillot already approved this commit in #152086, so this is ready to merge. @bors r=cjgillot |
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 1c316d3 (parent) -> 39219ce (this PR) Test differencesShow 18 test diffs18 doctest diffs were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 39219ceb97d1b37dda72517daa9ebe8364ffe186 --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 (39219ce): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (primary -4.3%, secondary -5.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 3.0%)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: 475.432s -> 477.309s (0.39%) |
Currently this trait has five methods. But it only really needs three.
For example, currently stable hashing of spans is implemented in
rustc_span, except a couple of sub-operations are delegated torustc_query_system:def_spanandspan_data_to_lines_and_cols. These two delegated sub-operations can be reduced to a single delegated operation that does the full hash computation.Likewise,
assert_default_hashing_controlsdepends on two delegated sub-operations,hashing_controlsandunstable_opts_incremental_ignore_spans, and can be simplified.I find the resulting code simpler and clearer -- when necessary, we do a whole operation in
rustc_query_systeminstead of doing it partly inrustc_spanand partly inrustc_query_system.r? @cjgillot