GLFW callbacks not released #821
Comments
UntilNextCall should pin whatever is the latest value for each individual function, and unpin any old ones. If it's pinning the latest of all calls (i.e. only pinning one delegate at any given time) then this is a bug that needs to be fixed, most likely in the SilkTouch generator.
GC utility should unpin all handles, regardless of the pin mode. If it's not, then this is the bug that needs to be fixed in a PR.
This sounds reasonable, would accept this as a PR. |
|
Will mark this as a community bugfix, as this the smallest memory leak ever, not variable, that in practice is benign. We'll be happy to accept PRs for fixing this! But I'd like to keep UntilNextCall, so that might mean fixing the generator to do the right thing. cc'ing @HurricanKai for visibility. |
|
Thanks for the feedback.
Ah, I see. I think in this case the problem is that all corresponding calls to PinUntilNextCall use 0 as the slot. At least I guess it should be a different slot for each function. If that is how thing are supposed to work I agree that UntilNextCall should remain there.
I agree and actually for that reason I avoided to call it memory leak as that usually sounds worse. Also my use is rather special with a service creating a new window for each call.
After looking into the code I am not sure yet if I will be able to provide a PR for that... But I can for sure create one to set the callbacks to null after unregistering. |
|
I'll investigate the slot functionality, I was pretty confident it worked well 🤔 |
|
Well, when debugging I see that the code generated for GLFW uses 0 as the slot value in each call to PinUntilNextCall. After looking a bit deeper into the issue I think that the problem is that NativeApiGenerator creates a new instance of MarshalContext in each call to ProcessMethod. This causes GCCount to be 0 which ends up as the slot value. I gave it a quick try by using a static field to back GCCount and with this the generated code actually uses non-zero values for the slots. But I think I still do not understand that code well enough to tell whether that is a good idea or not… |
|
We could probably use the native API entry point’s hash code as the slot, not sure how you get that from an IMarshalContext but Kai should know |
|
The constructor of MarshalContext already has IMethodSymbol as an argument. With that there could be something along the lines of |
I'd say we'd probably want That's my take anyway. |
|
To clarify: I'd prefer the slot to be per native entrypoint rather than per entire method, because what if we had two overloads of the same native function but with different delegate types? We still need to free the other one. By the way, thanks for the engaging conversation! You seem to have caught onto all this really quickly, and your input is really helpful :) |
|
I'm not a fan of using hash codes for this sort of thing, they can collide and it's hard to debug, understand, track. |
|
The GC utility code has been with us for 3 years now! |
|
@HurricanKai do you have any objections to just using a hash code generated from the entry point here? We could make it a dictionary but dictionaries themselves are based on hash codes so it'd be just as reliable to just use the native entry point's hash code as the GC slot. The main reason I don't want to use a slot allocation mechanism is each overload of the same native function would have its own GC slot (which is not ideal) |
|
I'd highly prefer just fixing the allocation mechanism to work once per native function, which I think is rather easy. Not sure how using the hash code would work given that GC slots are assigned incrementally into an array. |
|
The GC slots aren't actually used anywhere other than the GC Utility to my knowledge, which accepts the integer as an arbitrary index to map to a list of GC handles. Right now the slot is always 0. This is the bug, as it's causing only one delegate to be pinned at any given time (well, it's one of the bugs, the other is that GCUtility doesn't do a null check and as a result doesn't properly deallocate). We need to define that slot value, and I don't think it makes sense to give each individual overload of the same native function a slot. So, this presents us with two options:
I could be missing something though! |
Summary
After closing a GLFW rendering window GLFW ande some referenced objects remain in memory.
Steps to reproduce
Comments
From some debugging I think this might be due to those callbacks being lambdas/closures referenced by GlfwWindow.
Also I am not sure if GlfwWindow.UnregisterCallbacks acutally unregisters all of them. At least GCUtility.Unpin does not find the corresponding handles. To me it looks like only the most recent one is actually pinned due using PinMode.UntilNextCall.
I could get rid of the objects locally by making two changes:
[PinObject(PinMode.UntilNextCall)]to[PinObject]Since I do not understand this code well enough I doubt this is the right thing to do. The first step was because I noticed that GCUtilities.Pins contained only a single handle at all which causes Unpin not to find the callbacks in there.
Background
I have a service for rendering 3D scenes to images. This creates a new window for each request and I found that the memory usage keeps growing over time. Doing some memory profiling brought up that those Glfw* objects remain in memory.
The text was updated successfully, but these errors were encountered: