husker-dev / openglfx

OpenGL node for JavaFX
Apache License 2.0
88 stars 12 forks source link

Memory leak when creating multiple OpenGLCanvas elements #45

Closed streamingdv closed 1 year ago

streamingdv commented 1 year ago

In my previous bug report according the native crash in WinUtils.hasDXInterop in version 3.0.5 I mentioned in one of the comments a possible memory leak.

I created now a minimal reproducable example which constantly removes and re-creates the OpenGLCanvas. In a memory profiler or just the Windows Task Manager you can see that the memory will grow and grow. The onDispose event is never called and I could not find a way to manually dispose things (maybe there is already a way)?

For better organization, I decided to create this separate bug ticket for this possible issue. I know that this is maybe an unusual way of using the OpenGLCanvas but for applications which are rendering OpenGL content in a separate window for example there must be a way to proper remove and clean up OpenGLCanvas stuff and as far as I have seen there is no easy way to do this yet?

husker-dev commented 1 year ago

Yes, dispose doesn't work right now :) The library was not initially designed to generate a large number of OpenGL panels. I added the dispose listener at the very beginning of development for future implementation.

I think it's time to do it.

streamingdv commented 1 year ago

Awesome, thanks for the information

husker-dev commented 1 year ago

I have added ability to dispose OpenGLCanvas in this commit.

But it is worth understanding that OpenGLFX also uses internal JavaFX resources, which I cannot remove manually. So, you have to wait for the garbage collector.

streamingdv commented 1 year ago

Awesome thanks, yes sure that's totally okay. If stuff will be garbage collected that's good enough and should not be a blocker for my or any other projects. I appreciate the fix, will try and test as soon as I can.

streamingdv commented 1 year ago

@husker-dev when do you expect to release the new version? Or do you still work on something else you want to include in the next release?

husker-dev commented 1 year ago

@streamingdv I'm currently trying to get the code to work on all platforms because there have been so many changes. Currently it works only on Windows and does not work on Linux. MacOS have not even been tested.

I can only work on the project on weekends, so at best the release will be next weekend. Maybe I'll even find time this week.

husker-dev commented 1 year ago

@streamingdv some updates: I fixed the Linux implementation, but it doesn't work on MacOS right now. I will be able to fix this as soon as I have the opportunity to personally work on this system. This will be in at least a week.

streamingdv commented 1 year ago

@husker-dev okay thanks for the update 👍

husker-dev commented 1 year ago

Wish it is solved. Will see in new a version 🙃.

streamingdv commented 1 year ago

I will test extensively once the new version is available. Thanks again for this awesome lib, you can definetly think of creating some kind of Buy Me a Coffee account ;) Once I release my application your lib will have a lot of potential test users on Windows and Linux platforms :) I have also included the lib and your name in the open source credit window of my app.

husker-dev commented 1 year ago

Thanks for support!

The new version is ready and will be available in maven after some time. There have been big changes, so I recommend you read the readme.md

husker-dev commented 1 year ago

At the moment I think it is possible to use the library via jitpack

streamingdv commented 1 year ago

@husker-dev first glance it looks good with version 4.0 but I need to perform some more testing. One thing I noticed when I upgraded the lib is that WinUtils.hasDXInterop() is gone and I can no longer access this feature because NVDXInterop is internal. I know that this feature might not be interesting from outside the lib for most users but it might become handy in certain conditions. I have compiled my own javafx.graphics lib with OpenGL support and on startup of my application I do following

if (WinUtils.hasDXInterop()) {
    if (Objects.equals(VideoRenderingType.OPEN_GL, videoRenderingType)) { // user can force OpenGL via settings
        Logger.info("Running on Windows. DxInterop is supported on system but OpenGL was requested, set prism order to es2,d3d,sw. Rendering engine is {}", videoRenderingType);
        System.setProperty("prism.order", "es2,d3d,sw");
    } else {
        Logger.info("Running on Windows. DxInterop is supported on system, set prism order to d3d,es2,sw. Rendering engine is {}", videoRenderingType);
        System.setProperty("prism.order", "d3d,es2,sw");
    }
    return;
}

System.setProperty("prism.order", "es2,sw");

Now I don't have the possibility anymore to check if NVDXInterop is supported? Can this information now be accessed somehow else?

husker-dev commented 1 year ago

@streamingdv I didn't think anyone would use this feature at all. I've made the class open for now. I'm planning on redoing this class a bit, so I'll include it in version 4.1

streamingdv commented 1 year ago

Okay awesome, thanks

streamingdv commented 1 year ago

@husker-dev one feedback in regards of my testing, I can't see that fireDisposeEvent is ever called within the lib. Even though I added a listener it will never get notified. Not bad for my program as I don't really need it as I dispose stuff manually but I guess this is not intended?

husker-dev commented 1 year ago

I forgot to clarify. Automatically GLCanvas cannot call dispose. This must be done manually when it is no longer needed

streamingdv commented 1 year ago

Hmm okay, like I said no problem for my application but might be confusing for others.

streamingdv commented 1 year ago

But should the listener not be called when manually calling dispose?

streamingdv commented 1 year ago

@streamingdv some feedback about the memory leak. Good thing when calling dispose my application is able to free up some memory- However, there is still a memory leak in place which you can easily reproduce with my example project. This application just produces 1000 GLCanvas elements, every 1,5 seconds a new one and the old one will be disposed and thrown away. After this 1000 iterations I checked with my YourKit profiler and I can see that there is still stuff lying around which can't be garbage collected

memLeak

Without taking a very detailed look and I'm just guessing here but why are for example the Framebuffer and the Texture objects never disposed in let's say for example NVDXInteropCanvasImpl?

And one other small issue I found is that if disposed is called too fast this could leak to a potential NullPointerException as the context is not yet generated. Even though a null check in the dispose would help there is no mechanism in place that prevents the context generation after the dispose method was called which again would leak to a leak.

In my opinion you can very easily reproduce and test it with my application, and you can check the memory with the YourKit profiler (or a similar profiler) yourself. YourKit is a paid profiler but you can use a test version I think for 15 days with all features.

husker-dev commented 1 year ago

Most FBOs and Textures are not removed because their GLContext is removed. But I noticed some places where they should be manually removed because they are bound to the JavaFX context.

Also, I will try to remake the dospose mechanism so that it is called when GLCanvas is removed from the UI tree or collected by garbage collector.

husker-dev commented 1 year ago

Made some changes (https://github.com/husker-dev/openglfx/commit/7d48de76beceeb8edb0479370e795b1cd95cb738) regarding dispose. I haven't tested it yet, but in theory there should be no leakage points. Now every disposing is cheked for null-safety.

A little later I will implement an automatic dispose mechanism.

streamingdv commented 1 year ago

@husker-dev are there any easy build instructions of how I can compile OpenGLFX locally, i would like to perform some tests so I can further check what exactly is causing these leaks. I would like to test with my example project.

husker-dev commented 1 year ago

@streamingdv You can just clone the project and replace example in lwjgl module. It is the easiest way.

husker-dev commented 1 year ago

If you want to get jars, then execute compileJars task in gradle

streamingdv commented 1 year ago

Okay that's what I tried but I get following error

Task with path 'utils:jar' not found in root project 'openglfx'.

husker-dev commented 1 year ago

@streamingdv pushed fix, try again

streamingdv commented 1 year ago

Ah it was an obsolete module, yeah all good. It works now

streamingdv commented 1 year ago

@husker-dev when calling dispose I get a lot of NullPointerExceptions, probably because drawResultTexture of GLCanvas is still triggered one more time or so after dispose was called. I really suggest to test with my small example project as you can easily check if some new features broke something. Dispose mechanism seems not to work very reliable so far yet. Will continue testing

streamingdv commented 1 year ago

@husker-dev possible fix

https://github.com/streamingdv/openglfx/commit/5a7774688fdf56bfa671b87bd8d1458193c0c486

not sure if you have a better idea? Just added it in my fork as a workaround.

husker-dev commented 1 year ago

@streamingdv Lets chat in discussion tab

husker-dev commented 1 year ago

I think this issue can be changed as closed