RFR: 8322619: Parts of SG no longer update during rendering - overlapping - culling - dirty

[Ambarish Rapte]

On Mon, 6 May 2024 14:14:05 GMT, eduardsdv <duke at openjdk.org> wrote:

> This is an alternative solution to the PR: https://github.com/openjdk/jfx/pull/1310.
> 
> This solution is based on the invariant that if a node is marked as dirty, all ancestors must also be marked as dirty and that if an ancestor is marked as clean, all descendants must also be marked as clean. 
> Therefore I removed the ``clearDirtyTree()`` method and put its content to the ``clearDirty()`` method.
> 
> Furthermore, since dirty flag is only used when rendering by ``ViewPainter``, it should also be deleted by ``ViewPainter`` only. 
> This guarantees:
> 1. that all dirty flags are removed after rendering, and 
> 2. that no dirty flags are removed when a node is rendered, e.g. by creating a snapshot or printing.
> Therefore I removed all calls of the methods ``clearDirty()`` and ``clearDirtyTree()`` from all other classes except the ``ViewerPainter``.
> 
> The new version of the ``clearDirty()`` method together with calling it from the ``ViewerPainter`` needs to visit far fewer nodes compared to the version prior this PR.
> 
> The supplied test checks that the nodes are updated even if they are partially covered, which led to the error in the version before the PR. The test can be started with ``gradlew -PFULL_TEST=true :systemTests:test --tests NGNodeDirtyFlagTest``

Providing a few comments on test. 
I might need a day or two more for testing/reviewing the fix.

tests/system/src/test/java/test/com/sun/prism/impl/NGNodeDirtyFlagTest.java line 68:

> 66:             primaryStage.show();
> 67: 
> 68:             launchLatch.countDown();

This countDown() should be called after stage is shown to ensure the stage shown before proceeding.
`primaryStage.setOnShown(e -> Platform.runLater(launchLatch::countDown));`

tests/system/src/test/java/test/com/sun/prism/impl/NGNodeDirtyFlagTest.java line 75:

> 73:     public static void setupOnce() {
> 74:         Util.launch(launchLatch, MyApp.class);
> 75:         assertEquals(0, launchLatch.getCount());

Util.launch() asserts if launchLatch.countDown() does not occur in 15 seconds, so this line can be removed.

tests/system/src/test/java/test/com/sun/prism/impl/NGNodeDirtyFlagTest.java line 90:

> 88:         StackPane root = myApp.root;
> 89: 
> 90:         runAndWait(() -> {

All such calls can be changed to use `Util.runAndWait()`

tests/system/src/test/java/test/com/sun/prism/impl/NGNodeDirtyFlagTest.java line 103:

> 101:         });
> 102: 
> 103:         Thread.sleep(500);

Could use `Util.waitForIdle(Scene)` instead of sleep. It would reduce the test time and should work. We can consider a sleep if `Util.waitForIdle(Scene)` does not work. Similarly please see if other sleep calls can be removed.

tests/system/src/test/java/test/com/sun/prism/impl/NGNodeDirtyFlagTest.java line 149:

> 147:             throw new RuntimeException(e);
> 148:         }
> 149:     }

Please use the `Util.runAndWait()` instead of adding this method. So this method can be removed and all calls can be changed to Util.runAndWait().
Please do remove any imports that become unused afterwards.

tests/system/src/test/java/test/com/sun/prism/impl/NGNodeDirtyFlagTest.java line 188:

> 186:         return result;
> 187:     }
> 188: }

Please add an empty line at the end.

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

Changes requested by arapte (Reviewer).

PR Review: https://git.openjdk.org/jfx/pull/1451#pullrequestreview-2147056008
PR Review Comment: https://git.openjdk.org/jfx/pull/1451#discussion_r1658155426
PR Review Comment: https://git.openjdk.org/jfx/pull/1451#discussion_r1658158260
PR Review Comment: https://git.openjdk.org/jfx/pull/1451#discussion_r1658169114
PR Review Comment: https://git.openjdk.org/jfx/pull/1451#discussion_r1658173122
PR Review Comment: https://git.openjdk.org/jfx/pull/1451#discussion_r1658166372
PR Review Comment: https://git.openjdk.org/jfx/pull/1451#discussion_r1658161537