-
-
Notifications
You must be signed in to change notification settings - Fork 691
Make ClassDB::classes pointer-stable again by changing it to HashMap
#1887
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
Make ClassDB::classes pointer-stable again by changing it to HashMap
#1887
Conversation
72e3e79 to
c7ee662
Compare
c7ee662 to
788edc6
Compare
dsnopek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
|
Nice, thank you! |
|
I wasn't able to reproduce a crash with 35 classes (I suspect memory keeps being allocated inside the same page as the previous (printing old / outdated pointers for |
|
Might be redundant but I tested this in my project where AHashMap would crash previously. This commit however ran without any issues. Thank you very much! |
|
Cherry-picked for 4.4 in PR #1890 |
|
Cherry-picked for 4.5 in PR #1891 |
I'm not sure why I haven't thought of this problem when suggesting to use
AHashMapfor #1839, or noticed it on any of the issues that popped up. That was a blunder on my part. Sorry, folks!The problem materializes (in potentially various semi-related ways) as soon as at least ~13 classes are registered.
Explanation
ClassDB::classesholdsClassInfodensely. That means that on every resize, theClassInfostructs move in memory, which makes pointers to them illegal.@SiimonDev correctly identified this as a source of failure, and in #1886 fixes some issues by removing pointers to the
ClassInfostructs.However, I think it is expected that the
ClassInfostructs have stable locations in memory. Not being able to hold pointers to it could be a significant performance burden.Stable memory locations could be accomplished by either using
HashMap(see pointer stability in Core types), like Godot (https://github.com/godotengine/godot/blob/9dd6c4dbac70b28e8156255c3a2b78722772b036/core/object/class_db.h#L204), or by changing it to holdClassInfo *(like the current state of godotengine/godot#106646).I'm not fully convinced of the benefits of using
AHashMapwhen we're using allocations with values anyway, which is why I'm hesitant to merge godotengine/godot#106646 right now. So that's why Godot upstream is still usingHashMap.Long story short,
HashMapguarantees pointer stability, which should fix the problem.