Avoid rehashing Fingerprint as a map key #76233

cuviper · 2020-09-02T01:33:58Z

This introduces a no-op Unhasher for map keys that are already hash-
like, for example Fingerprint and its wrapper DefPathHash. For these
we can directly produce the u64 hash for maps. The first use of this
is def_path_hash_to_def_id: Option<UnhashMap<DefPathHash, DefId>>.

cc #56308
r? @eddyb

This introduces a no-op `Unhasher` for map keys that are already hash- like, for example `Fingerprint` and its wrapper `DefPathHash`. For these we can directly produce the `u64` hash for maps. The first use of this is `def_path_hash_to_def_id: Option<UnhashMap<DefPathHash, DefId>>`.

Mark-Simulacrum · 2020-09-02T01:50:16Z

@bors try @rust-timer queue

rust-timer · 2020-09-02T01:50:17Z

Awaiting bors try build completion

bors · 2020-09-02T01:50:28Z

⌛ Trying commit 469ca37 with merge 780ecb71aa4946e12f8857c770a687286834e33d...

bors · 2020-09-02T02:32:26Z

☀️ Try build successful - checks-actions, checks-azure
Build commit: 780ecb71aa4946e12f8857c770a687286834e33d (780ecb71aa4946e12f8857c770a687286834e33d)

rust-timer · 2020-09-02T02:32:27Z

Queued 780ecb71aa4946e12f8857c770a687286834e33d with parent 130359c, future comparison URL.

rust-timer · 2020-09-02T03:48:20Z

Finished benchmarking try commit (780ecb71aa4946e12f8857c770a687286834e33d): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

jyn514 · 2020-09-02T03:52:50Z

-1% on hello world :) smaller decreases on the rest

Mark-Simulacrum · 2020-09-02T11:42:48Z

compiler/rustc_data_structures/src/fingerprint.rs

+    #[inline]
+    default fn write_fingerprint(&mut self, fingerprint: &Fingerprint) {
+        self.write_u64(fingerprint.0);
+        self.write_u64(fingerprint.1);


Did you try a panic! here instead to try and track down all the cases where we are hashing fingerprints?

I hadn't, but I tried just now and it ran into a StableHasher for the parent DefPathHash:

rust/compiler/rustc_hir/src/definitions.rs

Lines 111 to 117 in 130359c

fn compute_stable_hash(&self, parent_hash: DefPathHash) -> DefPathHash {

let mut hasher = StableHasher::new();

// We hash a `0u8` here to disambiguate between regular `DefPath` hashes,

// and the special "root_parent" below.

0u8.hash(&mut hasher);

parent_hash.hash(&mut hasher);

I suppose we could have that hash without the parent at first, and then Fingerprint::combine them for the final value. I'll give that a shot and see if anything else comes up.

The next one that comes up is item_ids_hash.hash_stable(...):

rust/compiler/rustc_middle/src/ich/impls_hir.rs

Lines 52 to 65 in 130359c

// Combining the `DefPathHash`s directly is faster than feeding them

// into the hasher. Because we use a commutative combine, we also don't

// have to sort the array.

let item_ids_hash = item_ids

.iter()

.map(|id| {

let (def_path_hash, local_id) = id.id.to_stable_hash_key(hcx);

debug_assert_eq!(local_id, hir::ItemLocalId::from_u32(0));

def_path_hash.0

})

.fold(Fingerprint::ZERO, |a, b| a.combine_commutative(b));

item_ids.len().hash_stable(hcx, hasher);

item_ids_hash.hash_stable(hcx, hasher);

I wonder if we could instead add a combine operation directly in the StableHasher?

Alternatively, we could specialize to allow Fingerprints to be used with StableHasher too, and unimplement the rest.

Hm, supporting combine directly with StableHasher seems like a good idea -- we presumably want that (or similar) a lot.

I'm not sure if combine is "as good" as hashing though, from a "hash quality" perspective. I would sort of assume no because then we wouldn't get any wins from using it...

Here's another hit:

rust/compiler/rustc_query_system/src/dep_graph/graph.rs

Line 891 in 130359c

node_to_node_index: Sharded<FxHashMap<DepNode<K>, DepNodeIndex>>,

... where DepNode<K> is a K and a Fingerprint.

I'm inclined to let all of these hash normally for now. Unhasher is already a bit hacky itself...

If we wanted though, DepNode<K> could ignore its K for hashing purposes.

It does sound like there's sort of a lot of potential here but the current Hash/Hasher API doesn't readily allow specializing like we want to.

I was looking at the Hasher API, and it feels like it might be worth adding something like a write_hash or similar, where the Hasher can expect the incoming value to already be "hashed" or at least nicely distributed across the range. To start we could just take a u64 since that's what Hasher::finish() returns, though e.g. for Fingerprint we really want u128. Maybe fn write_hash(impl Into<u128>) makes sense, not sure.

I am leaning towards saying that we should just merge this PR as-is: it seems like a clear, if small, win, and while there may be more hidden through careful hash-skipping it's probably better to evaluate each in a standalone manner, particularly given the relative complexity of the Unhasher design. We can consider other improvements, like the one I suggested in the previous paragraph, later.

Hm, supporting combine directly with StableHasher seems like a good idea -- we presumably want that (or similar) a lot.

FWIW, I was just wanting something like this for span's hash implementation, which currently re-hashes a hash of the file name.

Mark-Simulacrum · 2020-09-02T19:57:03Z

@cuviper -- unless you specifically want review from @eddyb, given #76233 (comment) sounds reasonable to you, r=me

cuviper · 2020-09-02T20:23:42Z

I'm OK with exploring others separately, and I had only chosen the reviewer based on #56308, so let's go!

@bors r=Mark-Simulacrum

bors · 2020-09-02T20:23:43Z

📌 Commit 469ca37 has been approved by Mark-Simulacrum

bors · 2020-09-02T22:16:32Z

⌛ Testing commit 469ca37 with merge 51f79b6...

bors · 2020-09-03T00:08:55Z

☀️ Test successful - checks-actions, checks-azure
Approved by: Mark-Simulacrum
Pushing 51f79b6 to master...

cuviper · 2020-09-03T19:49:03Z

@Mark-Simulacrum I got an idea to create new map/set types where the key controls its own hash value:
https://github.com/cuviper/autohash

I haven't published that yet because I still need to convert the tests and documentation (forked from hashbrown), but I'd love to hear any thoughts you have on that approach.

Mark-Simulacrum · 2020-09-03T20:28:05Z

I like that approach. I wish we didn't have to fork hashbrown though -- I suspect it might be avoidable, by using the raw tables similar to (I think) indexmap's approach, or even having a fake Hash/Hasher pair into the hashbrown HashMap/HashSet types directly -- I'd need to sit down and sketch it out in detail to explore that, and without delegation of some kind you still need to duplicate the whole API surface :/

cuviper · 2020-09-03T20:31:19Z

Sorry, that wasn't clear -- I forked hashbrown to get the starting implementation of HashMap, HashSet, and everything that goes with them, but I did make this use hashbrown::raw::RawTable as a dependency.

The fake Hash/Hasher pair is kind of what I did here with Unhasher, but I don't really feel like it's a great solution.

Mark-Simulacrum · 2020-09-03T20:58:17Z

Ah, okay, yeah -- in that case seems fine. The RawTable is where the really interesting bits are, after all, AFAIK.

I still feel like something along the lines of the write_hash idea from #76233 (comment) would be good to add to std, and then in the case of autohash the Hasher could always expect only a write_hash call. (You can obviously also emulate that with just a write_u64 call that's "known good").

I'm not sure that there's a great solution -- you kind of want trait Hash<H: Hasher> instead of trait Hash { fn hash<H: Hasher> } so that you could have less generic impls. But that ship has long sailed, and might've been a bad idea too, I'm not sure :).

rust-highfive assigned eddyb Sep 2, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 2, 2020

jyn514 added I-compiletime Issue: Problems and improvements with respect to compile times. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 2, 2020

Mark-Simulacrum reviewed Sep 2, 2020

View reviewed changes

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 Sep 2, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 3, 2020

bors merged commit 51f79b6 into rust-lang:master Sep 3, 2020

bors mentioned this pull request Sep 3, 2020

Implement lazy decoding of DefPathTable during incremental compilation #74967

Merged

cuviper mentioned this pull request Sep 15, 2020

Add more safe methods to RawTable rust-lang/hashbrown#202

Merged

cuviper deleted the unhasher branch September 21, 2021 16:44

cuviper added this to the 1.48.0 milestone Nov 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid rehashing Fingerprint as a map key #76233

Avoid rehashing Fingerprint as a map key #76233

cuviper commented Sep 2, 2020

Mark-Simulacrum commented Sep 2, 2020

rust-timer commented Sep 2, 2020

bors commented Sep 2, 2020

bors commented Sep 2, 2020

rust-timer commented Sep 2, 2020

rust-timer commented Sep 2, 2020

jyn514 commented Sep 2, 2020

Mark-Simulacrum Sep 2, 2020

cuviper Sep 2, 2020

cuviper Sep 2, 2020

cuviper Sep 2, 2020

Mark-Simulacrum Sep 2, 2020

cuviper Sep 2, 2020

cuviper Sep 2, 2020

Mark-Simulacrum Sep 2, 2020

tgnottingham Sep 2, 2020

Mark-Simulacrum commented Sep 2, 2020

cuviper commented Sep 2, 2020

bors commented Sep 2, 2020

bors commented Sep 2, 2020

bors commented Sep 3, 2020

cuviper commented Sep 3, 2020

Mark-Simulacrum commented Sep 3, 2020

cuviper commented Sep 3, 2020

Mark-Simulacrum commented Sep 3, 2020

	fn compute_stable_hash(&self, parent_hash: DefPathHash) -> DefPathHash {
	let mut hasher = StableHasher::new();

	// We hash a `0u8` here to disambiguate between regular `DefPath` hashes,
	// and the special "root_parent" below.
	0u8.hash(&mut hasher);
	parent_hash.hash(&mut hasher);

	// Combining the `DefPathHash`s directly is faster than feeding them
	// into the hasher. Because we use a commutative combine, we also don't
	// have to sort the array.
	let item_ids_hash = item_ids
	.iter()
	.map(\|id\| {
	let (def_path_hash, local_id) = id.id.to_stable_hash_key(hcx);
	debug_assert_eq!(local_id, hir::ItemLocalId::from_u32(0));
	def_path_hash.0
	})
	.fold(Fingerprint::ZERO, \|a, b\| a.combine_commutative(b));

	item_ids.len().hash_stable(hcx, hasher);
	item_ids_hash.hash_stable(hcx, hasher);

Avoid rehashing Fingerprint as a map key #76233

Avoid rehashing Fingerprint as a map key #76233

Conversation

cuviper commented Sep 2, 2020

Mark-Simulacrum commented Sep 2, 2020

rust-timer commented Sep 2, 2020

bors commented Sep 2, 2020

bors commented Sep 2, 2020

rust-timer commented Sep 2, 2020

rust-timer commented Sep 2, 2020

jyn514 commented Sep 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mark-Simulacrum commented Sep 2, 2020

cuviper commented Sep 2, 2020

bors commented Sep 2, 2020

bors commented Sep 2, 2020

bors commented Sep 3, 2020

cuviper commented Sep 3, 2020

Mark-Simulacrum commented Sep 3, 2020

cuviper commented Sep 3, 2020

Mark-Simulacrum commented Sep 3, 2020