-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Ensure that Fingerprint
caching respects hashing configuration
#92278
Conversation
r? @jackh726 (rust-highfive has picked a reviewer for you, use r? to override) |
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit 0133c532074b7f9b3185f73069fd0efab8ad21ad with merge 4a8e40c5ad7cde9c3dba8f8633d09ed1df158899... |
This comment has been minimized.
This comment has been minimized.
☀️ Try build successful - checks-actions |
Queued 4a8e40c5ad7cde9c3dba8f8633d09ed1df158899 with parent c096176, future comparison URL. |
Finished benchmarking commit (4a8e40c5ad7cde9c3dba8f8633d09ed1df158899): comparison url. Summary: This change led to moderate relevant mixed results 🤷 in compiler performance.
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. 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 led to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never |
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit 900fec7d020395fabd3a2b8089a81a9b1fd5c8a9 with merge dd25572bf85b48f81c20ee8fffaac8f406d00af2... |
💥 Test timed out |
This comment has been minimized.
This comment has been minimized.
@bors try |
⌛ Trying commit 900fec7d020395fabd3a2b8089a81a9b1fd5c8a9 with merge 48a976a697a2b214a52a74e580f2cd347cd15f9d... |
☀️ Try build successful - checks-actions |
Queued 48a976a697a2b214a52a74e580f2cd347cd15f9d with parent f8abed9, future comparison URL. |
⌛ Testing commit 4ca275a with merge b431595d1630725bd11aabe6d349a937a2e9d37d... |
💔 Test failed - checks-actions |
@bors retry |
⌛ Testing commit 4ca275a with merge 98357a9e461aa033dfcb273430a6aa91a602ae04... |
💔 Test failed - checks-actions |
@bors retry |
☀️ Test successful - checks-actions |
Finished benchmarking commit (d63a8d9): comparison url. Summary: This change led to moderate relevant regressions 😿 in compiler performance.
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression |
This was already identified as being an acceptable performance hit since it is a correctness fix. I'll mark this as triaged for now. @rustbot label: +perf-regression-triaged |
Fixes #92266
In some
HashStable
impls, we use a cache to avoid re-computingthe same
Fingerprint
from the same structure (e.g. anAdtDef
).However, the
StableHashingContext
used can be configured toperform hashing in different ways (e.g. skipping
Span
s). Thisconfiguration information is not included in the cache key,
which will cause an incorrect
Fingerprint
to be used ifwe hash the same structure with different
StableHashingContext
settings.
To fix this, the configuration settings of
StableHashingContext
are split out into a separate
HashingControls
struct. Thisstruct is used as part of the cache key, ensuring that our caches
always produce the correct result for the given settings.
With this in place, we now turn off
Span
hashing during theentire process of computing the hash included in legacy symbols.
This current has no effect, but will matter when a future PR
starts hashing more
Span
s that we currently skip.