Replace unordered_map with AHashMap to improve build time #1839
Conversation
There was a problem hiding this comment.
Those look like some pretty great gains for compile time!
I think I concur with the general idea; if we don't need unordered_map in Godot, we probably don't need it in godot-cpp either. Especially when we can save so much compile time.
However, I think we should use AHashMap instead of HashMap for this. It should be quite a lot faster for lookups (and changes). I think it's not been transferred to godot-cpp yet, so this might be a good opportunity to do so.
Sounds good I totally agree. I use AHashMap all over in my module code that I'm converting. So I'm going to need it in the future coming up here anyway. I'll add it to this PR and make some tweaks. It should be pretty easy, I probably just have to copy and paste it and then change a few words. |
I'm not sure how we started using the STL containers in |
I found some uses when doing #1841 where the allocation function from Godot isn't possible to use yet since it's being called during static initialization. I assume there just used to be some lifetime issues that prevented using Godot templates that are (mostly) resolved now. The one legit use of STL containers now is extremely jank too and it'd be nice to rework it to be more sane imo but I have no idea how to go about doing something like that. Internal classes get registered in a macro that calls another macro by the constructor of an unused inline static variable. Then they get unregistered in the destructor of that unused static variable. Of all the ways to call a function in C++ this might be one of the least sane ways I can think of haha. |
Calinou
added
enhancement
|
@Ivorforce I'm running into a problem when converting AHashMap over. There is no Memory::alloc_static_zeroed in godot-cpp and there doesn't seem to be a way to make one on the godot-cpp side because there is no Presumably the whole point of calling I tried just changing the |
Right, i forgot about this. // instead of void *mem = Memory::alloc_static_zeroed(size);
void *mem = Memory::alloc_static(size);
memset(mem, 0, size); |
dementive
force-pushed
the
improve-build-time
branch
2 times, most recently
from
875c3bd to
f8f964f
Compare
dementive
changed the title
|
memset fixed it. Everything should be good to go with AHashMap now. |
There was a problem hiding this comment.
Looks good to me.
There is some risk that the mutation pointer instability of AHashMap could cause issues, but I looked over the code and didn't see any that jumped out to me as risky. I also don't know the exact guarantees of std::unordered_map.
If we do find regressions in this regard, we can switch to HashMap for singular cases.
| AHashMap<StringName, Object *>::ConstIterator i = engine_singletons.find(p_class_name); | ||
| if (i != engine_singletons.end()) { | ||
| ERR_FAIL_COND((*i).second != p_singleton); | ||
| ERR_FAIL_COND((*i).value != p_singleton); |
There was a problem hiding this comment.
AHashMap<StringName, Object *>::ConstIterator singleton_it = engine_singletons.find(p_class_name);
if (singleton_it != engine_singletons.end()) {
ERR_FAIL_COND((*singleton_it).value != p_singleton);To match the rest of the file, and for readability. i usually used for integer iterators.
std::unordered_map is used all over in the ClassDB but there doesn't seem to be any good reason for it, internally godot would normally use HashMap for something like this.
I tested the compile time of the test project with ClangBuildAnalyzer and got these results showing that almost all of the slowest templates to instantiate were unordered_map, even though it is only ever use in the ClassDB. These are the full results I got:
first.txt
The relevant parts that showed me that unorderd_map was insanely slow for compile times were here:
Almost all of the slowest to instantiate templates are unordered_map or some std hashing stuff.
Not only is it crazy slow to compile but some benchmarks show that it is also pretty slow at runtime compared to other hash map implementations due to it's constraints: https://github.com/boostorg/boost_unordered_benchmarks/tree/boost_unordered_aggregate.
After changing unordered_map to HashMap in ClassDB and then rerunning ClangBuildAnalyzer I got these results:
second.txt
The parts showing compile time differences in using HashMap vs unordered_map were here:
This shows the compile times increased by 40% and a lot less of the most expensive to instantiate templates are now HashMap compared to how many were unordered_map before. The 40% isn't the clock time though, that is the time ClangBuildAnalzyer reports by adding the time from every file individual file but the compiler runs a lot of that in parallel so the real time difference is lower. The clock time (on my computer) of compiling the tests was 128.86 originally and with HashMap it is 114.34, so only about 12% faster in actual time.
The results also show that now a lot of the slowest to instantiate templates are from
std::vectorandstd::stringso in a future PR I think I'll look into moving some of those over to the GodotLocalVectorandString/StringNameequivalents.