Add missing Color hash function #107289
Add missing Color hash function #107289
Conversation
There was a problem hiding this comment.
I can confirm this is a problem.
I ran across it in #106434, and fixed it in the same way there.
Unfortunately, that PR hasn't received reviews yet, so it might be a while till it's merged. Makes sense to merge the fix already.
| h = hash_murmur3_one_real(p_vec.w, h); | ||
| return hash_fmix32(h); | ||
| } | ||
| static _FORCE_INLINE_ uint32_t hash(const Color &p_vec) { |
There was a problem hiding this comment.
_FORCE_INLINE_ is not appropriate here.
I assume you got there since you copied it from other functions' signatures, but it also wasn't appropriate there either, just nobody fixed this yet :)
| static _FORCE_INLINE_ uint32_t hash(const Color &p_vec) { | |
| static uint32_t hash(const Color &p_vec) { |
There was a problem hiding this comment.
Given that every other method has it, I think it's better for code adding the missing one to be consistent.
If the _FORCE_INLINE_ is wrong in the other methods, it should be removed in a separate PR (and then this one can be removed too).
|
Thanks! |
The Color type has no hash function in HashMapHasherDefault, it still works right now but it only works because color gets implicitly converted to a String.
I had some code that was insanely slow for no good reason so I ran it through callgrind to figure out what was going on and I saw this in Kcachegrind:
40 billion instructions from String allocations when doing Color HashMap lookups didn't really make any sense, Color should have nothing to do with String. So I looked into the HashMap lookup function and sure enough there was no Color hash function, which caused keys of Color type to get implicitly converted to a String and then it called the String hash function.
Just by adding this Color hash function my startup time went from around 5 seconds to about half a second.