On Tue, 26 Nov 2019 09:22:01 GMT, Ajit Ghaisas <aghai...@openjdk.org> wrote:
> On Tue, 19 Nov 2019 14:29:16 GMT, David Grieve > <github.com+4412658+dsgri...@openjdk.org> wrote: > >> On Tue, 19 Nov 2019 10:48:52 GMT, Ajit Ghaisas <aghai...@openjdk.org> wrote: >> >>> On Fri, 15 Nov 2019 09:14:04 GMT, Ajit Ghaisas <aghai...@openjdk.org> wrote: >>> >>>> On Thu, 14 Nov 2019 18:33:05 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 >>>>> >>>>> 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. >>>> >>>>> I think inverting the call is fine. That's what I did in my fix >>>>> ([DeanWookey/openjdk-jfx@65a1ed8](https://github.com/DeanWookey/openjdk-jfx/commit/65a1ed82bce262294f1969e9a12e1126ec8a1ec6)) >>>>> and we've been testing that out thoroughly for over a year. >>>>> >>>>> It's as if you are adding nodes 1 by 1 to the scene graph, something we >>>>> know works and is fast. My change tries to emulate that more accurately >>>>> to avoid side effects. Theoretically, we should be able to do better when >>>>> many nodes are added at once because we have all the information upfront. >>>>> >>>>> The one side effect I can see by only applying commit 2 is that the first >>>>> call of reapplyCSS() calls reapplyCss on every node in the tree and that >>>>> sets the cssFlag = CssFlags.UPDATE;. The subsequent calls will hit this >>>>> in reapplyCSS(): >>>>> >>>>> ``` >>>>> if (cssFlag == CssFlags.UPDATE) { >>>>> cssFlag = CssFlags.REAPPLY; >>>>> notifyParentsOfInvalidatedCSS(); >>>>> return; >>>>> } >>>>> ``` >>>>> >>>>> and return without doing all the unnecessary work. As a result however, >>>>> instead of leaving with cssFlag = CssFlags.UPDATE, all the nodes leave >>>>> with CssFlags.REAPPLY. That might cause an unnecessary css pass later? >>>>> >>>>> Doing it in the order it happens now, that check for the update flag >>>>> shouldn't be true because its bottom up. >>>> >>>> It is a good observation about cssFlag. I have not seen any side effect >>>> with the limited testing that I have done. It may be possible that the >>>> "unnecessary css pass later" scenario is not covered by the test cases >>>> that we have. >>> >>>> Perhaps short-circuiting the call to reapplyCss() from the reapplyCSS() >>>> method is the thing to do. >>> >>> This comment from @dsgrieve got me interested. I checked the test code >>> JDK-8151756 with cssFlags logged, it became evident that the cssFlag gets >>> set to DIRTY_BRANCH for every parent and reapplyCss() gets invoked for each >>> of the children. This is the exact redundant processing. >>> >>> >>> Test from JDK-8151756 with additional one level of Node hierarchy. >>> >>> Parent1<--Parent2<--Parent3<--Rectangle (leaf child) >>> >>> Log from test program ---- >>> Parent 1 : VBox@1d9e402b >>> Parent 2 : VBox@4cc2dcce >>> Parent 3 : VBox@4cc2dcce >>> Rectangle >>> >>> **REAPPLY_CSS called for : VBox@1d9e402b ----- CssFlags.CLEAN >>> REAPPLY_CSS called for : Rectangle[...] ----- CssFlags.CLEAN** >>> reapplyCss called for : Rectangle[...] ----- CssFlags.CLEAN >>> **REAPPLY_CSS called for : VBox@19234c0d ----- CssFlags.DIRTY_BRANCH** >>> reapplyCss called for : VBox@19234c0d ----- CssFlags.DIRTY_BRANCH >>> reapplyCss called for : Rectangle[...] ----- CssFlags.CLEAN >>> **REAPPLY_CSS called for : VBox@4cc2dcce ----- CssFlags.DIRTY_BRANCH** >>> reapplyCss called for : VBox@4cc2dcce ----- CssFlags.DIRTY_BRANCH >>> reapplyCss called for : VBox@19234c0d ----- CssFlags.UPDATE >>> reapplyCss called for : Rectangle[...] ----- CssFlags.CLEAN >>> **REAPPLY_CSS called for : VBox@1d9e402b ----- CssFlags.DIRTY_BRANCH** >>> reapplyCss called for : VBox@1d9e402b ----- CssFlags.DIRTY_BRANCH >>> reapplyCss called for : VBox@4cc2dcce ----- CssFlags.UPDATE >>> reapplyCss called for : VBox@19234c0d ----- CssFlags.UPDATE >>> reapplyCss called for : Rectangle[...] ----- CssFlags.CLEAN >>> >>> >>> Proposed New Fix : >>> ------------------- >>> I added a simple check to avoid reapplyCss() call for each Node with >>> DIRTY_BRANCH cssFlag. Here is the patch - >>> >>> diff --git a/modules/javafx.graphics/src/main/java/javafx/scene/Node.java >>> b/modules/javafx.graphics/src/main/java/javafx/scene/Node.java >>> index 877e0fd6c8..8606dfb575 100644 >>> --- a/modules/javafx.graphics/src/main/java/javafx/scene/Node.java >>> +++ b/modules/javafx.graphics/src/main/java/javafx/scene/Node.java >>> @@ -9416,7 +9416,7 @@ public abstract class Node implements EventTarget, >>> Styleable { >>> if (cssFlag == CssFlags.REAPPLY) return; >>> >>> // RT-36838 - don't reapply CSS in the middle of an update >>> - if (cssFlag == CssFlags.UPDATE) { >>> + if (cssFlag == CssFlags.UPDATE || cssFlag == >>> CssFlags.DIRTY_BRANCH) { >>> cssFlag = CssFlags.REAPPLY; >>> notifyParentsOfInvalidatedCSS(); >>> return; >>> >>> With this fix - >>> Log from test program ---- >>> Parent 1 : VBox@36d24c70 >>> Parent 2 : VBox@35af5cea >>> Parent 3 : VBox@35af5cea >>> Rectangle >>> >>> **REAPPLY_CSS called for : VBox@36d24c70 ----- CssFlags.CLEAN** >>> **REAPPLY_CSS called for : Rectangle[...] ----- CssFlags.CLEAN** >>> reapplyCss called for : Rectangle[...] ----- CssFlags.CLEAN >>> **REAPPLY_CSS called for : VBox@5d4b6983 ----- CssFlags.DIRTY_BRANCH >>> REAPPLY_CSS called for : VBox@35af5cea ----- CssFlags.DIRTY_BRANCH >>> REAPPLY_CSS called for : VBox@36d24c70 ----- CssFlags.DIRTY_BRANCH** >>> reapplyCss called for : VBox@36d24c70 ----- CssFlags.REAPPLY >>> reapplyCss called for : VBox@35af5cea ----- CssFlags.REAPPLY >>> reapplyCss called for : VBox@5d4b6983 ----- CssFlags.REAPPLY >>> reapplyCss called for : Rectangle[...] ----- CssFlags.CLEAN >>> >>> >>> I verified that all graphics/controls unit tests & all system tests pass >>> with this fix. >>> I launched and played with Ensemble app. I did not see any visible >>> artifacts. >> >> @aghaisas You can avoid the call to notifyParentsOfInvalidatedCSS in the >> case where the flag is DIRTY_BRANCH. >> >> I like the looks of this. From the 10,000 foot view, when a node's parent >> changes, or a node's scene changes, CSS should be reapplied. This is exactly >> what 'if (sceneChanged) reapplyCSS()' says, and what happens in parent >> property's invalidated method. All of the optimizations (do I _really_ need >> to reapply css?) happen elsewhere, so I like this solution better than >> passing a boolean around (the original patch). > > Thanks @dsgrieve for having a look. I have updated the PR as suggested to > avoid call to notifyParentsOfInvalidatedCSS in case the flag is DIRTY_BRANCH. > Also, I have modified the system test as suggested by @kevinrushforth. > > Kindly review. > > Limited testing shows that this fix holds up good. Trying to run this, but have to build on Windows. Ugh! PR: https://git.openjdk.java.net/jfx/pull/34
