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. PR: https://git.openjdk.java.net/jfx/pull/34
