-
Notifications
You must be signed in to change notification settings - Fork 151
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
Hash keys once #194
Hash keys once #194
Conversation
Used in the following commit to avoid hashing key twice.
Hashbrown `HashMap` provides API to work with prehashed keys.
@@ -43,7 +43,7 @@ cfg_if! { | |||
} | |||
} | |||
|
|||
pub(crate) type HashMap<K, V, S> = std::collections::HashMap<K, SharedValue<V>, S>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This type alias is exposed by public facing apis and therefor is a semver breakage, this was experienced in using one of the rust-analyzer crates as a dependency rust-lang/rust-analyzer#12344 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Veykril Hey! This is indeed a semver breakage, however this is only visible if you use the raw_api
feature which exposes the raw shards. I should probably do a better job at noting this in crate level documentation but this feature is unstable, this means you need to pin the versions if you rely on the types from that API as it may change due to internal needs like this PR and an internal lock change that was also done recently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, good to know!
AFACT this got reverted as part of another bug fix, but I guess can be re-added? |
Two commits:
raw_entry
API to avoid hashing keys twiceHashing twice issue can be solved by storing
(K, hash)
insteadK
in inner hashmap and supplying custom hash function.With hashbrown raw entry API it is also possible to implement
Equivalent
API #183.The downside of this diff is that
shards
function now exposes hashbrown maps instead of std maps (switching to(K, hash)
has the same issue) .The alternative to this diff is to wait for stabilization of rust-lang/rust#56167, as pointed in #183.