RFR: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
Ajit Ghaisas
aghaisas at openjdk.org
Tue Nov 19 10:48:52 UTC 2019
On Fri, 15 Nov 2019 09:14:04 GMT, Ajit Ghaisas <aghaisas at openjdk.org> wrote:
> 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.
> 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 at 1d9e402b
Parent 2 : VBox at 4cc2dcce
Parent 3 : VBox at 4cc2dcce
Rectangle
**REAPPLY_CSS called for : VBox at 1d9e402b ----- CssFlags.CLEAN
REAPPLY_CSS called for : Rectangle[...] ----- CssFlags.CLEAN**
reapplyCss called for : Rectangle[...] ----- CssFlags.CLEAN
**REAPPLY_CSS called for : VBox at 19234c0d ----- CssFlags.DIRTY_BRANCH**
reapplyCss called for : VBox at 19234c0d ----- CssFlags.DIRTY_BRANCH
reapplyCss called for : Rectangle[...] ----- CssFlags.CLEAN
**REAPPLY_CSS called for : VBox at 4cc2dcce ----- CssFlags.DIRTY_BRANCH**
reapplyCss called for : VBox at 4cc2dcce ----- CssFlags.DIRTY_BRANCH
reapplyCss called for : VBox at 19234c0d ----- CssFlags.UPDATE
reapplyCss called for : Rectangle[...] ----- CssFlags.CLEAN
**REAPPLY_CSS called for : VBox at 1d9e402b ----- CssFlags.DIRTY_BRANCH**
reapplyCss called for : VBox at 1d9e402b ----- CssFlags.DIRTY_BRANCH
reapplyCss called for : VBox at 4cc2dcce ----- CssFlags.UPDATE
reapplyCss called for : VBox at 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 at 36d24c70
Parent 2 : VBox at 35af5cea
Parent 3 : VBox at 35af5cea
Rectangle
**REAPPLY_CSS called for : VBox at 36d24c70 ----- CssFlags.CLEAN**
**REAPPLY_CSS called for : Rectangle[...] ----- CssFlags.CLEAN**
reapplyCss called for : Rectangle[...] ----- CssFlags.CLEAN
**REAPPLY_CSS called for : VBox at 5d4b6983 ----- CssFlags.DIRTY_BRANCH
REAPPLY_CSS called for : VBox at 35af5cea ----- CssFlags.DIRTY_BRANCH
REAPPLY_CSS called for : VBox at 36d24c70 ----- CssFlags.DIRTY_BRANCH**
reapplyCss called for : VBox at 36d24c70 ----- CssFlags.REAPPLY
reapplyCss called for : VBox at 35af5cea ----- CssFlags.REAPPLY
reapplyCss called for : VBox at 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.
PR: https://git.openjdk.java.net/jfx/pull/34
More information about the openjfx-dev
mailing list