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). PR: https://git.openjdk.java.net/jfx/pull/34
