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