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

List:       openjdk-openjfx-dev
Subject:    Re: RFR: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
From:       Sverre Moe <github.com+3070076+DJViking () openjdk ! java ! net>
Date:       2019-11-26 15:04:51
Message-ID: MyRL-9hg1UsrOuE2Dtlc3g_-g0fKLQj4YHJVvf_VbXA=.cffad606-3188-440b-b62c-745306094f31 () github ! com
[Download RAW message or body]

On Tue, 26 Nov 2019 09:22:04 GMT, Kevin Rushforth <kcr@openjdk.org> wrote:

> On Tue, 12 Nov 2019 16:45:04 GMT, Ajit Ghaisas <aghaisas@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).

I am curious. Will∕Could this CSS performance improvement be backported to JavaFX \
11? The bug report says only that it will be fixed in JavaFX 14.

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


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