Skip to content

Conversation

Ivorforce
Copy link
Member

@Ivorforce Ivorforce commented May 15, 2025

Godot is plagued by long compile times because of unnecessary cross project includes.
The main culprits are hashfuncs.h and variant.h.
This PR eliminates cross project includes from hashfuncs.h, to improve compile time and reduce type coupling.

Explanation

HashMapHasherDefault was previously written by implementing a static uint32_t hash(const T &) function for every known Variant type. As Variant types grew, so did the includes for hashfuncs.h.

I rewrote the struct by adding a type trait called HashMapHasherDefaultImpl<T>. This type trait can be declared for any type, providing a hash function for default hashing, analogous to HashMapComparatorDefault.

The type trait is specialized for primitives (like double and int32_t).
It is further specialized for enum types by using the underlying type, which was also the previous behavior (through implicit conversion).

It is also specialized for types that declare uint32_t hash() const;, by calling this function. This behavior was previously implemented with HashableHasher, whose implementation claimed it is only possible to generalize this with c++20 concepts. I removed the now unnecessary HashableHasher type.
Using this specialization, hash functions are moved to the appropriate types. By this move, the includes from hashfuncs.h could be removed.

The same trick was applied to HashMapDefaultComparator, which previously had unnecessary specializations that all called is_same. I generalized this assumption with type trait logic (SFINAE).

Side effects

  • Color was previously hashed by converting to String and hashing the string (through implicit conversion). This was slow. I optimized the conversion to hash with the components instead.
  • hash_map previously needed a Variant include because it declared a function that assumed Variant keys. I generalized the function to use a comparator, and moved the variant comparison method to dictionary.cpp (the only place where it was used). This allowed me to delete hash_map.cpp.
  • It is no longer possible to "auto-declare" hashing functions by declaring an implicit conversion operator. This is sane, especially since we have many lossy and slow implicit conversion operators in the codebase (as seen on Color). Two types now need to define an explicit hashing function that did not need to declare it before.
  • It is now possible to hash enum struct, which was not possible before. Previously, the enum needed to be implicitly convertible to its underlying type, to support being hashed by HashMapHasherDefault.
  • I had a problem with ImageLoaderSVG remotely, to debug it I inserted the error code to the error message. It seems to have mysteriously disappeared, but I'd argue having the error code for the future is good anyway.

@Ivorforce Ivorforce added this to the 4.5 milestone May 15, 2025
@Ivorforce Ivorforce requested review from a team as code owners May 15, 2025 10:02
@Ivorforce Ivorforce removed request for a team May 15, 2025 10:02
@Ivorforce Ivorforce force-pushed the invert-hashfuncs branch 2 times, most recently from dc37fe6 to b2a441d Compare May 15, 2025 11:56
@Ivorforce Ivorforce requested a review from a team as a code owner May 15, 2025 11:56
@YYF233333
Copy link
Contributor

YYF233333 commented May 15, 2025

I've done some experiments and unfortunately this PR alone doesn't reduce build time, because all the headers removed from hashfuncs.h are still included in variant.h, and hashfuncs.h is almost always indirectly included via variant.h. So in practice, the number of headers processed by each translation unit hasn't decreased (But anyway, this is a good start ❤️). I tried cleaning up variant.h, but ran into some tricky issues (mostly caused by Callable). I think this is a good opportunity to revisit #105254, #105231, and #104627, as those PRs should help address some of these problems.

@Ivorforce
Copy link
Member Author

I've done some experiments and unfortunately this PR alone doesn't reduce build time, because all the headers removed from hashfuncs.h are still included in variant.h, and hashfuncs.h is almost always indirectly included via variant.h. So in practice, the number of headers processed by each translation unit hasn't decreased. I tried cleaning up variant.h, but ran into some tricky issues (mostly caused by Callable). I think this is a good opportunity to revisit #105254, #105231, and #104627, as those PRs should help address some of these problems.

Thanks for checking! I was guessing as much, for the same reasons you mentioned; almost everything ends up including variant.h anyway. Still, we'll need to resolve includes in both headers (variant.h and hashfuncs.h) to finally untangle this mess, so we might as well move forwards with this one now.

…ble to declare a default hashing function for any type.

Remove cross-project includes from `hashfuncs.h`.
Improve hashing function for `Color` (based on values instead of `String`).
Move `Variant` comparison from `hash_map.h` to `dictionary.cpp` (`VariantComparatorDictionary`), where it's used.
Remove now unnecessary `HashableHasher`.
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.

4 participants