-
-
Notifications
You must be signed in to change notification settings - Fork 21.5k
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
Fix StringName leaks in GDExtension, core, and editor themes #83562
Fix StringName leaks in GDExtension, core, and editor themes #83562
Conversation
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.
The GDExtension changes look good to me!
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.
Tested locally, seems to work well!
On a test project with a 3D scene and a few imported .glbs, closing the editor reports:
StringName: 1291 unclaimed string names at exit.
After the PR, it's down to 38 string names, which seem to be Variant types:
Orphan StringName: Vector2
Orphan StringName: Vector3
Orphan StringName: Vector4
Orphan StringName: RID
Orphan StringName: Color
Orphan StringName: PackedByteArray
Orphan StringName: Vector2i
Orphan StringName: Callable
Orphan StringName: Vector3i
Orphan StringName: Vector4i
Orphan StringName: PackedStringArray
Orphan StringName: Basis
Orphan StringName: int
Orphan StringName: PackedFloat32Array
Orphan StringName: Plane
Orphan StringName: PackedFloat64Array
Orphan StringName: NodePath
Orphan StringName: Signal
Orphan StringName: StringName
Orphan StringName: String
Orphan StringName: Transform2D
Orphan StringName: Transform3D
Orphan StringName: Array
Orphan StringName: Quaternion
Orphan StringName: bool
Orphan StringName: PackedColorArray
Orphan StringName: Node
Orphan StringName: PackedVector2Array
Orphan StringName: Object
Orphan StringName: AABB
Orphan StringName: float
Orphan StringName: Dictionary
Orphan StringName: Projection
Orphan StringName: Rect2
Orphan StringName: PackedVector3Array
Orphan StringName: PackedInt32Array
Orphan StringName: Rect2i
Orphan StringName: PackedInt64Array
StringName: 38 unclaimed string names at exit.
That's already a big win so good to merge IMO, but if you want to look into the last ones in this PR, feel free to.
I can confirm that variant type names are being reported when you open a project in the editor. This may be hard to pinpoint, since it's too common, but I will try to take a look. Feel free to merge it at your convenience too, this can be a follow-up! For the record, @KoBeWi suspects that these reports may be bogus and that strings don't actually leak, we just do the checks too early. I can't really confirm or deny that with my knowledge of core internals. Nonetheless, this seems like a good indication for missing housekeeping. |
Yeah, from what I understand, StringName data is heap-allocated, like Nodes. Thus when exiting the application, "unclaimed" StringNames are potential memory leaks. In case of static sets, they are destroyed when the program exists, but apparently the |
Thanks! |
Closes #69195. Closes #73554.
We are currently leaking 1255 string names (tested by launching the project manager with a verbose flag). All of these occurrences are related to the use of static hashmaps, which do not clear themselves and so don't properly free their references string names. Issues related to my own changes in the editor theme code, to various changes in GDExtension (cc @dsnopek), and to changes in global enums (cc @vnen #73590).
The proposed solution in the issue is a bit sus, as it creates copies of string names, which somehow circumvents the issue. But it doesn't appear to the a proper solution. Instead I added the necessary code to clear all relevant hashmaps when objects should be finalized/deinitialized/destroyed.
I'm not sure if #74031 should be redone in a similar fashion, but that one might be correct as is, since it's about one singular static StringName.
A bit of stats :)
Base
EditorColorMap changes
GDExtension changes
GDExtensionCompatHashes changes
CoreConstants changes
EditorTheme changes
😊
Some of this should be backported to 4.1, but I don't think this would be cherry-pickable.