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

Clean up Hash Functions #62176

Merged
merged 1 commit into from
Jun 20, 2022
Merged

Conversation

reduz
Copy link
Member

@reduz reduz commented Jun 18, 2022

Clean up and do fixes to hash functions and newly introduced murmur3 hashes in #61934

  • Clean up usage of murmur3
  • Fixed usages of binary murmur3 on floats (this is invalid because -0/0 as well as inf must be taken into consideration).
  • Changed DJB2 to use xor (which seems to be better)

@reduz reduz marked this pull request as ready for review June 18, 2022 14:23
@reduz reduz requested review from a team as code owners June 18, 2022 14:23
@akien-mga akien-mga requested a review from Geometror June 18, 2022 14:50
@akien-mga akien-mga added this to the 4.0 milestone Jun 18, 2022
Copy link
Member

@Geometror Geometror left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me:)
I wonder if we could use murmur3 for string hashing too? Based on this actually pretty good stackexchange answer it should perform better than DBJ2, other benchmarks show similar results (though I am not sure how it behaves with short strings). During my experiments with different hash functions I tried to do that, but the engine crashed since it somehow broke ClassDB.

core/templates/hashfuncs.h Show resolved Hide resolved
modules/mono/managed_callable.cpp Outdated Show resolved Hide resolved
servers/rendering/renderer_rd/uniform_set_cache_rd.h Outdated Show resolved Hide resolved
@lmurray
Copy link

lmurray commented Jun 19, 2022

During my experiments with different hash functions I tried to do that, but the engine crashed since it somehow broke ClassDB.

StringName makes an assumption that all encodings of a string will result with the same hash, e.g. with a char * vs a char32_t *. This is only true with the current DJB2 algorithm with ASCII characters which all StringName strings currently are.

It's also worth noting that Murmur3A is slower than the DJB2 algorithm for strings under ~16 characters or so. I've been doing my own tests with hashes and have yet to find a good solution that performs better than the current version. I'm currently of the impression that just using a higher-quality minimal algorithm such as FNV-1a everywhere will end up being more performant than trying to use a heavier algorithm in certain circumstances due to being easier to inline.

@reduz
Copy link
Member Author

reduz commented Jun 19, 2022

@lmurray @Geometror To be honest I think for strings DJB2 does the job. The reason I moved most stuff to murmur3 is to improve the distribution in robin hood hash tables (which are super fast, but require in exchange much better hashing than what we had, or else the empty spaces get a poor distribution in big tables. This happens specially when using sequential elements, which DJB2 kind of sucks for). But for strings it should be fine, I don't see distribution being a problem in that scenario, so I doubt it makes much sense to change.

@reduz reduz force-pushed the cleanup-hashfuncs branch 3 times, most recently from d209722 to 31d7a64 Compare June 20, 2022 10:45
Clean up and do fixes to hash functions and newly introduced murmur3 hashes in godotengine#61934
* Clean up usage of murmur3
* Fixed usages of binary murmur3 on floats (this is invalid)
* Changed DJB2 to use xor (which seems to be better)
Copy link
Member

@Geometror Geometror left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be good to go, we can always change these if we find something which has a noticeable performance benefit over the combination of murmur3 and dbj2.

@akien-mga akien-mga merged commit 66a30da into godotengine:master Jun 20, 2022
@akien-mga
Copy link
Member

Thanks!

@Zireael07
Copy link
Contributor

Side note: are the new hash() functions exposed to the scripting? I need to hash a Vector3...

@Calinou
Copy link
Member

Calinou commented Sep 23, 2022

Side note: are the new hash() functions exposed to the scripting? I need to hash a Vector3...

It's not exposed for Vector2/3(i) specifically, but you can use the global scope method on any Variant type (which includes Vector 2/3(i)):

image

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

Successfully merging this pull request may close these issues.

None yet

6 participants