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

Fri Nov 15 09:14:04 UTC 2019

On Thu, 14 Nov 2019 18:33:05 GMT, Kevin Rushforth <kcr 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
> 
> 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 at 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.

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