Open akien-mga opened 3 months ago
akien-mga
commented
3 months ago I bisected the dev4 regression to #88162, CC @KoBeWi.
The dev6 regression is from #88306, which adds more PropertyListHelper uses. The EditorFileDialog orphan seems to be a bit older than #88306, I didn't bisect it separately.
Now bisecting the beta1 part.
KoBeWi
commented
3 months ago Sounds like another case of "leaks" reported as leaks, because they get freed too late. Each class that uses PropertyListHelper has a static instance which gets freed only once the program fully exits, which is likely after the leak detector. The only solution would be manually freeing static helpers, not sure where though.
akien-mga
commented
3 months ago Now bisecting the beta1 part.
Some of the beta1 "leaks" seem to be Engine singletons, I suspect this was caused by #92060, CC @raulsntos.
The last chunk of "leaks" seems to come from #92564, CC @Chaosus. These ones are easily fixable with:
diff --git a/servers/rendering/shader_language.cpp b/servers/rendering/shader_language.cpp
index 9aa54d0bb7..ad7ed07b41 100644
--- a/servers/rendering/shader_language.cpp
+++ b/servers/rendering/shader_language.cpp
@@ -10732,4 +10732,5 @@ ShaderLanguage::ShaderLanguage() {
ShaderLanguage::~ShaderLanguage() {
clear();
+ global_func_set.clear();
}(could also go in the clear() method, but it's called several times and I think set only needs to be populated in the constructor and removed in the destructor)
Chaosus
commented
2 months ago This one exist since dev1 build:
If someone wants to help to hunt this bug down to close this issue. It's related with editor nodes since it doesn't happen at exit of Project Manager.
vms-code
commented
1 week ago @akien-mga @Chaosus @KoBeWi I solved this problem but I'm using a 4.3.dev version that is before the merge that solved the other orphan StringNames, for the other StringNames the issue was on PropertyInfo class_name, so I changed it to a String instead of StringName and it solved, I believe it was happening whenever a class used a static PropertyListHelper to register properties. For the remaining orphan StringName (Node) the problem comes from the following places:
// editor/editor_node.cpp line 4570;
ClassDB::is_parent_class(p_class, SNAME("Node"))
// edior/scene_create_dialog.cpp line 55:
node_type_other->add_theme_icon_override(SNAME("icon"), get_editor_theme_icon(SNAME("Node")));
// editor/scene_tree_dock.cpp -> line 1506:
filter_menu->set_item_icon(filter_menu->get_item_index(FILTER_BY_TYPE), get_editor_theme_icon(SNAME("Node")));
// modules/multiplayer/editor/editor_network_profiler.cpp line 66:
theme_cache.node_icon = get_theme_icon(SNAME("Node"), EditorStringName(EditorIcons));
// editor/create_dialog.cpp line 505:
String text = help_bit->get_class_description(p_type);for the places using SNAME, removing SNAME solves the issue (but not sure if it's a good solution), for the create_dialog.cpp line 505 situation I believe is happening because the StringName is placed on a static HashMap and the map is not cleaned, but I still haven't tried to solve yet...
I think the problem happens whenever StringName is initialized using static because it lives longer then the StringName::cleanup function call point (as it was suggested on @KoBeWi reply), but I still don't understand why it happens on the places using SNAME since there are several other places that also use SNAME and don't have a problem, the only explanation that makes sense to me now is that on this specific cases the StringName is also used somewhere else so it is referenced one extra time and because it is static as a byproduct of the SNAME macro the extra reference is not dereferenced correctly, does anyone know what is going on? Also, how does SNAME (static StringName) gets automatically dereferenced/deleted on other situations? Is it because of the SafeNumeric functionality? Would appreciate any insights or direction on resources to learn more about this... I'm going to spend more time trying to figure it out as well and maybe post an update if I believe I understood what is happening.
vms-code
commented
5 days ago I understand what is happening now, the problem is that the engine never cleans static StringNames, the reason why only those instances trigger the warning message is because of the condition:
if (d->static_count.get() != d->refcount.get())and the situation that triggers the warning is the only one where there is a difference between refcount and static_count, by cleaning up the static HashMap on the situation where the StringName is not static the warning goes away:
~EditorHelpBit() {
doc_class_cache.clear();
};but this still doesn't clean the static StringNames, so I'm not sure about a good solution yet but one I'm trying to implement is to have a static Vector
if (d->refcount.get()) {
...
}instead
Tested versions
System information
Fedora Linux 40 (KDE Plasma) - Wayland - Vulkan (Forward+) - dedicated AMD Radeon RX 7600M XT (RADV NAVI33) () - AMD Ryzen 7 7840HS w/ Radeon 780M Graphics (16 Threads)
Issue description
When closing the project manager or the editor with
--verbosemode, I get 174 orphan StringNames reported:Most of this seems to have been introduced between dev 6 and beta 1.
In previous snapshots, there's a subset of those that were reported:
In 4.3.dev6:
And in 4.3.dev4 / 4.3.dev5:
Steps to reproduce
godot -v, close it and check terminalMinimal reproduction project (MRP)
n/a