Re: [Integrated] RFR: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
Changeset: 83eb0a7c Author:Ajit Ghaisas Date: 2019-11-27 07:05:15 + URL: https://git.openjdk.java.net/jfx/commit/83eb0a7c 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation Reviewed-by: kcr, dgrieve ! modules/javafx.graphics/src/main/java/javafx/scene/Node.java + tests/system/src/test/java/test/javafx/scene/QuadraticCssTimeTest.java
Re: [Approved] RFR: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
On Tue, 26 Nov 2019 09:22:01 GMT, Ajit Ghaisas wrote: > The pull request has been updated with additional changes. > > > > Added commits: > - 2054da4c: Address review comments on test > - 4dade6e5: Simpler fix + System test corrections > - bd4a306a: Revert commit1 and commit 2 > > Changes: > - all: https://git.openjdk.java.net/jfx/pull/34/files > - new: https://git.openjdk.java.net/jfx/pull/34/files/12ea8220..2054da4c > > Webrevs: > - full: https://webrevs.openjdk.java.net/jfx/34/webrev.01 > - incr: https://webrevs.openjdk.java.net/jfx/34/webrev.00-01 > > Issue: https://bugs.openjdk.java.net/browse/JDK-8193445 > Stats: 253 lines in 6 files changed: 132 ins; 104 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 Looks good. Approved by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/34
Re: RFR: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
On Tue, 26 Nov 2019 09:22:04 GMT, Kevin Rushforth wrote:
> On Tue, 12 Nov 2019 16:45:04 GMT, Ajit Ghaisas 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
>
> The fix itself looks good and is a much safer approach than the previous one.
> I've done a fair bit of testing and can see no regressions as a result of
> this fix. I did find one unrelated issue while testing (a visual bug
> introduced back in JDK 10) that I will file separately.
>
> The test is pretty close, but still needs a few changes. The main problem is
> that it does not catch the performance problem, meaning if you run it without
> the fix, it will still pass. Your test class extends
> `javafx.application.Application`, which can cause problems, since JUnit will
> create a new instance of the test class that is different from the one
> created by the call to `Application.launch` in the `@BeforeClass` method.
> This in turn leads to the `rootPane` instance used by the test method being
> different from the one used as the root of the visible scene. The fix is to
> use a separate nested (static) subclass of Application, and make the rootPane
> field a static field. I have left inline comments for this and a few other
> things I noticed.
>
> tests/system/src/test/java/test/javafx/scene/QuadraticCssTimeTest.java line
> 63:
>
>> 62:
>> 63: public class QuadraticCssTimeTest extends Application {
>> 64:
>
> Remove `extends Application`
>
> tests/system/src/test/java/test/javafx/scene/QuadraticCssTimeTest.java line
> 66:
>
>> 65: static private CountDownLatch startupLatch;
>> 66: static private
Re: RFR: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
On Tue, 26 Nov 2019 09:22:02 GMT, Ajit Ghaisas wrote: > On Tue, 26 Nov 2019 09:22:01 GMT, David Grieve wrote: > >> On Tue, 26 Nov 2019 09:22:01 GMT, Ajit Ghaisas wrote: >> >>> On Tue, 19 Nov 2019 14:29:16 GMT, David Grieve >>> wrote: >>> On Tue, 19 Nov 2019 10:48:52 GMT, Ajit Ghaisas wrote: > On Fri, 15 Nov 2019 09:14:04 GMT, Ajit Ghaisas > wrote: > >> On Thu, 14 Nov 2019 18:33:05 GMT, Kevin Rushforth >> wrote: >> >>> On Tue, 12 Nov 2019 16:45:04 GMT, Ajit Ghaisas >>> 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
Re: RFR: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
On Tue, 26 Nov 2019 09:22:01 GMT, Ajit Ghaisas wrote:
> On Tue, 19 Nov 2019 14:29:16 GMT, David Grieve
> wrote:
>
>> On Tue, 19 Nov 2019 10:48:52 GMT, Ajit Ghaisas wrote:
>>
>>> On Fri, 15 Nov 2019 09:14:04 GMT, Ajit Ghaisas wrote:
>>>
On Thu, 14 Nov 2019 18:33:05 GMT, Kevin Rushforth wrote:
> On Tue, 12 Nov 2019 16:45:04 GMT, Ajit Ghaisas
> 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
Re: RFR: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
On Tue, 26 Nov 2019 09:22:01 GMT, David Grieve wrote: > On Tue, 26 Nov 2019 09:22:01 GMT, Ajit Ghaisas wrote: > >> On Tue, 19 Nov 2019 14:29:16 GMT, David Grieve >> wrote: >> >>> On Tue, 19 Nov 2019 10:48:52 GMT, Ajit Ghaisas wrote: >>> On Fri, 15 Nov 2019 09:14:04 GMT, Ajit Ghaisas wrote: > On Thu, 14 Nov 2019 18:33:05 GMT, Kevin Rushforth > wrote: > >> On Tue, 12 Nov 2019 16:45:04 GMT, Ajit Ghaisas >> 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 >>
Re: RFR: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
On Tue, 26 Nov 2019 09:22:02 GMT, Geoff wrote: > On Tue, 26 Nov 2019 09:22:02 GMT, Ajit Ghaisas wrote: > >> On Tue, 26 Nov 2019 09:22:01 GMT, David Grieve wrote: >> >>> On Tue, 26 Nov 2019 09:22:01 GMT, Ajit Ghaisas wrote: >>> On Tue, 19 Nov 2019 14:29:16 GMT, David Grieve wrote: > On Tue, 19 Nov 2019 10:48:52 GMT, Ajit Ghaisas > wrote: > >> On Fri, 15 Nov 2019 09:14:04 GMT, Ajit Ghaisas >> wrote: >> >>> On Thu, 14 Nov 2019 18:33:05 GMT, Kevin Rushforth >>> wrote: >>> On Tue, 12 Nov 2019 16:45:04 GMT, Ajit Ghaisas 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:
Re: RFR: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
On Tue, 12 Nov 2019 16:45:04 GMT, Ajit Ghaisas 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
The fix itself looks good and is a much safer approach than the previous one.
I've done a fair bit of testing and can see no regressions as a result of this
fix. I did find one unrelated issue while testing (a visual bug introduced back
in JDK 10) that I will file separately.
The test is pretty close, but still needs a few changes. The main problem is
that it does not catch the performance problem, meaning if you run it without
the fix, it will still pass. Your test class extends
`javafx.application.Application`, which can cause problems, since JUnit will
create a new instance of the test class that is different from the one created
by the call to `Application.launch` in the `@BeforeClass` method. This in turn
leads to the `rootPane` instance used by the test method being different from
the one used as the root of the visible scene. The fix is to use a separate
nested (static) subclass of Application, and make the rootPane field a static
field. I have left inline comments for this and a few other things I noticed.
tests/system/src/test/java/test/javafx/scene/QuadraticCssTimeTest.java line 63:
> 62:
> 63: public class QuadraticCssTimeTest extends Application {
> 64:
Remove `extends Application`
tests/system/src/test/java/test/javafx/scene/QuadraticCssTimeTest.java line 66:
> 65: static private CountDownLatch startupLatch;
> 66: static private Stage stage;
> 67: private BorderPane rootPane = new BorderPane();
Minor: our convention is `private static`.
tests/system/src/test/java/test/javafx/scene/QuadraticCssTimeTest.java line 67:
>
Re: [Approved] RFR: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
On Tue, 12 Nov 2019 16:45:04 GMT, Ajit Ghaisas 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 Approved by dgrieve (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/34
Re: [Rev 01] RFR: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
The pull request has been updated with additional changes. Added commits: - 2054da4c: Address review comments on test - 4dade6e5: Simpler fix + System test corrections - bd4a306a: Revert commit1 and commit 2 Changes: - all: https://git.openjdk.java.net/jfx/pull/34/files - new: https://git.openjdk.java.net/jfx/pull/34/files/12ea8220..2054da4c Webrevs: - full: https://webrevs.openjdk.java.net/jfx/34/webrev.01 - incr: https://webrevs.openjdk.java.net/jfx/34/webrev.00-01 Issue: https://bugs.openjdk.java.net/browse/JDK-8193445 Stats: 253 lines in 6 files changed: 132 ins; 104 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 PR: https://git.openjdk.java.net/jfx/pull/34
Re: RFR: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
On Tue, 19 Nov 2019 14:29:16 GMT, David Grieve
wrote:
> On Tue, 19 Nov 2019 10:48:52 GMT, Ajit Ghaisas wrote:
>
>> On Fri, 15 Nov 2019 09:14:04 GMT, Ajit Ghaisas wrote:
>>
>>> On Thu, 14 Nov 2019 18:33:05 GMT, Kevin Rushforth wrote:
>>>
On Tue, 12 Nov 2019 16:45:04 GMT, Ajit Ghaisas
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:
Re: RFR: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
On Tue, 19 Nov 2019 10:48:52 GMT, Ajit Ghaisas wrote:
> On Fri, 15 Nov 2019 09:14:04 GMT, Ajit Ghaisas wrote:
>
>> On Thu, 14 Nov 2019 18:33:05 GMT, Kevin Rushforth wrote:
>>
>>> On Tue, 12 Nov 2019 16:45:04 GMT, Ajit Ghaisas 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
Re: RFR: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
On Fri, 15 Nov 2019 09:14:04 GMT, Ajit Ghaisas wrote:
> On Thu, 14 Nov 2019 18:33:05 GMT, Kevin Rushforth wrote:
>
>> On Tue, 12 Nov 2019 16:45:04 GMT, Ajit Ghaisas 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`.
>>
>>
Re: RFR: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
On Thu, 14 Nov 2019 18:33:05 GMT, Kevin Rushforth wrote:
> On Tue, 12 Nov 2019 16:45:04 GMT, Ajit Ghaisas 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
Re: RFR: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
On Tue, 12 Nov 2019 16:45:04 GMT, Ajit Ghaisas 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.
Re: RFR: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
On Thu, 14 Nov 2019 15:02:40 GMT, Dean Wookey wrote: > On Thu, 14 Nov 2019 14:27:24 GMT, Kevin Rushforth wrote: > >> On Thu, 14 Nov 2019 12:34:56 GMT, Kevin Rushforth wrote: >> >>> On Thu, 14 Nov 2019 11:33:24 GMT, Dean Wookey wrote: >>> On Wed, 13 Nov 2019 19:10:45 GMT, David Grieve wrote: > On Tue, 12 Nov 2019 16:55:45 GMT, Ajit Ghaisas > wrote: > >> On Tue, 12 Nov 2019 16:52:54 GMT, Ajit Ghaisas >> wrote: >> >>> On Tue, 12 Nov 2019 16:45:04 GMT, Ajit Ghaisas >>> 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
Re: RFR: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
On Thu, 14 Nov 2019 14:27:24 GMT, Kevin Rushforth wrote: > On Thu, 14 Nov 2019 12:34:56 GMT, Kevin Rushforth wrote: > >> On Thu, 14 Nov 2019 11:33:24 GMT, Dean Wookey wrote: >> >>> On Wed, 13 Nov 2019 19:10:45 GMT, David Grieve >>> wrote: >>> On Tue, 12 Nov 2019 16:55:45 GMT, Ajit Ghaisas wrote: > On Tue, 12 Nov 2019 16:52:54 GMT, Ajit Ghaisas > wrote: > >> On Tue, 12 Nov 2019 16:45:04 GMT, Ajit Ghaisas >> 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 >>>
Re: RFR: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
On Thu, 14 Nov 2019 12:34:56 GMT, Kevin Rushforth wrote: > On Thu, 14 Nov 2019 11:33:24 GMT, Dean Wookey wrote: > >> On Wed, 13 Nov 2019 19:10:45 GMT, David Grieve >> wrote: >> >>> On Tue, 12 Nov 2019 16:55:45 GMT, Ajit Ghaisas wrote: >>> On Tue, 12 Nov 2019 16:52:54 GMT, Ajit Ghaisas wrote: > On Tue, 12 Nov 2019 16:45:04 GMT, Ajit Ghaisas > 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
Re: RFR: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
On Thu, 14 Nov 2019 11:33:24 GMT, Dean Wookey wrote: > On Wed, 13 Nov 2019 19:10:45 GMT, David Grieve > wrote: > >> On Tue, 12 Nov 2019 16:55:45 GMT, Ajit Ghaisas wrote: >> >>> On Tue, 12 Nov 2019 16:52:54 GMT, Ajit Ghaisas wrote: >>> On Tue, 12 Nov 2019 16:45:04 GMT, Ajit Ghaisas 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
Re: RFR: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
On Wed, 13 Nov 2019 19:10:45 GMT, David Grieve wrote: > On Tue, 12 Nov 2019 16:55:45 GMT, Ajit Ghaisas wrote: > >> On Tue, 12 Nov 2019 16:52:54 GMT, Ajit Ghaisas wrote: >> >>> On Tue, 12 Nov 2019 16:45:04 GMT, Ajit Ghaisas 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
Re: RFR: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
On Tue, 12 Nov 2019 16:55:45 GMT, Ajit Ghaisas wrote: > On Tue, 12 Nov 2019 16:52:54 GMT, Ajit Ghaisas wrote: > >> On Tue, 12 Nov 2019 16:45:04 GMT, Ajit Ghaisas 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. PR: https://git.openjdk.java.net/jfx/pull/34
Re: RFR: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
On Tue, 12 Nov 2019 16:52:54 GMT, Ajit Ghaisas wrote: > On Tue, 12 Nov 2019 16:45:04 GMT, Ajit Ghaisas 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. PR: https://git.openjdk.java.net/jfx/pull/34
Re: RFR: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
On Tue, 12 Nov 2019 16:45:04 GMT, Ajit Ghaisas 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 PR: https://git.openjdk.java.net/jfx/pull/34
RFR: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
**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 PR: https://git.openjdk.java.net/jfx/pull/34
