[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: RFR: 8269907 memory leak - Dirty Nodes / Parent removed [v2]
From:       Kevin Rushforth <kcr () openjdk ! java ! net>
Date:       2021-10-30 20:40:25
Message-ID: iBxSIlKa8G3taXdt8R_n1L2noWQDx5Z0SLMg00U9MxM=.09a024b4-c0d9-4533-bb43-6abc0840ac0e () github ! com
[Download RAW message or body]

On Thu, 22 Jul 2021 11:50:11 GMT, Florian Kirmaier <fkirmaier@openjdk.org> wrote:

> > After thinking about this issue for some time, I've now got a solution.
> > I know put the scene in the state it is, before is was shown, when the dirtyNodes \
> > are unset, the whole scene is basically considered dirty.  This has the drawback \
> > of rerendering, whenever a window is "reshown", but it restores sanity about \
> > memory behaviour, which should be considered more important.
> 
> Florian Kirmaier has updated the pull request incrementally with one additional \
> commit since the last revision: 
> JDK-8269907
> The bug is now fixed in a new way. Toolkit now supports registering \
> CleanupListeners, which can clean up the dirty nodes, avoiding memoryleaks.

I took a closer look, and the current PR has at least one problem. The call to \
`synchronizeSceneNode` must only be done with the `renderLock` held. If you are using \
this in production, then it likely means you aren't hitting the cases where this \
would break. I recommend either my earlier suggestion of hooking into the existing \
listener, or else you will need to acquire the `renderLock`. If the latter, then one \
thing you will want to avoid is running both a regular sync pass and a cleanup pass \
in the same pulse. I see that you try to ensure that by only adding the listener if \
the scene is not showing, but since you might have to also check in the listener \
itself (not sure).

-------------

PR: https://git.openjdk.java.net/jfx/pull/584


[prev in list] [next in list] [prev in thread] [next in thread]