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 While we are still discussing the fix itself, I added a few comments on the new test. It generally looks good, but should be run on a variety of systems, with and without the fix (once we have a final fix that we are satisfied with). tests/system/src/test/java/test/robot/javafx/scene/CSSPerf_JDK8193445Test.java line 26: > 25: > 26: package test.robot.javafx.scene; > 27: There is no need for this test to require robot. I recommend moving it to `test.javafx.scene` (and not inherit from `VisualTestBase`). tests/system/src/test/java/test/robot/javafx/scene/CSSPerf_JDK8193445Test.java line 55: > 54: > 55: public class CSSPerf_JDK8193445Test extends VisualTestBase { > 56: We have moved away from putting the bug ID in the test class name, so I recommend renaming it. tests/system/src/test/java/test/robot/javafx/scene/CSSPerf_JDK8193445Test.java line 78: > 77: HBox hbox = new HBox(); > 78: for (int i = 0; i < 300; i++) { > 79: hbox = new HBox(new Text("y"), hbox); In my testing on various machines, the bug is more pronounced, and less prone to system differences with `500` nodes instead of `300`. tests/system/src/test/java/test/robot/javafx/scene/CSSPerf_JDK8193445Test.java line 94: > 93: // It is good enough to catch the regression in performance, if > any > 94: assertTrue("Time to add 300 Nodes is more than 400 mSec", mSec < > 400); > 95: } If you increase the number of nodes to `500` then I recommend bumping the time threshold to `800` msec in case it is run on a very slow system. PR: https://git.openjdk.java.net/jfx/pull/34
