You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Avoid hashing more than is strictly necessary, in the compiler. #56308
@Zoxc That is, change the implementation of the HashSetInterner trait to use the new APIs?
That seems good, and I'd even call the abstraction HashingInterner, InternByHash or something similar (since the "set" aspect is less relevant than the fact that this is based on hashing).
Oh, another place where we could take advantage of computing the hash only once is the query engine, in case we had to compute a query, and we're inserting the result.
Another note, not directly related to interners: We have some things in the compiler that already contain high-quality hash values of themselves. E.g. it's unnecessary work to hash a DefPathHash. There's at least one such map here:
@michaelwoerister I think we can maybe use a wrapper for the hash state that uses specialization to avoid double-hashing, in that case, instead of relying on raw_entry.
@michaelwoerister Yes but it would require making an entire wrapper of the HashMap public API which carefully avoids letting anything hash the key (going as far as wrapping the key in a type that panics in its Hashimpl).
I ran some analysis to find the maps changed in #120076 by looking for things put into HashMaps that looked like they were already hashes, using a few heuristics (e.g., similar number of 1 and 0 bits). I think that found most or all of the problem places. I don't see a good way to assert we don't regress there -- with a TypeId for non 'static things, we could assert in Hash64/Hash128 etc impls that we're not hashing them with FxHash (or that we're only hashing them with Unhash). But that assert doesn't exist today, and I don't think adding e.g. specialization or a doc(hidden) method on Hasher in std makes sense for this at this time.
I think the next big opportunity for reducing hashing would be maps with dense, relatively small integer valued keys, where we actually don't need to hash because they're sufficiently dense that a vec makes more sense. We saw big wins with #119977 from both (a) removing the hashing and (b) improved cache locality due to keeping things closer together. But those typically aren't as easy of a transformation to make, and to some extent are a time/space tradeoff.
Activity
Zoxc commentedon Nov 28, 2018
I have a branch which adds functions for interning based on this PR. I suggest that further work be based on that.
cc @nnethercote
Also relevant #55514 #56241
eddyb commentedon Nov 28, 2018
@Zoxc That is, change the implementation of the
HashSetInternertrait to use the new APIs?That seems good, and I'd even call the abstraction
HashingInterner,InternByHashor something similar (since the "set" aspect is less relevant than the fact that this is based on hashing).Zoxc commentedon Nov 28, 2018
Sure. I think we have to change things to use a
HashMapanyway.HashSethides all the fun bits.eddyb commentedon Nov 28, 2018
Oh, another place where we could take advantage of computing the hash only once is the query engine, in case we had to compute a query, and we're inserting the result.
michaelwoerister commentedon Nov 29, 2018
Another note, not directly related to interners: We have some things in the compiler that already contain high-quality hash values of themselves. E.g. it's unnecessary work to hash a
DefPathHash. There's at least one such map here:rust/src/librustc/ty/context.rs
Line 919 in 691a7f8
There are probably a few additional, similar cases.
eddyb commentedon Nov 29, 2018
@michaelwoerister I think we can maybe use a wrapper for the hash state that uses specialization to avoid double-hashing, in that case, instead of relying on
raw_entry.michaelwoerister commentedon Nov 29, 2018
I think the raw_entry API would be the nicer way to do it. It seems to be tailored to support a case just like this (among other use cases).
eddyb commentedon Nov 29, 2018
@michaelwoerister Yes but it would require making an entire wrapper of the
HashMappublic API which carefully avoids letting anything hash the key (going as far as wrapping the key in a type that panics in itsHashimpl).michaelwoerister commentedon Nov 29, 2018
What would your solution look like?
Rollup merge of rust-lang#56324 - Zoxc:int-ext, r=nikomatsakis
1 remaining item
eddyb commentedon Dec 2, 2018
I'll reopen this because the discussion mentioned things other than interners.
Also, I still have to write those code examples for @michaelwoerister.
Noratrieb commentedon May 10, 2023
The interners currently do use the raw entry APIs:
rust/compiler/rustc_data_structures/src/sharded.rs
Line 99 in cba1407
oli-obk commentedon May 11, 2023
We should still look for other uses of hash maps with keys that are already hashes
Auto merge of rust-lang#120076 - Mark-Simulacrum:unhash, r=<try>
Mark-Simulacrum commentedon Jan 18, 2024
I ran some analysis to find the maps changed in #120076 by looking for things put into HashMaps that looked like they were already hashes, using a few heuristics (e.g., similar number of 1 and 0 bits). I think that found most or all of the problem places. I don't see a good way to assert we don't regress there -- with a TypeId for non 'static things, we could assert in Hash64/Hash128 etc impls that we're not hashing them with FxHash (or that we're only hashing them with Unhash). But that assert doesn't exist today, and I don't think adding e.g. specialization or a doc(hidden) method on Hasher in std makes sense for this at this time.
I think the next big opportunity for reducing hashing would be maps with dense, relatively small integer valued keys, where we actually don't need to hash because they're sufficiently dense that a vec makes more sense. We saw big wins with #119977 from both (a) removing the hashing and (b) improved cache locality due to keeping things closer together. But those typically aren't as easy of a transformation to make, and to some extent are a time/space tradeoff.
Auto merge of rust-lang#120078 - Mark-Simulacrum:skip-hashers, r=<try>
Auto merge of rust-lang#120076 - Mark-Simulacrum:unhash, r=cjgillot
Auto merge of rust-lang#120078 - Mark-Simulacrum:skip-hashers, r=<try>
Auto merge of #120076 - Mark-Simulacrum:unhash, r=cjgillot
Auto merge of #120076 - Mark-Simulacrum:unhash, r=cjgillot