Skip to content
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

Issue #225: Specialization fix #232

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tkaitchuck
Copy link
Contributor

This fixes #225
It uses the key type as the type specialization even when lookup occurs with a different type.

Signed-off-by: Tom Kaitchuck <Tom.Kaitchuck@gmail.com>
Signed-off-by: Tom Kaitchuck <Tom.Kaitchuck@gmail.com>
Signed-off-by: Tom Kaitchuck <Tom.Kaitchuck@gmail.com>
@Amanieu
Copy link
Member

Amanieu commented Jan 26, 2021

CI is still failing on the test_into_iter_refresh test, could you look into it?

@Amanieu
Copy link
Member

Amanieu commented Jan 27, 2021

It seems that any scheme in which hashbrown calculates the hash in a non-standard way depending on the type can't work. This is because we expose raw hashes as part of the RawEntry and RawTable APIs, where the user will pass hashes calculated using the standard method of Hash::hash.

As I have said before, I think the only way to support "short" hashing reliably is through changes to the standard library. This should be done through changes to the Hash and Hasher traits, as well as the Hash derive macro.

@tkaitchuck
Copy link
Contributor Author

tkaitchuck commented Jan 27, 2021

This is because we expose raw hashes as part of the RawEntry and RawTable APIs, where the user will pass hashes calculated using the standard method of Hash::hash

That strikes me as both unergonomic and limiting. It's not just the hasher implementation that restricts, but it also prevents the user from doing anything fancier like a specialized algorithm for their type. It would be a shame if RawEntrys forever prevented specialization, as it would impact all maps' performance when that API is not used.

@Amanieu What sort of changes could accomplish this? I don't think the changes I am doing in specialization could just be ported as-is to the default implementation. That would change for example the numeric hash value of a hashed slice with existing hashers. I assume that would be unacceptable.

Perhaps the invocation path I am using of calling get_hash on something implementing Hash could be standardized. But that doesn't solve the problem that the user is expected to know how the HashMap invokes something internally and do it the same way.

@tkaitchuck
Copy link
Contributor Author

On thought: The trait I am using do do the dispatch is here:
https://github.com/tkaitchuck/aHash/blob/master/src/lib.rs#L120
It is defined for all hasher types so that it can work seamlessly with other hashers. One solution might be to pull this trait and the default impl into HashBrown.

@bors
Copy link
Contributor

bors commented Jan 27, 2021

☔ The latest upstream changes (presumably #233) made this pull request unmergeable. Please resolve the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possible bug in v0.10 with lookups on Box<[u8]>
3 participants