On Thu, 14 Nov 2019 11:33:24 GMT, Dean Wookey <dwoo...@openjdk.org> wrote:
> On Wed, 13 Nov 2019 19:10:45 GMT, David Grieve > <github.com+4412658+dsgri...@openjdk.org> wrote: > >> On Tue, 12 Nov 2019 16:55:45 GMT, Ajit Ghaisas <aghai...@openjdk.org> wrote: >> >>> On Tue, 12 Nov 2019 16:52:54 GMT, Ajit Ghaisas <aghai...@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 >>>> >>>> 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. PR: https://git.openjdk.java.net/jfx/pull/34
