search

amethyst / shred

Shared resource dispatcher
Apache License 2.0
233 stars 66 forks source link

Use ahash instead of SipHash #220

Closed torkleyy closed 2 years ago

torkleyy commented 2 years ago

Partially reverts 496564035390f085f677ed4e37852b1a5b4854cd (we use ahash again, but without using hashbrown which is just a copy of std::collections::HashMap with ahash as the default).

torkleyy commented 2 years ago

cc @xMAC94x

This gets us back to the speed before I removed hashbrown in the last PR, but without the unnecessary dependency on hashbrown.

xMAC94x commented 2 years ago

Any reason to choose ahash over fxhash? https://nnethercote.github.io/perf-book/hashing.html

I am not familar with the wide range, but let the veloren community know. there are quite some good experts in there that prob know the best hash for a crate like shred :)

torkleyy commented 2 years ago

I figured the 1-4% improvement wouldn't be worth an implementation that's not as widely applicable and apparently it has some other pitfalls.

xMAC94x commented 2 years ago

mhhh okay, do we have any benchmarks or how did we compare the performance regarding to hasbrown ?

torkleyy commented 2 years ago

The benchmarks are linked in your comment:

An attempt to switch from fxhash to ahash resulted in slowdowns of 1-4%.

I think benchmarks in shred for this would be great as well, but for now, I just want to revert the accidental performance regression. Once we have such benchmarks we can also consider other hashers if they show beneficial.