RFR: 8269907 memory leak - Dirty Nodes / Parent removed [v7]

[John Hendrikx]

On Fri, 27 Jan 2023 14:51:59 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> Florian Kirmaier has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains eight commits:
>> 
>>  - JDK-8269907
>>    Added missing changes after merge
>>  - Merge remote-tracking branch 'origjfx/master' into JDK-8269907-dirty-and-removed
>>    
>>    # Conflicts:
>>    #	modules/javafx.graphics/src/main/java/com/sun/javafx/tk/Toolkit.java
>>    #	modules/javafx.graphics/src/main/java/javafx/scene/Scene.java
>>  - Merge remote-tracking branch 'origin/master'
>>  - JDK-8269907
>>    Removed the sync methods for the scene, because they don't work when peer is null, and they are not necessary.
>>  - JDK-8269907
>>    Fixed rare bug, causing bounds to be out of sync.
>>  - JDK-8269907
>>    We now require the rendering lock when cleaning up dirty nodes. To do so, we moved some code required for snapshot into a reusable method.
>>  - 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.
>>  - JDK-8269907
>>    Fixing dirty nodes and parent removed, when a window is no longer showing.  This typically happens with context menus.
>
> I took another good look at this solution, and I would like to offer an alternative as I think this solution is more dealing with symptoms of the underlying problem.
> 
> I think the underlying problem is:
> 
> 1) `Parent` is tracking a `removed` list, which is a list of `Node`. However, it only requires the peer for the dirty area calculation. This is a classic case of not being specific enough with the information you need. If `Parent` is modified to store `NGNode` in its `removed` list, then the problem of `removed` keeping references to old nodes disappears as `NGNode` does not refer back to `Node`.  Note: there is a test method currently in `Parent` that returns the list of removed nodes -- these tests would need to be modified to work with a list of `NGNode` or some other way should be found to check these cases.
> 
> 2) `Scene` keeps tracking dirty nodes even when it is not visible. The list of dirty nodes could (before this fix) become as big as you want as it was never cleared as the pulse listener that synchronizes the nodes never runs when `peer == null` -- keep adding and removing new nodes while the scene is not shown, and the list of dirty nodes grows indefinitely. This only happens if the scene has been shown at least once before, as before that time special code kicks in that tries to avoid keeping track of all scene nodes in the dirty list.
> 
> I think the better solution here would be to reset the scene to go back to its initial state (before it was shown) and sync all nodes when it becomes visible again. This can be achieved by setting the dirty node list to `null` to trigger the logic that normally only triggers on the first show.  `addToDirtyList` should simply do nothing when `peer == null` .
> 
> I believe the `syncPeer` call is smart enough not to update the peer in the situation where a scene is hidden and then reshown, even if `Scene` calls it again on all its nodes (`syncPeer` in `Node` will check `dirtyBits.isEmpty()`).
> 
> I'd also highly recommend using `ArrayList` instead of a manual `Node[] dirtyNodes` in `Scene`.

> @hjohn
> 
> ```
> The Parent is tracking a removed list, which is a list of Node. However, it only requires the peer for the dirty area calculation. This is a classic case of not being specific enough with the information you need. If Parent is modified to store NGNode in its removed list, then the problem of removed keeping references to old nodes disappears as NGNode does not refer back to Node. Note: there is a test method currently in Parent that returns the list of removed nodes -- these tests would need to be modified to work with a list of NGNode or some other way should be found to check these cases.
> ```
> 
> This would just end in an leak of the NGNode, instead of the Node? There are also some leaks related to NGNode, bug guess I'll have to fix the more obvious bugs first. Also, NGNode sometimes refers to the Node. There are a lot of issues in this area.

Why not fix the root causes? This PR introduces several new things to fix a bug in a round about way that would not be needed at all if the root causes are fixed. I'm happy to help out here, as I prefer fixing the underlying problems (which probably solves multiple problems at once) rather than having to deal with each symptom and making things even more inscrutable in places where these symptoms appear.

If you know the root causes, then please explain them so they can be dealt with. I've only taken a short look, and I agree that `NGNode` probably would be leaked instead. However, the end goal is to track how big the dirty area is. If keeping a list of nodes/ngnodes is bad, then how about keeping a rectangle instead and tracking the dirty area immediately?  That would also solve problems related to NGNode pointing back to Node.

This would likely solve multiple problems, not just related to stages -- I suppose nodes can be detached and this removed list could cause problems then as well.
 
> ## Resetting the Scene
> This approach has one problem - it would change the complexity of showing/hiding a window. This issue typically happened with Popups, which are regularly shown/hidden. I guess changing the complexity to O() might be ok - but I'm not sure about it.

I think that the resulting code would be simpler, with less special cases (why should a stage that was shown before act differently from a fresh one). The O(n) loop to sync all peers should not be an issue when doing a relatively heavy action like showing a window as `syncPeer` will do nothing for nodes that haven't become dirty.

Also, it has benefits as well. Changes to nodes on a shown then hidden stage would now be more performant, and would eliminate any performance differences between a fresh stage and one that was shown before.

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

PR: https://git.openjdk.org/jfx/pull/584