-
Notifications
You must be signed in to change notification settings - Fork 505
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8193445: JavaFX CSS is applied redundantly leading to significant performance degradation #34
Conversation
👋 Welcome back aghaisas! A progress list of the required criteria for merging this PR into |
Webrevs
|
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 DeanWookey/openjdk-jfx@65a1ed8), I'm a bit confused. Doesn't commit 12ea822 essentially add another reapplyCSS() above the scenesChanged call on line 1074? I'm guessing this works because reapplyCss() (different from reapplyCSS()) sets cssFlag to UPDATE, which then means subsequent calls to reapplyCSS() don't call reapplyCss()? Does this leave the whole tree with the CssFlags.REAPPLY set instead of CssFlags.UPDATE as they would be without these changes? I'm not sure about the impact of that. I'm also curious whether commit 1 is even necessary with commit 2. |
The above raises good questions about the effect of adding a call to Regarding whether commit 2 by itself would be sufficient, my initial guess was that it wouldn't be. However, I tested it, and it does in fact improve performance of the test case. Very interesting. |
Looking at this a little further, the change from commit 2, to call
The first of the above appears to be both necessary and sufficient to both maintain correctness while still avoiding the n^2 application of CSS in the case of adding a deep scene graph. If this is in fact the case, then I think the fix can be simplified even further by moving the following call from its current location (after
in which case the changes to The following patch, by itself (with nothing from commit 1) might be sufficient:
We will need to do a more complete evaluation as to whether this inversion of calling reapplyCSS on the parent before its children has any side effects, and also whether there are any of the performance improvements from the original fix that aren't covered here. |
I think inverting the call is fine. That's what I did in my fix (DeanWookey/openjdk-jfx@65a1ed8) and we've been testing that out thoroughly for over a year. It's as if you are adding nodes 1 by 1 to the scene graph, something we know works and is fast. My change tries to emulate that more accurately to avoid side effects. Theoretically, we should be able to do better when many nodes are added at once because we have all the information upfront. The one side effect I can see by only applying commit 2 is that the first call of reapplyCSS() calls reapplyCss on every node in the tree and that sets the cssFlag = CssFlags.UPDATE;. The subsequent calls will hit this in reapplyCSS():
and return without doing all the unnecessary work. As a result however, instead of leaving with cssFlag = CssFlags.UPDATE, all the nodes leave with CssFlags.REAPPLY. That might cause an unnecessary css pass later? Doing it in the order it happens now, that check for the update flag shouldn't be true because its bottom up. |
I think the order of calls in invalidatedScenes is correct. The scene graph plumbing needs to be done before CSS is applied since a node's style can depend on the styles of its parent. The issue is really calling reapplyCss(). In the case where a child is added to a parent, the child's parent property is invalidated which should call reapplyCSS(), which should (in turn) call reapplyCss(). Next the invalidatedScenes call happens and the whole dance starts again. (See Parent children.onChanged). Perhaps short-circuiting the call to reapplyCss() from the reapplyCSS() method is the thing to do. Since CSS is reapplied from the top down, my parent's cssFlag should be CLEAN before reapplyCss() method is called from reapplyCSS(). It has been a long time since I've been in this code. I'm going to look at this some more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
tests/system/src/test/java/test/robot/javafx/scene/CSSPerf_JDK8193445Test.java
Outdated
Show resolved
Hide resolved
tests/system/src/test/java/test/robot/javafx/scene/CSSPerf_JDK8193445Test.java
Outdated
Show resolved
Hide resolved
tests/system/src/test/java/test/robot/javafx/scene/CSSPerf_JDK8193445Test.java
Outdated
Show resolved
Hide resolved
It is a good observation about cssFlag. I have not seen any side effect with the limited testing that I have done. It may be possible that the "unnecessary css pass later" scenario is not covered by the test cases that we have. |
This comment from @dsgrieve got me interested. I checked the test code JDK-8151756 with cssFlags logged, it became evident that the cssFlag gets set to DIRTY_BRANCH for every parent and reapplyCss() gets invoked for each of the children. This is the exact redundant processing. Test from JDK-8151756 with additional one level of Node hierarchy. Parent1<--Parent2<--Parent3<--Rectangle (leaf child) Log from test program ---- REAPPLY_CSS called for : VBox@1d9e402b ----- CssFlags.CLEAN Proposed New Fix :I added a simple check to avoid reapplyCss() call for each Node with DIRTY_BRANCH cssFlag. Here is the patch -
With this fix - REAPPLY_CSS called for : VBox@36d24c70 ----- CssFlags.CLEAN I verified that all graphics/controls unit tests & all system tests pass with this fix. |
@aghaisas You can avoid the call to notifyParentsOfInvalidatedCSS in the case where the flag is DIRTY_BRANCH. I like the looks of this. From the 10,000 foot view, when a node's parent changes, or a node's scene changes, CSS should be reapplied. This is exactly what 'if (sceneChanged) reapplyCSS()' says, and what happens in parent property's invalidated method. All of the optimizations (do I really need to reapply css?) happen elsewhere, so I like this solution better than passing a boolean around (the original patch). |
Thanks @dsgrieve for having a look. I have updated the PR as suggested to avoid call to notifyParentsOfInvalidatedCSS in case the flag is DIRTY_BRANCH. Kindly review. Limited testing shows that this fix holds up good. |
Trying to run this, but have to build on Windows. Ugh! |
Request to @DeanWookey, @tomsontom, @swpalmer - can you please confirm if this fix helps your application or tests? |
Does the performance problem addressed here relate to reducing heap space allocations? My profiler pointed me at this issue, and around the same time I'm seeing |
I did a quick check w.r.t. memory using profiler. This patch significantly reduces the heap allocations as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
tests/system/src/test/java/test/javafx/scene/QuadraticCssTimeTest.java
Outdated
Show resolved
Hide resolved
tests/system/src/test/java/test/javafx/scene/QuadraticCssTimeTest.java
Outdated
Show resolved
Hide resolved
tests/system/src/test/java/test/javafx/scene/QuadraticCssTimeTest.java
Outdated
Show resolved
Hide resolved
tests/system/src/test/java/test/javafx/scene/QuadraticCssTimeTest.java
Outdated
Show resolved
Hide resolved
tests/system/src/test/java/test/javafx/scene/QuadraticCssTimeTest.java
Outdated
Show resolved
Hide resolved
tests/system/src/test/java/test/javafx/scene/QuadraticCssTimeTest.java
Outdated
Show resolved
Hide resolved
@aghaisas This change can now be integrated. The commit message will be:
Since the source branch of this PR was last updated there have been 17 commits pushed to the
Since there are no conflicts, your changes will automatically be rebased on top of the above commits when integrating. If you prefer to do this manually, please merge
|
I am curious. Will∕Could this CSS performance improvement be backported to JavaFX 11? The bug report says only that it will be fixed in JavaFX 14. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
/integrate |
@aghaisas The following commits have been pushed to master since your change was applied:
Your commit was automatically rebased without conflicts. Pushed as commit 83eb0a7. |
Mailing list message from Ajit Ghaisas on openjfx-dev: Changeset: 83eb0a7 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
|
|
This has already been backported to FX 8 for Oracle's JDK 8u251. |
Will JavaFX 11 get the same treatment? |
Yes, this would be an excellent candidate to backport to JavaFX 11 (and one I was going to recommend). |
Issue :
https://bugs.openjdk.java.net/browse/JDK-8193445
Background :
The CSS performance improvement done in JDK-8151756 had to be backed out due to functional regressions reported in JDK-8185709, JDK-8183100 and JDK-8168951.
Refer to JDK-8183100 for more details on this backout.
Description :
This PR reintroduces the CSS performance improvement fix done in JDK-8151756 while addressing the functional regressions that were reported in JDK-8185709, JDK-8183100 and JDK-8168951.
For ease of review, I have made two separate commits -
Root Cause :
CSS performance improvement fix proposed in JDK-8151756 correctly avoids the redundant CSS reapplication to children of a Parent.
What was missed earlier in JDK-8151756 fix : "CSS reapplication to the Parent itself”.
This missing piece was the root cause of all functional regressions reported against JDK-8151756
Fix :
Fixed the identified root cause. See commit 2.
Testing :
In addition, testing by community with specific CSS performance / functionality will be helpful.
Progress
Issue
JDK-8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
Approvers