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

Added finalizer to default hash function. #78

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

Conversation

stgatilov
Copy link

See #73

…unction on uint64_t.

It constructs a set of distinct keys with zero lower half, then computes hash function for all of them, and checks that the hashes are mostly different in lower bits.
@Tessil
Copy link
Owner

Tessil commented May 26, 2024

Thanks for the contribution.

The change looks good but I'm a bit worried about the backward compatibility and the risk of changing the performance of existing users. We're applying the finalizer by default no matter the type of T, the std lib or the GrowthPolicy.

I wonder if we should instead provide the tsl::hash as utility but keep the default class Hash = std::hash<Key> and put a note in the README.

@stgatilov
Copy link
Author

Finalizer is not applied on MSVC, i.e. it is only applied on libstd-c++ and libc++.
It is maybe OK to ignore it with prime size policy: I can add this tweak if you want.

In my opinion, if you don't enable finalizer by default and keep default size policy as "power of two", then it is incorrect to say that tsl map is drop-in replacement for std::unordered_map (or other hash map implementations) and that no expertise is required to use it. And you should certainly static_assert against using pointers as keys then, because this is the case where this issue almost always happens.

If you have some specific benchmarks in mind, I can run them.
There should be little difference on large hash tables (since they are memory-limited).
And if the results of some benchmark changes a lot, then I would argue that the previous results are a lie in a sense. Reliably spreading elements across table to provide O(1) average time complexity is the main feature of hash table. It is not fair to compare an implementation which does not have this feature with implementations which do.

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.

2 participants