On Thu, 26 Jan 2023 09:54:25 GMT, Florian Kirmaier <fkirma...@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 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've left some comments.
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/Toolkit.java line 420:
> 418: new WeakHashMap<>();
> 419: final Map<TKPulseListener,AccessControlContext> cleanupList =
> 420: new WeakHashMap<>();
I wonder why we need `WeakHashMap` here. These maps are short-lived anyway,
since they're local variables.
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/Toolkit.java line 494:
> 492: public void addCleanupListener(TKPulseListener listener) {
> 493: AccessControlContext acc = AccessController.getContext();
> 494: cleanupListeners.put(listener,acc);
Access to `cleanupListeners` is not synchronized on `this` here, but it is
synchronized in L426.
You should either synchronize each and every access, or none of it.
modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 745:
> 743: private void checkCleanDirtyNodes() {
> 744: if(!cleanupAdded) {
> 745: if((window.get() == null || !window.get().isShowing()) &&
> dirtyNodesSize > 0) {
Minor: space after `if`.
You could also reduce nesting by consolidating the two separate conditions.
modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 2112:
> 2110:
> **************************************************************************/
> 2111:
> 2112: private void windowForSceneChanged(Window oldWindow, Window window)
> {
This change doesn't seem necessary.
modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 2129:
> 2127: }
> 2128:
> 2129: private final ChangeListener<Boolean> sceneWindowShowingListener =
> (p, o, n) -> {checkCleanDirtyNodes(); } ;
Minor: should be no space before `;`.
You could also remove the curly braces.
tests/system/src/test/java/test/javafx/scene/DirtyNodesLeakTest.java line 35:
> 33: import junit.framework.Assert;
> 34: import org.junit.BeforeClass;
> 35: import org.junit.Test;
Since this is a new class, you should use the JUnit5 API.
tests/system/src/test/java/test/javafx/scene/DirtyNodesLeakTest.java line 44:
> 42: import static test.util.Util.TIMEOUT;
> 43:
> 44: public class DirtyNodesLeakTest {
Since this tests dirty nodes of a `Scene`, maybe you could use a name like
`Scene_dirtyNodesLeakTest`?
-------------
PR: https://git.openjdk.org/jfx/pull/584
