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

Swap ahash with foldhash #1294

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

IvanIsCoding
Copy link
Collaborator

This matches hashbrown's 0.15 release that switches the default hasher. This is more to keep consistency with the internals.

I will put this on hold until #1293 is merged, we are still stuck with an older version of hashbrown until we update PyO3 anyway

@IvanIsCoding
Copy link
Collaborator Author

This fails MSRV, I guess hashbrown will be stuck as well

@jamestwebber
Copy link
Contributor

I used to use hashbrown for my own code but stopped when I noticed this on their github repo:

Since Rust 1.36, this is now the HashMap implementation for the Rust standard library. However you may still want to use this crate instead since it works in environments without std, such as embedded systems and kernels.

Maybe there have been additional features in hashbrown that make it worth using now but I'm curious is the difference is measurable.

@IvanIsCoding
Copy link
Collaborator Author

I used to use hashbrown for my own code but stopped when I noticed this on their github repo:

Since Rust 1.36, this is now the HashMap implementation for the Rust standard library. However you may still want to use this crate instead since it works in environments without std, such as embedded systems and kernels.

Maybe there have been additional features in hashbrown that make it worth using now but I'm curious is the difference is measurable.

That is a fair point. It is not strictly necessary, but I feel hashbrown is always trying to push a little bit above the standard library in terms of API and performance.

Also, it is a transitive dependency of indexmap so the crate is already in the dependencies anyway. We might as well benefit from some performance improvements like the one from foldhash

@mtreinish
Copy link
Member

Maybe there have been additional features in hashbrown that make it worth using now but I'm curious is the difference is measurable.

The key reasons we use hashbrown on rustworkx is the default hasher is higher performance, it actually is surprising how much a difference using ahash made here when I did the benchmarking for it many years ago (I hadn't heard of foldhash before this PR though so I can't comment on that). We could have gotten this by directly using ahash on the stdlib hashmap though (or ahash's mapping type). The other key reason though was rayon support, we don't use it extensively but if we need a parallel iterator over a hashmap or hashset hashbrown has a rayon feature we can use.

@jamestwebber
Copy link
Contributor

it actually is surprising how much a difference using ahash made here when I did the benchmarking for it many years ago

Yeah I was curious if the difference is still there, now that the stdlib uses the same implementation. But the transitive dependency due to indexmap makes it a moot point!

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

Successfully merging this pull request may close these issues.

3 participants