Tessil / hat-trie

C++ implementation of a fast and memory efficient HAT-trie
MIT License
792 stars 114 forks source link

Users should use std::hash by default #17

Closed hungptit closed 5 years ago

hungptit commented 5 years ago

Hi Thibaut,

The recent implement of std::hash use CityHash for 64 bit platforms and will fallback to Murmur hash for 32 bit platforms. Could you update README.md to reflect the new change in std::hash?

IMHO users should use std::hash by default and try a better hash function if needed. From these benchmark results (https://github.com/hungptit/clhash/blob/convert.to.header.only.library/benchmark/results.svg) we can see that std::hash is pretty fast i.e about 0.5 CPU cycle per byte.

BTW, thank you very much for your work I have started to replace std::hash with hopscotch_map/set in all of my projects.

Regards, Hung

Tessil commented 5 years ago

Hello,

You mean in the Hash function section? I could effectively recommend the usage of std::hash<std::string_view> if the user has access to it (C++17) and fall-back to CityHash otherwise if it's what you mean.

Or is it replacing the default FNV-1a function with std::hash?

hungptit commented 5 years ago

Hi Thibaut,

Yes, I mean the "Hash function" section. I think you could use std::hash by default in your source code and recommend users to use std::hash by default and take a look at xxHash, farmhash, or clhash if the default hash function is not fast enough. BTW, you can find comparison graphs for the best hash functions here https://github.com/hungptit/clhash/blob/convert.to.header.only.library/benchmark/

Thanks, Hung

Tessil commented 5 years ago

Hi,

In the end I replaced the default FNV-1a hash function to std::hash<std::string_view> if std::string_view is detected. There was not much reason to use it as default hash function when C++17 or above was used.

Thanks, Thibaut