On Tue, 26 Nov 2019 09:22:01 GMT, David Grieve <dgri...@openjdk.org> wrote:
> 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! Request to @DeanWookey, @tomsontom, @swpalmer - can you please confirm if this fix helps your application or tests? PR: https://git.openjdk.java.net/jfx/pull/34
