RFR: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation

Thu Nov 14 15:02:40 UTC 2019

On Thu, 14 Nov 2019 14:27:24 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:

> On Thu, 14 Nov 2019 12:34:56 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:
> 
>> On Thu, 14 Nov 2019 11:33:24 GMT, Dean Wookey <dwookey at openjdk.org> wrote:
>> 
>>> On Wed, 13 Nov 2019 19:10:45 GMT, David Grieve <github.com+4412658+dsgrieve at openjdk.org> wrote:
>>> 
>>>> On Tue, 12 Nov 2019 16:55:45 GMT, Ajit Ghaisas <aghaisas at openjdk.org> wrote:
>>>> 
>>>>> On Tue, 12 Nov 2019 16:52:54 GMT, Ajit Ghaisas <aghaisas at openjdk.org> wrote:
>>>>> 
>>>>>> On Tue, 12 Nov 2019 16:45:04 GMT, Ajit Ghaisas <aghaisas at 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.

PR: https://git.openjdk.java.net/jfx/pull/34