-
-
Notifications
You must be signed in to change notification settings - Fork 14.5k
Stable hashing cleanups #152086
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
Stable hashing cleanups #152086
Conversation
|
LGTM |
|
@bors try @rust-timer queue Just to be sure :) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
06afe61 to
c1d3cfc
Compare
|
@bors try cancel |
|
Try build cancelled. Cancelled workflows: |
|
@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.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (540561d): 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 (primary -3.2%, secondary -2.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 2.8%)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.808s -> 473.718s (-0.44%) |
c1d3cfc to
52c330a
Compare
This comment has been minimized.
This comment has been minimized.
|
Locally I get small perf improvements, of a similar magnitude to the regressions seen on CI :( Cachegrind diffs indicate it's something about @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
It has a single use. It doesn't need to be public. It doesn't use `self` and so doesn't need to be in the trait. And `IGNORED_ATTRIBUTES` can be moved within it.
It has a single `bool` field.
Calling `match` on a struct is a really weird thing to do. As the name suggests, it's an assert, so let's write it as one. Also clarify the comment a little.
It reads the `HashingControls::span` field, but there is also the `hashing_controls` method. No need to have two different ways of doing it.
It has a single impl, which does nothing, as you'd expect when hashing a type called `HashIgnoredAttrId`! So this commit just removes it, and `HashIgnoredAttrId::hash_stable` ends up doing nothing (with a comment) instead of calling the trait method to do nothing.
The new type makes the behaviour clearer: we start with the cache in an "unused" form and then instantiate it once it is actually used.
They both have a single use. Also adjust a couple of visibilities.
`HashStableContext` impls should be in `hcx.rs`, and `HashStable` impls should be in `impls_syntax.rs`. This commit moves a few that are in the wrong file.
It only has two uses. We can instead use `with_stable_hashing_context`, which has more than 30 uses.
I tried removing it to see what happened. Not a good idea!
f87bdac to
e35fd45
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
I rebased. @bors r=cjgillot |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
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 a3ceeeb (parent) -> cf16cd9 (this PR) Test differencesShow 30 test diffs30 doctest diffs were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard cf16cd9b751f6e13caec393cef4f6f67de2f3da8 --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 (cf16cd9): 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.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.7%, secondary 2.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.0%, secondary -0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 475.331s -> 474.937s (-0.08%) |
I took a close look at
rustc_query_system::ichand found a bunch of things to improve.r? @cjgillot