8386663: Stylesheet/StyleClass list should be lazily initialized - #2191
8386663: Stylesheet/StyleClass list should be lazily initialized#2191Maran23 wants to merge 6 commits into
Conversation
|
👋 Welcome back mhanl! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
The total number of required reviews for this PR has been set to 2 based on the presence of this label: |
Webrevs
|
|
Today I learned about https://github.com/openjdk/jol to measure the exact memory footprint of objects and used those insights to measure the memory gain of some apps - results in the PR description. Memory stats: With |
The stats you showed lead me to a different conclusion: style class is usually used, and most things do have styles associated with them, including containers; most containers I create definitely have a style, they just don't have one by default. I fully agree on the StyleSheets property. |
Yes. It might still be a good idea though, but I agree that it seems to have the biggest advantage for Chart/ It would be interesting to also measure an app that uses many I still plan to test with some more application, but currently not at home so need to setup some of the projects on my laptop first. |
There's https://github.com/openjdk/jfx/tree/master/tests/performance/3DLighting. |
|
I tested this in an app with lots of 2D shapes:
|
|
I think this effort is similar to the idea of stashing rarely used attributes into a hidden hashmap. @Maran23 would you like to cooperate (review, etc.) with me on this? Once we get through with the rare properties, |
andy-goryachev-oracle
left a comment
There was a problem hiding this comment.
I like it.
| */ | ||
|
|
||
| if (styleClassNames.size() < selectorStyleClassNames.size()) { | ||
| if (styleClassNames == null || styleClassNames.size() < selectorStyleClassNames.size()) { |
There was a problem hiding this comment.
would it make more sense, instead of adding the null check here, move it up to after L278, to avoid unnecessary check?
There was a problem hiding this comment.
I think this will not really matter.
Just made the change locally to check what looks/feels better and personally it felt better in matchesStyleClasses.
Because it bascially reads like "well, a null styleClass will not match anything, so just return". But just my personally feeling here really.
Yes. I think we could probably reuse and adjust the |
|
Added two more Open Source projects to the benchmark section. |
TwoLevelFocusListBehavior was very weirdly relying on the toString() behavior
…Stylesheet/StyleClass-list-should-be-lazily-initialized
|
Seems like the latest GHA catched a race condition: |
It's a known issue: https://bugs.openjdk.org/browse/JDK-8357459 |
andy-goryachev-oracle
left a comment
There was a problem hiding this comment.
Please document the toString() format change in the JBS ticket and this PR description.
I don't know whether a CSR is needed, since the actual format has not been documented, but the fact that multiple tests fail with this change indicate that the applications that rely on the exact string (and not on .contains()) will break.
But note that the tests did check for whole A normal |
andy-goryachev-oracle
left a comment
There was a problem hiding this comment.
Mentioning this change in the toString() behavior in the JBS ticket should be sufficient, I think.
I agree. No need for a CSR (nor a release note). |
| * @param node the node | ||
| * @return the styleClass or null | ||
| */ | ||
| public static List<String> getStyleClassOrNull(Node node) { |
There was a problem hiding this comment.
Most (all?) helper classes that implement the private accessor pattern don't have documentation, because it is implied that it's a simple forwarding method.
There was a problem hiding this comment.
My thought was to explain why this method exists. Do you have a suggestion, or should we just remove it entirely?
|
It doesn't look like this will be ready before the jfx27 RDP1 fork, but in the unlikely event that it is, wait until after the fork. Any CSS lazy eval fix has a relatively high risk of regression, so this is the sort of change that is better earlier in the release cycle. |
That would be my suggestion, too. (On that, I wouldn't want to merge #1970, #2175 or #2167 right before RDP1 either) |
This PR is an optimization for
Node.styleClassandParent.stylesheets.Instead of always initializing both properties with an empty list, we are creating (therefore allocating) the list on the first access instead.
Similar to many other lazy properties.
Why?
Parent.getStylesheets()is very rarely used by developers and JavaFX code. So this list is very often completely unused and empty. Most developers usually only add stylesheets at theSceneNode.getStyleClass()is usually not used (empty) for layout containers such asPaneorGroupor shapes. A JavaFX App usually consists of a good amount of such containers (or shapes)So that our CSS code is not initializing both lists on access, I added related
NodeHelperandParentHelpermethods to returnnullwhen both lists were not initialized and therefore used. Otherwise we return the list as before.This will save us some memory and allocation, which is both good for the memory consumption but also for
Node/Parentcreation (time).Added documentation and tests. Will do some measurements with some apps very soon and attach it here.
Benchmarks
I wrote a very small scene graph analyzer snippet to measure the memory gain.
Feel free to test this on your own apps!
SceneGraphAnalyzerhere: SceneGraphAnalyzerSceneand press the shortcutF12when all of the UI is loaded:--add-opens javafx.graphics/javafx.scene=yourappProjects
1.1. Test with an Icon Library at JFXCentral
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/2191/head:pull/2191$ git checkout pull/2191Update a local copy of the PR:
$ git checkout pull/2191$ git pull https://git.openjdk.org/jfx.git pull/2191/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 2191View PR using the GUI difftool:
$ git pr show -t 2191Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/2191.diff
Using Webrev
Link to Webrev Comment