robin-map icon indicating copy to clipboard operation
robin-map copied to clipboard

Added finalizer to default hash function.

Open stgatilov opened this issue 1 year ago • 2 comments

See https://github.com/Tessil/robin-map/issues/73

stgatilov avatar May 11 '24 14:05 stgatilov

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.

Tessil avatar May 26 '24 15:05 Tessil

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.

stgatilov avatar May 27 '24 17:05 stgatilov