On Tue, 26 Nov 2019 09:22:04 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
> On Tue, 12 Nov 2019 16:45:04 GMT, Ajit Ghaisas <aghai...@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
