Cached stable hash cleanups by oli-obk · Pull Request #95524 · rust-lang/rust ·...
source link: https://github.com/rust-lang/rust/pull/95524
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
Conversation
r? @nnethercote
Add a sanity assertion in debug mode to check that the cached hashes are actually the ones we get if we compute the hash each time.
Add a new data structure that bundles all the hash-caching work to make it easier to re-use it for different interned data structures
added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label
added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label
Awaiting bors try build completion.
@rustbot label: +S-waiting-on-perf
added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label
Trying commit 
Try build successful - checks-actions
Build commit: 0914be4 (0914be4865da468777d1ce5bdd9a546550850308)
Finished benchmarking commit (0914be4): comparison url.
Summary: This benchmark run did not return any relevant results.
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.
@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression
removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label
The new name helps, thanks.
I'm still a little unclear about the goal here.
- I can see it's nice to pull the hash out of
TyS. Because the hash is auxiliary, rather than being a fundamental part of the type. - The PR description says "I pulled out the perf improvement from #94487" but the perf results are neutral. Is that expected?
/// This is useful if you have values that you intern but never (can?) use for stable
/// hashing.
#[derive(Copy, Clone)]
pub struct TypeAndStableHash<T> {
Maybe call it WithStableHash?
I think @oli-obk means that the perf improvement is not included in this PR and only the refactorings that enable the perf improvement change.
Nope, the other PR had a perf improvement for these commits, but that seems to be gone now, maybe it was spurious or randomly dependent on inlining decisions
So is this still worthwhile for the conceptual improvement of separating the hash from TyS?
Yes, I can investigate perf things independently, editing the main message now to reflect that (and will address bjorn's comments)
Renamed and force pushed
@@ -1266,18 +1266,29 @@ pub trait PrettyPrinter<'tcx>:
ty::Ref(
_,
Ty(Interned(
Maybe use nested match with the inner match matching on pointee.kind()? This indentation is getting crazy and pretty unreadable. In addition there are other match arms where the outer type is a reference.
@oli-obk can now approve this pull request
@bors r=nnethercote
Commit 25d6f8e has been approved by nnethercote
added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
labels
Test successful - checks-actions
Approved by: nnethercote
Pushing e980c62 to master...
Finished benchmarking commit (e980c62): comparison url.
Summary:
- Primary benchmarks:
relevant improvements found - Secondary benchmarks: mixed results
Regressions 
(primary)
Regressions
(secondary)
Improvements
(primary)
Improvements
(secondary)
All
(primary)
count1 0 6 6 9 6
mean2 N/A 0.4% -0.3% -0.9% -0.3%
max N/A 0.5% -0.4% -1.3% -0.4%
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.
@rustbot label: -perf-regression
Footnotes
-
number of relevant changes
-
the arithmetic mean of the percent change
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
merged-by-bors This PR was explicitly merged by bors S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
None yet
Successfully merging this pull request may close these issues.
None yet
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK