Cached stable hash cleanups #95524

oli-obk · 2022-03-31T14:56:55Z

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

…h other interned types.

…s are enabled

compiler/rustc_data_structures/src/intern.rs

nnethercote · 2022-03-31T20:29:11Z

@bors try @rust-timer queue

rust-timer · 2022-03-31T20:29:12Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-03-31T20:29:19Z

⌛ Trying commit 6ffd654 with merge 0914be4865da468777d1ce5bdd9a546550850308...

bors · 2022-03-31T22:09:50Z

☀️ Try build successful - checks-actions
Build commit: 0914be4865da468777d1ce5bdd9a546550850308 (0914be4865da468777d1ce5bdd9a546550850308)

rust-timer · 2022-03-31T22:09:51Z

Queued 0914be4865da468777d1ce5bdd9a546550850308 with parent 0677edc, future comparison URL.

rust-timer · 2022-03-31T23:28:39Z

Finished benchmarking commit (0914be4865da468777d1ce5bdd9a546550850308): 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

nnethercote · 2022-04-07T05:47:54Z

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 Also cache the stable hash of interned Predicates #94487" but the perf results are neutral. Is that expected?

compiler/rustc_middle/src/ty/mod.rs

bjorn3 · 2022-04-07T11:19:21Z

compiler/rustc_data_structures/src/intern.rs

+/// 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?

bjorn3 · 2022-04-07T11:21:29Z

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.

oli-obk · 2022-04-07T11:43:10Z

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

nnethercote · 2022-04-07T11:59:31Z

So is this still worthwhile for the conceptual improvement of separating the hash from TyS?

oli-obk · 2022-04-07T12:04:41Z

Yes, I can investigate perf things independently, editing the main message now to reflect that (and will address bjorn's comments)

oli-obk · 2022-04-07T13:02:07Z

Renamed and force pushed

bjorn3 · 2022-04-07T13:56:07Z

compiler/rustc_middle/src/ty/print/pretty.rs

@@ -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.

nnethercote · 2022-04-07T22:59:50Z

Ok by me, I'll let you address @bjorn3's final comment if you want.

@bors delegate+

bors · 2022-04-07T22:59:52Z

✌️ @oli-obk can now approve this pull request

oli-obk · 2022-04-08T15:58:10Z

@bors r=nnethercote

bors · 2022-04-08T15:58:11Z

📌 Commit 25d6f8e has been approved by nnethercote

bors · 2022-04-09T02:31:27Z

⌛ Testing commit 25d6f8e with merge e980c62...

bors · 2022-04-09T04:53:28Z

☀️ Test successful - checks-actions
Approved by: nnethercote
Pushing e980c62 to master...

rust-timer · 2022-04-09T07:05:02Z

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)
count¹	0	6	6	9	6
mean²	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

number of relevant changes ↩
the arithmetic mean of the percent change ↩

…r=nnethercote Cached stable hash cleanups 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

oli-obk added 3 commits March 31, 2022 14:54

Move stable hash from TyS into a datastructure that can be shared wit…

00c24dd

…h other interned types.

inline a trivial function

33d0ce9

Check that the cached stable hash is the right one if debug assertion…

6ffd654

…s are enabled

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 31, 2022

rust-highfive assigned nnethercote Mar 31, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 31, 2022

nnethercote reviewed Mar 31, 2022

View reviewed changes

compiler/rustc_data_structures/src/intern.rs Outdated Show resolved Hide resolved

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 31, 2022

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 31, 2022

bjorn3 reviewed Apr 7, 2022

View reviewed changes

compiler/rustc_middle/src/ty/mod.rs Show resolved Hide resolved

bjorn3 reviewed Apr 7, 2022

View reviewed changes

Document and rename the new wrapper type

2e0ef70

oli-obk force-pushed the cached_stable_hash_cleanups branch from 1b638b2 to 2e0ef70 Compare April 7, 2022 13:01

bjorn3 reviewed Apr 7, 2022

View reviewed changes

Avoid looking at the internals of Interned directly

25d6f8e

bors 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 Apr 8, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 9, 2022

bors merged commit e980c62 into rust-lang:master Apr 9, 2022

rustbot added this to the 1.62.0 milestone Apr 9, 2022

This was referenced Apr 9, 2022

Deterministic FxHashSet wrapper #94188

Closed

Fix stack overflows when compiling high-recursion_limit programs #93056

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cached stable hash cleanups #95524

Cached stable hash cleanups #95524

oli-obk commented Mar 31, 2022 •

edited

Loading

nnethercote commented Mar 31, 2022

rust-timer commented Mar 31, 2022

bors commented Mar 31, 2022

bors commented Mar 31, 2022

rust-timer commented Mar 31, 2022

rust-timer commented Mar 31, 2022

nnethercote commented Apr 7, 2022

bjorn3 Apr 7, 2022

bjorn3 commented Apr 7, 2022

oli-obk commented Apr 7, 2022

nnethercote commented Apr 7, 2022

oli-obk commented Apr 7, 2022

oli-obk commented Apr 7, 2022

bjorn3 Apr 7, 2022 •

edited

Loading

nnethercote commented Apr 7, 2022

bors commented Apr 7, 2022

oli-obk commented Apr 8, 2022

bors commented Apr 8, 2022

bors commented Apr 9, 2022

bors commented Apr 9, 2022

rust-timer commented Apr 9, 2022

Cached stable hash cleanups #95524

Cached stable hash cleanups #95524

Conversation

oli-obk commented Mar 31, 2022 • edited Loading

nnethercote commented Mar 31, 2022

rust-timer commented Mar 31, 2022

bors commented Mar 31, 2022

bors commented Mar 31, 2022

rust-timer commented Mar 31, 2022

rust-timer commented Mar 31, 2022

nnethercote commented Apr 7, 2022

bjorn3 Apr 7, 2022

Choose a reason for hiding this comment

bjorn3 commented Apr 7, 2022

oli-obk commented Apr 7, 2022

nnethercote commented Apr 7, 2022

oli-obk commented Apr 7, 2022

oli-obk commented Apr 7, 2022

bjorn3 Apr 7, 2022 • edited Loading

Choose a reason for hiding this comment

nnethercote commented Apr 7, 2022

bors commented Apr 7, 2022

oli-obk commented Apr 8, 2022

bors commented Apr 8, 2022

bors commented Apr 9, 2022

bors commented Apr 9, 2022

rust-timer commented Apr 9, 2022

Footnotes

oli-obk commented Mar 31, 2022 •

edited

Loading

bjorn3 Apr 7, 2022 •

edited

Loading