Bug Report:Memory leak with passing lua functions to GDScript #155

RadiantUwU · 2023-09-08T17:02:02Z

Describe the bug
This bug happens by creating constant references and not being able to know when the reference is removed or not.

To Reproduce

extends Node

func do_something_with_callable(callable: Callable):
	pass

# Called when the node enters the scene tree for the first time.
func _ready():
	var api = LuaAPI.new()
	api.push_variant("do_something_with_callable",do_something_with_callable)
	api.do_string("""
	while true do
		do_something_with_callable(function() end)
	end
	""")

Expected behavior
The memory usage should remain stable, not constantly rising.

Environment (please complete the following information):

OS: Linux 6.4.12-arch1-1
Godot version: v4.1.1.stable.official [bd6af8e0e]
Module version: v2.1-beta4

Additional context
Add any other context about the problem here.

Very quickly rising memory usages from 200 MB to 500MB within 5 seconds, potentially more if left longer.

Trey2k · 2023-09-08T17:05:03Z

So this is a known memory leak, the issue is without the CallableCustom type we have no way to have a deconstructor tied to the callable, meaning a copy of the lua function remains on the state.

This should hopefully be resolved once godotengine/godot#79005 is merged, and we can use the CallableCustom type.

RadiantUwU · 2023-09-08T17:06:34Z

RadiantUwU · 2023-09-08T17:07:47Z

So this is a known memory leak, the issue is without the CallableCustom type we have no way to have a deconstructor tied to the callable, meaning a copy of the lua function remains on the state.

This should hopefully me resolved once godotengine/godot#79005 is merged, and we can use the CallableCustom type.

Make a new refcounted and pass it as the instance for creating the callable, give it the reference and make it so when the refcounted disappears it clears it from the registry.

RadiantUwU · 2023-09-08T17:08:03Z

Should i try doing a pull request...?

Trey2k · 2023-09-08T17:09:46Z

Make a new refcounted and pass it as the instance for creating the callable, give it the reference and make it so when the refcounted disappears it clears it from the registry.

This is something I had attempted, feel free to try. But check out this issue as well godotengine/godot#74990

dsnopek · 2023-09-08T17:55:48Z

If you could test the CustomCallable PR (godotengine/godot#79005) with this language binding and report whether or not it was sufficient for your needs, that could help get it merged. :-) The main thing holding back that PR at the moment, is that we're waiting for other language bindings to try it out and give feedback.

Trey2k · 2023-09-08T17:57:10Z

If you could test the CustomCallable PR (godotengine/godot#79005) with this language binding and report whether or not it was sufficient for your needs, that could help get it merged. :-) The main thing holding back that PR at the moment, is that we're waiting for other language bindings to try it out and give feedback.

Sure thing! Ill give it a test later this evening and report back.

Trey2k · 2023-09-08T19:55:46Z

@dsnopek I am having some trouble testing the PR, I maintain a fork of godot-cpp with godotengine/godot-cpp#1017 applied, without this PR LuaAPI will not compile at all. There is no workaround I can apply that I know of. This is another big one we have been waiting on for awhile. The issue I am having is there seems to be a lot of merge conflicts and blindly accepting incoming changes does not seem work.

I don't have that much time to dig through the actual issues at the moment sadly, i might give it some more time this weekend.

RadiantUwU · 2023-09-08T20:16:30Z

@dsnopek I am having some trouble testing the PR, I maintain a fork of godot-cpp with godotengine/godot-cpp#1017 applied, without this PR LuaAPI will not compile at all. There is no workaround I can apply that I know of. This is another big one we have been waiting on for awhile. The issue I am having is there seems to be a lot of merge conflicts and blindly accepting incoming changes does not seem work.

I don't have that much time to dig through the actual issues at the moment sadly, i might give it some more time this weekend.

Wish i could fix the merge conflicts since i sure as hell dont have anything to do.

Trey2k · 2023-09-08T20:18:19Z

Wish i could fix the merge conflicts since i sure as hell dont have anything to do.

Feel free, my failed attempt is here https://github.com/Trey2k/godot-cpp/tree/CallableCustom

dsnopek · 2023-09-08T20:21:05Z

I am having some trouble testing the PR, I maintain a fork of godot-cpp with godotengine/godot-cpp#1017 applied, without this PR LuaAPI will not compile at all.

PR godotengine/godot-cpp#1017 needs to get rebased and updated for the review I gave last month before it can be merged. The creator hasn't responded, so I've been thinking about taking that one over in a new PR, but haven't had a chance to do it yet. It is definitely an issue I'd like to see fixed!

Trey2k · 2023-09-09T18:13:46Z

@dsnopek Thank you for all the work you are doing. I have a small update for you, I have gotten godotengine/godot-cpp#1155 to merge with godotengine/godot-cpp#1238 which now builds godot-cpp without issue. I have also compiled Godot with godotengine/godot#79005 and extracted the GDExtension interface.

I am not sure if I am overlooking something or what, but no class for CallableCustom seems to generate to be able to extend from. Does this indicate something went wrong during the merge, or are we supposed to create CallableCustoms in a different way then within Godot itself? You can find the fork of godot-cpp I am working on here if it is any help https://github.com/Trey2k/godot-cpp/tree/CallableCustom

dsnopek · 2023-09-10T13:54:41Z

Ah, so the Godot PR adds the API to gdextension_interface.h to create custom callables, but the godot-cpp PR only uses that API to implement callable_mp() and callable_mp_static(), it doesn't provide a base class for making your own custom callables. But that's a good idea! When I have a chance I think I'll try to extend the godot-cpp PR to include something like that.

However, in the meantime, you could use the APIs added to gdextension_interface.h: basically, you make a GDExtensionCallableCustomInfo struct and pass it to the godot::internal::gdextension_interface_callable_custom_create() function.

Trey2k · 2023-09-10T20:07:26Z

However, in the meantime, you could use the APIs added to gdextension_interface.h: basically, you make a GDExtensionCallableCustomInfo struct and pass it to the godot::internal::gdextension_interface_callable_custom_create() function.

Makes sense, I will attempt this evening and report back. Thank you!

Trey2k · 2023-09-14T19:16:37Z

I think my merge is bad, the test fail to build with

 -DNDEBUG -I/home/trey2k/Development/WeaselGames/modules/luaAPI/external/godot-cpp/gdextension -I/home/trey2k/Development/WeaselGames/modules/luaAPI/external/godot-cpp/include -I/home/trey2k/Development/WeaselGames/modules/luaAPI/external/godot-cpp/gen/include -I/home/trey2k/Development/WeaselGames/modules/luaAPI/external/godot-cpp/src /home/trey2k/Development/WeaselGames/modules/luaAPI/external/godot-cpp/gen/src/classes/texture3drd.cpp
In file included from src/register_types.h:9,
                 from src/register_types.cpp:6:
/home/trey2k/Development/WeaselGames/modules/luaAPI/external/godot-cpp/include/godot_cpp/core/class_db.hpp: In instantiation of 'static void godot::ClassDB::_register_class(bool) [with T = ExampleRef; bool is_abstract = false]':
/home/trey2k/Development/WeaselGames/modules/luaAPI/external/godot-cpp/include/godot_cpp/core/class_db.hpp:212:36:   required from 'static void godot::ClassDB::register_class(bool) [with T = ExampleRef]'
src/register_types.cpp:24:37:   required from here
/home/trey2k/Development/WeaselGames/modules/luaAPI/external/godot-cpp/include/godot_cpp/core/class_db.hpp:181:39: error: invalid conversion from 'void (*)(GDExtensionClassInstancePtr, int32_t, GDExtensionBool)' {aka 'void (*)(void*, int, unsigned char)'} to 'GDExtensionClassNotification2' {aka 'void (*)(void*, int, bool)'} [-fpermissive]
  181 |         GDExtensionClassCreationInfo2 class_info = {
      |                                       ^~~~~~~~~~
      |                                       |
      |                                       void (*)(GDExtensionClassInstancePtr, int32_t, GDExtensionBool) {aka void (*)(void*, int, unsigned char)}
/home/trey2k/Development/WeaselGames/modules/luaAPI/external/godot-cpp/include/godot_cpp/core/class_db.hpp: In instantiation of 'static void godot::ClassDB::_register_class(bool) [with T = ExampleMin; bool is_abstract = false]':
/home/trey2k/Development/WeaselGames/modules/luaAPI/external/godot-cpp/include/godot_cpp/core/class_db.hpp:212:36:   required from 'static void godot::ClassDB::register_class(bool) [with T = ExampleMin]'
src/register_types.cpp:25:37:   required from here
/home/trey2k/Development/WeaselGames/modules/luaAPI/external/godot-cpp/include/godot_cpp/core/class_db.hpp:181:39: error: invalid conversion from 'void (*)(GDExtensionClassInstancePtr, int32_t, GDExtensionBool)' {aka 'void (*)(void*, int, unsigned char)'} to 'GDExtensionClassNotification2' {aka 'void (*)(void*, int, bool)'} [-fpermissive]
  181 |         GDExtensionClassCreationInfo2 class_info = {
      |                                       ^~~~~~~~~~
      |                                       |
      |                                       void (*)(GDExtensionClassInstancePtr, int32_t, GDExtensionBool) {aka void (*)(void*, int, unsigned char)}
/home/trey2k/Development/WeaselGames/modules/luaAPI/external/godot-cpp/include/godot_cpp/core/class_db.hpp: In instantiation of 'static void godot::ClassDB::_register_class(bool) [with T = Example; bool is_abstract = false]':
/home/trey2k/Development/WeaselGames/modules/luaAPI/external/godot-cpp/include/godot_cpp/core/class_db.hpp:212:36:   required from 'static void godot::ClassDB::register_class(bool) [with T = Example]'
src/register_types.cpp:26:34:   required from here
/home/trey2k/Development/WeaselGames/modules/luaAPI/external/godot-cpp/include/godot_cpp/core/class_db.hpp:181:39: error: invalid conversion from 'void (*)(GDExtensionClassInstancePtr, int32_t, GDExtensionBool)' {aka 'void (*)(void*, int, unsigned char)'} to 'GDExtensionClassNotification2' {aka 'void (*)(void*, int, bool)'} [-fpermissive]
  181 |         GDExtensionClassCreationInfo2 class_info = {
      |                                       ^~~~~~~~~~
      |                                       |
      |                                       void (*)(GDExtensionClassInstancePtr, int32_t, GDExtensionBool) {aka void (*)(void*, int, unsigned char)}
/home/trey2k/Development/WeaselGames/modules/luaAPI/external/godot-cpp/include/godot_cpp/core/class_db.hpp: In instantiation of 'static void godot::ClassDB::_register_class(bool) [with T = ExampleVirtual; bool is_abstract = false]':
/home/trey2k/Development/WeaselGames/modules/luaAPI/external/godot-cpp/include/godot_cpp/core/class_db.hpp:212:36:   required from 'static void godot::ClassDB::register_class(bool) [with T = ExampleVirtual]'
src/register_types.cpp:27:41:   required from here
/home/trey2k/Development/WeaselGames/modules/luaAPI/external/godot-cpp/include/godot_cpp/core/class_db.hpp:181:39: error: invalid conversion from 'void (*)(GDExtensionClassInstancePtr, int32_t, GDExtensionBool)' {aka 'void (*)(void*, int, unsigned char)'} to 'GDExtensionClassNotification2' {aka 'void (*)(void*, int, bool)'} [-fpermissive]
  181 |         GDExtensionClassCreationInfo2 class_info = {
      |                                       ^~~~~~~~~~
      |                                       |
      |                                       void (*)(GDExtensionClassInstancePtr, int32_t, GDExtensionBool) {aka void (*)(void*, int, unsigned char)}
/home/trey2k/Development/WeaselGames/modules/luaAPI/external/godot-cpp/include/godot_cpp/core/class_db.hpp: In instantiation of 'static void godot::ClassDB::_register_class(bool) [with T = ExampleAbstract; bool is_abstract = true]':
/home/trey2k/Development/WeaselGames/modules/luaAPI/external/godot-cpp/include/godot_cpp/core/class_db.hpp:217:35:   required from 'static void godot::ClassDB::register_abstract_class() [with T = ExampleAbstract]'
src/register_types.cpp:28:51:   required from here
/home/trey2k/Development/WeaselGames/modules/luaAPI/external/godot-cpp/include/godot_cpp/core/class_db.hpp:181:39: error: invalid conversion from 'void (*)(GDExtensionClassInstancePtr, int32_t, GDExtensionBool)' {aka 'void (*)(void*, int, unsigned char)'} to 'GDExtensionClassNotification2' {aka 'void (*)(void*, int, bool)'} [-fpermissive]
  181 |         GDExtensionClassCreationInfo2 class_info = {
      |                                       ^~~~~~~~~~
      |                                       |
      |                                       void (*)(GDExtensionClassInstancePtr, int32_t, GDExtensionBool) {aka void (*)(void*, int, unsigned char)}

I don't know enough about the inner working of GDExtension yet to really debug this myself, but if I get more time I will try to dig into it.

RadiantUwU · 2023-09-30T18:35:20Z

@Trey2k Seems like 2.1-stable will only be released for 4.2+

RadiantUwU · 2023-09-30T18:36:19Z

godotengine/godot#79005 And this got merged as well, amazing

Trey2k · 2023-09-30T18:39:19Z

Seems like 2.1-stable will only be released for 4.2+

This will probably be the case unless the change is back ported yes.

Trey2k · 2023-10-08T22:18:34Z

Fixed for module based builds in #169

Trey2k · 2023-10-10T18:43:32Z

I wouldn't call this fully resolved until CallableCustoms actually work for GDExtension.

dsnopek · 2023-10-22T14:26:14Z

Ah, so the Godot PR adds the API to gdextension_interface.h to create custom callables, but the godot-cpp PR only uses that API to implement callable_mp() and callable_mp_static(), it doesn't provide a base class for making your own custom callables. But that's a good idea! When I have a chance I think I'll try to extend the godot-cpp PR to include something like that.

Sorry it took me so long to circle back to this, but there is now a PR to godot-cpp that makes it easier to create your own custom callables (by extending the CallableCustom class):

godotengine/godot-cpp#1280

Please give it a try when you have a chance, and let me know if it works for you!

michieal · 2023-10-22T19:20:15Z

I'm very hopeful for this, as I have encountered this bug myself, and wondered what had caused it.
It's especially noticeable when running your game in debug mode from your IDE instead of from the editor.

RadiantUwU added the bug Something isn't working label Sep 8, 2023

dsnopek mentioned this issue Sep 19, 2023

Implement callable_mp() and callable_mp_static() godotengine/godot-cpp#1155

Merged

Trey2k mentioned this issue Oct 9, 2023

Add LuaFuntionRef type #172

Merged

RadiantUwU closed this as completed Oct 10, 2023

Trey2k reopened this Oct 10, 2023

dsnopek mentioned this issue Oct 22, 2023

Add CallableCustom that devs can use in their GDExtensions godotengine/godot-cpp#1280

Merged

Trey2k mentioned this issue Oct 22, 2023

Bump to godot 4.2-stable #180

Merged

RadiantUwU mentioned this issue Nov 9, 2023

Feature Request:Cross VM object support #185

Closed

Trey2k closed this as completed in #180 Nov 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug Report:Memory leak with passing lua functions to GDScript #155

Bug Report:Memory leak with passing lua functions to GDScript #155

RadiantUwU commented Sep 8, 2023

Trey2k commented Sep 8, 2023 •

edited

Loading

RadiantUwU commented Sep 8, 2023

RadiantUwU commented Sep 8, 2023

RadiantUwU commented Sep 8, 2023

Trey2k commented Sep 8, 2023

dsnopek commented Sep 8, 2023 •

edited

Loading

Trey2k commented Sep 8, 2023

Trey2k commented Sep 8, 2023 •

edited

Loading

RadiantUwU commented Sep 8, 2023

Trey2k commented Sep 8, 2023

dsnopek commented Sep 8, 2023

Trey2k commented Sep 9, 2023 •

edited

Loading

dsnopek commented Sep 10, 2023

Trey2k commented Sep 10, 2023 •

edited

Loading

Trey2k commented Sep 14, 2023

RadiantUwU commented Sep 30, 2023

RadiantUwU commented Sep 30, 2023

Trey2k commented Sep 30, 2023

Trey2k commented Oct 8, 2023

Trey2k commented Oct 10, 2023

dsnopek commented Oct 22, 2023

michieal commented Oct 22, 2023

Bug Report:Memory leak with passing lua functions to GDScript #155

Bug Report:Memory leak with passing lua functions to GDScript #155

Comments

RadiantUwU commented Sep 8, 2023

Trey2k commented Sep 8, 2023 • edited Loading

RadiantUwU commented Sep 8, 2023

RadiantUwU commented Sep 8, 2023

RadiantUwU commented Sep 8, 2023

Trey2k commented Sep 8, 2023

dsnopek commented Sep 8, 2023 • edited Loading

Trey2k commented Sep 8, 2023

Trey2k commented Sep 8, 2023 • edited Loading

RadiantUwU commented Sep 8, 2023

Trey2k commented Sep 8, 2023

dsnopek commented Sep 8, 2023

Trey2k commented Sep 9, 2023 • edited Loading

dsnopek commented Sep 10, 2023

Trey2k commented Sep 10, 2023 • edited Loading

Trey2k commented Sep 14, 2023

RadiantUwU commented Sep 30, 2023

RadiantUwU commented Sep 30, 2023

Trey2k commented Sep 30, 2023

Trey2k commented Oct 8, 2023

Trey2k commented Oct 10, 2023

dsnopek commented Oct 22, 2023

michieal commented Oct 22, 2023

Trey2k commented Sep 8, 2023 •

edited

Loading

dsnopek commented Sep 8, 2023 •

edited

Loading

Trey2k commented Sep 8, 2023 •

edited

Loading

Trey2k commented Sep 9, 2023 •

edited

Loading

Trey2k commented Sep 10, 2023 •

edited

Loading