On Tue, 26 Nov 2019 09:22:02 GMT, Geoff <github.com+1680611+groos...@openjdk.org> wrote:
> 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. I did a quick check w.r.t. memory using profiler. This patch significantly reduces the heap allocations as well. There is a win on both - performance & memory fronts. PR: https://git.openjdk.java.net/jfx/pull/34
