JDK-8090462 : CSS performance: Link style helpers together to avoid going through parent chain looking for parent style helpers
  • Type: Enhancement
  • Component: javafx
  • Sub-Component: controls
  • Affected Version: 8,9
  • Priority: P4
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2013-06-04
  • Updated: 2020-01-17
  • Resolved: 2017-10-05
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 10
10Fixed
Related Reports
Blocks :  
JDK-8090757 - Improve JavaFX embedded performance
Relates :  
JDK-8187955 - CssStyleHelper : recursion in isUserSetFont() can be avoided
Relates :  
JDK-8177635 - Optimise CSS lookup resolution
Relates :  
JDK-8234877 - Changing CSS style of Button sometimes fails to take effect
Relates :  
JDK-8237469 - Inherited styles don't update when node is moved
Description
Typical code in CssStyleHelper is 

Styleable parent = node.getStyleableParent();
CssStyleHelper styleHelper = this;
while (parent != null) {
   styleHelper = parent instanceof Node ? ((Node)parent).styleHelper : null;
   if (styleHelper != null) break;
   parent = parent.getStyleableParent();
}

If you are deep in the scene graph, it may be that you get to the Control and have to go through several parents to get to the root style helper. If CssStyleHelper had a ref to a parent CssStyleHelper, then this code is just. 

CssStyleHelper styleHelper = this.getParentStyleHelper();

Maybe parent isn't such a good name for it. Maybe "upstream"?
Comments
Changeset: 5d95c6501d19 Author: aghaisas Date: 2017-10-05 17:18 +0530 URL: http://hg.openjdk.java.net/openjfx/10-dev/rt/rev/5d95c6501d19 8090462: CSS performance: Link style helpers together to avoid going through parent chain looking for parent style helpers Reviewed-by: dgrieve, jgiles
05-10-2017
JDK-8187955 has been created to address the review comment from David.
26-09-2017
+1, and please file a bug for the final comment from David regarding "isUserSetFont walks the parent chain because of the recursion. I think this can be fixed by defaulting the CacheContainer.fontProp instead of allowing it to be null. But this should probably be the subject of a different bug."
13-09-2017
Received via email a +1 from David Grieve.
07-09-2017
Thanks for the review. Below are my comments - >> My original thought was to pass information into createStyleHelper so that it would never have to traverse the parent chain. Maybe createStyleHelper could take a "Styleable[] styleableParents" argument, and a StyleableProperty<Font> argument for resolving font-relative sizes, for instance. This might also help fix the problem where some elements are not Nodes, but are Styleable (there are "TODO: this should work on Styleable, not Node" comments here and there in the code). [Ajit] I did see this proposal being discussed in JDK-8177635. I intend to work on it sooner. >> * If the StyleHelper can be reused, do you really need to lookup the firstStyleableAncestor again? Won't it always be the same? [Ajit] Initially, I also thought the same; but, when I checked with a TabPane performance benchmark test, it breaks without this change. I think, recomputing the first Styleable ancestor should be OK in this case. >> * Would it be better to make firstStyleableAncestor a member of CacheContainer, since CacheContainer does that parent loop anyway? [Ajit] There are two types of loops in CssStyleHelper class. Type-1 - this loop traverses till it finds first Styleable node having a valid stylehelper Type-2 - this loop traverses all parents of the node till null and accumulates information along the way to be used lateron (such as smapIds) What I am addressing in this fix is of Type-1 loop. The loop in CacheContainer is of Type-2 >>* Maybe CacheContainer could keep a Styleable[] for all styleable parents. I'm looking at updateParentTriggerStates, for example, and am thinking that holding on to Styleable[] parents would eliminate the parent.getStyleableParent() call in there. In other words, is there a way to have it so we only have to call getStyleableParent() in one place (do that loop only once)? I think CacheContainer might be that place, but again, my memory on the details are foggy. And one must be careful to avoid leaks from holding references as nodes are removed from the scenegraph. [Ajit] The suggestion to keep Styleable[] in CacheContainer could be used to address Type-2 loops. I am a bit skeptical about how helpful this solution would be as - 1) We still need to iterate through Styleable[] and accumulate information 2) We need to keep Styleable[] updated whenever nodes are re-parented 3) Keeping Styleable[] array per node would definitely cause a lot of duplicate references What I think is - existing code having Type-2 loops is good enough as it does (1) and avoids (2) and (3) of above points. >>* findFirstStyleableAncestor can return null. Is this accounted for in the code that accesses firstStyleableAncestor? [Ajit] Yes. There are null checks before using firstStyleableAncestor - either directly or indirectly via newly added method getStyleHelper(node) >>* isUserSetFont walks the parent chain because of the recursion. I think this can be fixed by defaulting the CacheContainer.fontProp instead of allowing it to be null. But this should probably be the subject of a different bug. [Ajit] This would probably a tweak and as suggested I would like to address it separately. The proposed webrev still holds good. Kindly let me know if you have other concerns regarding it.
16-08-2017
My original thought was to pass information into createStyleHelper so that it would never have to traverse the parent chain. Maybe createStyleHelper could take a "Styleable[] styleableParents" argument, and a StyleableProperty<Font> argument for resolving font-relative sizes, for instance. This might also help fix the problem where some elements are not Nodes, but are Styleable (there are "TODO: this should work on Styleable, not Node" comments here and there in the code). Here are some comments on the proposed patch, keeping in mind I haven't been in this code in a long time. * If the StyleHelper can be reused, do you really need to lookup the firstStyleableAncestor again? Won't it always be the same? * Would it be better to make firstStyleableAncestor a member of CacheContainer, since CacheContainer does that parent loop anyway? * Maybe CacheContainer could keep a Styleable[] for all styleable parents. I'm looking at updateParentTriggerStates, for example, and am thinking that holding on to Styleable[] parents would eliminate the parent.getStyleableParent() call in there. In other words, is there a way to have it so we only have to call getStyleableParent() in one place (do that loop only once)? I think CacheContainer might be that place, but again, my memory on the details are foggy. And one must be careful to avoid leaks from holding references as nodes are removed from the scenegraph. * findFirstStyleableAncestor can return null. Is this accounted for in the code that accesses firstStyleableAncestor? * isUserSetFont walks the parent chain because of the recursion. I think this can be fixed by defaulting the CacheContainer.fontProp instead of allowing it to be null. But this should probably be the subject of a different bug.
10-08-2017
I counted the number of times Node.getStyleableParent() gets invoked with attached two test programs. Please note that - the tests are random and not designed specifically for this issue. Main.java -------------- Number of invocations without this patch : 1190 Number of invocations with this patch : 973 Test.java ------------ Number of invocations without this patch : 13512 Number of invocations with this patch : 8607 This shows there is reduction in number of times Node.getStyleableParent() is invoked.
10-08-2017
Thanks David for identifying this tweak. I have made an attempt to implement it. Here is the fix : http://cr.openjdk.java.net/~aghaisas/fx/e8090462/webrev.2/ Request to review.
10-08-2017
This should probably be a tweak.
18-12-2014