RFR: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation

Tue Nov 26 09:22:04 UTC 2019

On Tue, 12 Nov 2019 16:45:04 GMT, Ajit Ghaisas <aghaisas at openjdk.org> wrote:

> **Issue :**
> https://bugs.openjdk.java.net/browse/JDK-8193445
> 
> **Background :**
> The CSS performance improvement done in [JDK-8151756](https://bugs.openjdk.java.net/browse/JDK-8151756) had to be backed out due to functional regressions reported in [JDK-8185709](https://bugs.openjdk.java.net/browse/JDK-8185709), [JDK-8183100](https://bugs.openjdk.java.net/browse/JDK-8183100) and [JDK-8168951](https://bugs.openjdk.java.net/browse/JDK-8168951).
> Refer to [JDK-8183100](https://bugs.openjdk.java.net/browse/JDK-8183100) for more details on this backout. 
> 
> **Description :**
> This PR reintroduces the CSS performance improvement fix done in [JDK-8151756](https://bugs.openjdk.java.net/browse/JDK-8151756) while addressing the functional regressions that were reported in [JDK-8185709](https://bugs.openjdk.java.net/browse/JDK-8185709), [JDK-8183100](https://bugs.openjdk.java.net/browse/JDK-8183100) and [JDK-8168951](https://bugs.openjdk.java.net/browse/JDK-8168951).
> For ease of review, I have made two separate commits -
> 1) [Commit 1](https://github.com/openjdk/jfx/pull/34/commits/d964675ebc2a42f2fd6928b773819502683f2334) - Reintroduces the CSS performance improvement fix done in [JDK-8151756](https://bugs.openjdk.java.net/browse/JDK-8151756) - most of the patch applied cleanly.
> 2) [Commit 2 ](https://github.com/openjdk/jfx/pull/34/commits/12ea8220a730ff8d98d520ce870691d54f0de00f)- fixes the functional regressions keeping performance improvement intact + adds a system test
> 
> **Root Cause :**
> CSS performance improvement fix proposed in [JDK-8151756 ](https://bugs.openjdk.java.net/browse/JDK-8151756)correctly avoids the redundant CSS reapplication to children of a Parent. 
> What was missed earlier in [JDK-8151756 ](https://bugs.openjdk.java.net/browse/JDK-8151756) fix : "CSS reapplication to the Parent itself”. 
> This missing piece was the root cause of all functional regressions reported against [JDK-8151756](https://bugs.openjdk.java.net/browse/JDK-8151756)
> 
> **Fix :**
> Fixed the identified root cause. See commit 2.
> 
> **Testing :**
> 1. All passing unit tests continue to pass
> 2. New system test (based on [JDK-8209830](https://bugs.openjdk.java.net/browse/JDK-8209830)) added in this PR - fails before this fix and passes after the fix
> 3. System test JDK8183100Test continues to pass
> 4. All test cases attached to regressions [JDK-8185709](https://bugs.openjdk.java.net/browse/JDK-8185709), [JDK-8183100](https://bugs.openjdk.java.net/browse/JDK-8183100) and [JDK-8168951](https://bugs.openjdk.java.net/browse/JDK-8168951) pass with this fix
> 
> In addition, testing by community with specific CSS performance / functionality will be helpful.
> 
> ----------------
> 
> Commits:
>  - 12ea8220: Fix for functional regressions of JDK-8151756 + add a sytem test
>  - d964675e: Reintroduce JDK-8151756 CSS performance fix
> 
> Changes: https://git.openjdk.java.net/jfx/pull/34/files
>  Webrev: https://webrevs.openjdk.java.net/jfx/34/webrev.00
>   Issue: https://bugs.openjdk.java.net/browse/JDK-8193445
>   Stats: 121 lines in 5 files changed: 104 ins; 0 del; 17 mod
>   Patch: https://git.openjdk.java.net/jfx/pull/34.diff
>   Fetch: git fetch https://git.openjdk.java.net/jfx pull/34/head:pull/34

The fix itself looks good and is a much safer approach than the previous one. I've done a fair bit of testing and can see no regressions as a result of this fix. I did find one unrelated issue while testing (a visual bug introduced back in JDK 10) that I will file separately.

The test is pretty close, but still needs a few changes. The main problem is that it does not catch the performance problem, meaning if you run it without the fix, it will still pass. Your test class extends `javafx.application.Application`, which can cause problems, since JUnit will create a new instance of the test class that is different from the one created by the call to `Application.launch` in the `@BeforeClass` method. This in turn leads to the `rootPane` instance used by the test method being different from the one used as the root of the visible scene. The fix is to use a separate nested (static) subclass of Application, and make the rootPane field a static field. I have left inline comments for this and a few other things I noticed.

tests/system/src/test/java/test/javafx/scene/QuadraticCssTimeTest.java line 63:

> 62: 
> 63: public class QuadraticCssTimeTest extends Application {
> 64: 

Remove `extends Application`

tests/system/src/test/java/test/javafx/scene/QuadraticCssTimeTest.java line 66:

> 65:     static private CountDownLatch startupLatch;
> 66:     static private Stage stage;
> 67:     private BorderPane rootPane = new BorderPane();

Minor: our convention is `private static`.

tests/system/src/test/java/test/javafx/scene/QuadraticCssTimeTest.java line 67:

> 66:     static private Stage stage;
> 67:     private BorderPane rootPane = new BorderPane();
> 68: 

Based on how it is used, this needs to be a `static` field (this will not compile anyway once you move the `start method` to a nested class). Also, there is no need to initialize it both here and in the `Application::start` method. One or the other will do.

tests/system/src/test/java/test/javafx/scene/QuadraticCssTimeTest.java line 70:

> 69:     @Override
> 70:     public void start(Stage primaryStage) throws Exception {
> 71:         stage = primaryStage;

Enclose this method in a nested subclass of Application:

    public static class TestApp extends Application {
        @Override
        public void start(Stage primaryStage) throws Exception {
            ...
        }
    }

tests/system/src/test/java/test/javafx/scene/QuadraticCssTimeTest.java line 81:

> 80:     @BeforeClass
> 81:     public static void initFX() {
> 82:         startupLatch = new CountDownLatch(1);

If you add `throws Exception` to the method signature you can avoid the try / catch (most of our newer tests do this).

tests/system/src/test/java/test/javafx/scene/QuadraticCssTimeTest.java line 86:

> 85:         try {
> 86:             if (!startupLatch.await(15, TimeUnit.SECONDS)) {
> 87:                 Assert.fail("Timeout waiting for FX runtime to start");

The entire try / catch block, including the `if` test, can be simplified to this:

    assertTrue(startupLatch.await(15, TimeUnit.SECONDS));

tests/system/src/test/java/test/javafx/scene/QuadraticCssTimeTest.java line 96:

> 95:     public void testTimeForAdding500NodesToScene() throws Exception {
> 96: 
> 97:         // Compute time for adding 500 Nodes

Adding a node to a live scene graph must be done on the JavaFX Application thread. I recommend wrapping the body of this method in a `Util.runAndWait` call.

tests/system/src/test/java/test/javafx/scene/QuadraticCssTimeTest.java line 83:

> 82:         startupLatch = new CountDownLatch(1);
> 83:         new Thread(() -> Application.launch(QuadraticCssTimeTest.class, (String[]) null)).start();
> 84: 

Change `QuadraticCssTimeTest.class` to `TestApp.class`.

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

Changes requested by kcr (Lead).

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