On Thu, 14 Nov 2019 15:02:40 GMT, Dean Wookey <dwoo...@openjdk.org> wrote:
> On Thu, 14 Nov 2019 14:27:24 GMT, Kevin Rushforth <k...@openjdk.org> wrote: > >> On Thu, 14 Nov 2019 12:34:56 GMT, Kevin Rushforth <k...@openjdk.org> wrote: >> >>> On Thu, 14 Nov 2019 11:33:24 GMT, Dean Wookey <dwoo...@openjdk.org> wrote: >>> >>>> On Wed, 13 Nov 2019 19:10:45 GMT, David Grieve >>>> <github.com+4412658+dsgri...@openjdk.org> wrote: >>>> >>>>> On Tue, 12 Nov 2019 16:55:45 GMT, Ajit Ghaisas <aghai...@openjdk.org> >>>>> wrote: >>>>> >>>>>> On Tue, 12 Nov 2019 16:52:54 GMT, Ajit Ghaisas <aghai...@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 >>>>>>> >>>>>>> Reviewers: @kevinrushforth and @arapte >>>>>> >>>>>> It will be helpful if I can get some feedback from community with >>>>>> additional testing apart from what is mentioned in the description. >>>>> >>>>> Has this been checked with SubScene? Two cases would be 1) SubScene >>>>> inheriting a style from its parent, and 2) behavior of a parent in the >>>>> SubScene itself. I don't expect that this would be an issue, but there is >>>>> some special handling of CSS in SubScene IIRC. >>>> >>>> Having looked at this issue previously >>>> (https://mail.openjdk.java.net/pipermail/openjfx-dev/2018-November/022842.html >>>> and >>>> https://github.com/DeanWookey/openjdk-jfx/commit/65a1ed82bce262294f1969e9a12e1126ec8a1ec6), >>>> I'm a bit confused. >>>> >>>> Doesn't commit >>>> https://github.com/openjdk/jfx/pull/34/commits/12ea8220a730ff8d98d520ce870691d54f0de00f >>>> essentially add another reapplyCSS() above the scenesChanged call on line >>>> 1074? I'm guessing this works because reapplyCss() (different from >>>> reapplyCSS()) sets cssFlag to UPDATE, which then means subsequent calls to >>>> reapplyCSS() don't call reapplyCss()? >>>> >>>> Does this leave the whole tree with the CssFlags.REAPPLY set instead of >>>> CssFlags.UPDATE as they would be without these changes? I'm not sure about >>>> the impact of that. >>>> >>>> I'm also curious whether commit 1 is even necessary with commit 2. >>> >>> The above raises good questions about the effect of adding a call to >>> `reapplyCSS` when `scenesChanged` is called. This will need a careful >>> analysis. >>> >>> Regarding whether commit 2 by itself would be sufficient, my initial guess >>> was that it wouldn't be. However, I tested it, and it does in fact improve >>> performance of the test case. Very interesting. >> >> Looking at this a little further, the change from commit 2, to call >> `reapplyCSS` at the beginning of `scenesChanged` really does two things: >> >> 1. Calling `reapplyCSS` right at the beginning of `scenesChanged` flips the >> order of CSS reapplication from the existing "bottom up" order to a "top >> down" order. Note that `scenesChanged` calls `invalidatedScenes` recursively >> (via `setScenes`) to traverse the scene graph. >> >> 2. It effectively undoes the rest of the changes, which were intended to >> avoid calling `reapplyCSS` in cases where it is redundant. As long as the >> performance improvement is preserved, this seems fine. >> >> The first of the above appears to be both necessary and sufficient to both >> maintain correctness while still avoiding the n^2 application of CSS in the >> case of adding a deep scene graph. If this is in fact the case, then I think >> the fix can be simplified even further by moving the following call from its >> current location (after `updateTreeShowing`) to right before the call to >> `scenesChanged`: >> >> if (sceneChanged) reapplyCSS(); >> >> in which case the changes to `Node::scenesChanged` and >> `Parent::scenesChanged` can be reverted >> >> The following patch, by itself (with nothing from commit 1) might be >> sufficient: >> >> --- a/modules/javafx.graphics/src/main/java/javafx/scene/Node.java >> +++ b/modules/javafx.graphics/src/main/java/javafx/scene/Node.java >> @@ -1069,6 +1069,7 @@ public abstract class Node implements EventTarget, >> Styleable { >> focusSetDirty(oldScene); >> focusSetDirty(newScene); >> } >> + if (sceneChanged) reapplyCSS(); // Note this can be moved inside >> the previous 'if' block >> scenesChanged(newScene, newSubScene, oldScene, oldSubScene); >> >> // isTreeShowing needs to take into account of Window's showing >> @@ -1091,8 +1092,6 @@ public abstract class Node implements EventTarget, >> Styleable { >> } >> updateTreeShowing(); >> >> - if (sceneChanged) reapplyCSS(); >> - >> if (sceneChanged && !isDirtyEmpty()) { >> //Note: no need to remove from scene's dirty list >> //Scene's is checking if the node's scene is correct >> >> We will need to do a more complete evaluation as to whether this inversion >> of calling reapplyCSS on the parent before its children has any side >> effects, and also whether there are any of the performance improvements from >> the original fix that aren't covered here. > > I think inverting the call is fine. That's what I did in my fix > (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. I think the order of calls in invalidatedScenes is correct. The scene graph plumbing needs to be done before CSS is applied since a node's style can depend on the styles of its parent. The issue is really calling reapplyCss(). In the case where a child is added to a parent, the child's parent property is invalidated which should call reapplyCSS(), which should (in turn) call reapplyCss(). Next the invalidatedScenes call happens and the whole dance starts again. (See Parent children.onChanged). Perhaps short-circuiting the call to reapplyCss() from the reapplyCSS() method is the thing to do. Since CSS is reapplied from the top down, my parent's cssFlag should be CLEAN before reapplyCss() method is called from reapplyCSS(). It has been a long time since I've been in this code. I'm going to look at this some more. PR: https://git.openjdk.java.net/jfx/pull/34
