Skip to content
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

Merged

Conversation

YuriSizov
Copy link
Contributor

@YuriSizov YuriSizov commented Oct 18, 2023

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

StringName: 1255 unclaimed string names at exit.

EditorColorMap changes

StringName: 1219 unclaimed string names at exit.

GDExtension changes

StringName: 1070 unclaimed string names at exit.

GDExtensionCompatHashes changes

StringName: 526 unclaimed string names at exit.

CoreConstants changes

StringName: 4 unclaimed string names at exit.

EditorTheme changes

😊


Some of this should be backported to 4.1, but I don't think this would be cherry-pickable.

@YuriSizov YuriSizov added this to the 4.2 milestone Oct 18, 2023
@YuriSizov YuriSizov requested review from dsnopek and vnen October 18, 2023 15:42
@YuriSizov YuriSizov requested review from a team as code owners October 18, 2023 15:42
Copy link
Contributor

@dsnopek dsnopek left a 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!

Copy link
Member

@akien-mga akien-mga left a 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.

@YuriSizov
Copy link
Contributor Author

YuriSizov commented Oct 18, 2023

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.

@KoBeWi
Copy link
Member

KoBeWi commented Oct 18, 2023

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 StringName::cleanup() runs before that and reports false-positives. The reported StringNames aren't leaked, they just get destroyed too late.
Also not sure how exactly a StringName can leak if they are basically ref counted.

@akien-mga akien-mga merged commit 2714a73 into godotengine:master Oct 18, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants