On Tue, 26 Nov 2019 09:22:02 GMT, Ajit Ghaisas <aghai...@openjdk.org> wrote:
> 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? Does the performance problem addressed here relate to reducing heap space allocations? My profiler pointed me at this issue, and around the same time I'm seeing `OutOfMemoryError`s. I'm wondering if (or rather, _hoping_) they're the same problem. PR: https://git.openjdk.java.net/jfx/pull/34
