8322964: Optimize performance of CSS selector matching

[hjohn]

Improves performance of selector matching in the CSS subsystem. This is done by using custom set implementation which are highly optimized for the most common cases where the number of selectors is small (most commonly 1 or 2). It also should be more memory efficient for medium sized and large applications which have many style names defined in various CSS files.

Due to the optimization, the concept of StyleClass, which was only introduced to assign a fixed bit index for each unique style class name encountered, is no longer needed. This is because style classes are no longer stored in a BitSet which required a fixed index per encountered style class.

The performance improvements are the result of several factors:

• Less memory use when only very few style class names are used in selectors and styles from a large pool of potential styles (a BitSet for potentially 1000 different style names needed 1000 bits (worst case) as it was not sparse).
• Specialized sets for small number of elements (0, 1, 2, 3-9 and 10+)
• Specialized sets are append only (reduces code paths) and can be made read only without requiring a wrapper
• Iterator creation is avoided when doing containsAll check thanks to the inverse function isSuperSetOf
• Avoids making a copy of the list of style class names to compare (to convert them to StyleClass and put them into a Set) -- this copy could not be cached and was always discarded immediately after...

The overall performance was tested using the JFXCentral application which displays about 800 nodes on its start page with about 1000 styles in various style sheets (and which allows to refresh this page easily).

On JavaFX 20, the fastest refresh speed was 121 ms on my machine. With the improvements in this PR, the fastest refresh had become 89 ms. The speed improvement is the result of a 30% faster Scene#doCSSPass, which takes up the bulk of the time to refresh the JFXCentral main page (about 100 ms before vs 70 ms after the change).

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (3 reviews required, with at least 1 Reviewer, 2 Authors)

Issue

• JDK-8322964: Optimize performance of CSS selector matching (Enhancement - P4)

Reviewers

• Kevin Rushforth (@kevinrushforth - Reviewer)
• Andy Goryachev (@andy-goryachev-oracle - Reviewer)
• Michael Strauß (@mstr2 - Committer)
• Marius Hanl (@Maran23 - Committer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1316/head:pull/1316
$ git checkout pull/1316

Update a local copy of the PR:
$ git checkout pull/1316
$ git pull https://git.openjdk.org/jfx.git pull/1316/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1316

View PR using the GUI difftool:
$ git pr show -t 1316

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1316.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back jhendrikx! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[mlbridge]

Webrevs

• 08: Full - Incremental (e4e3a7ce)
• 07: Full - Incremental (2392d8db)
• 06: Full - Incremental (16da6eb0)
• 05: Full - Incremental (dc59c4db)
• 04: Full - Incremental (c388f23d)
• 03: Full (a33f797a)
• 02: Full - Incremental (5201cd2d)
• 01: Full - Incremental (6c3271b0)
• 00: Full (8da98928)

[nlisker]

I just scrolled quickly through the changes and left some comments,

If deprecation for removal is part of the PR the consequences will need to be discussed. Are the for-removal classes and methods not widely used?

[kevinrushforth]

As @nlisker notes, the implications of any deprecation will need to be discussed. In particular, we need to consider what the consequences are for existing applications. If any of them might be used by apps, then simple deprecation (rather than deprecating for removal) might be best.

/reviewers 3
/csr

[openjdk]

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 3 (with at least 1 Reviewer, 2 Authors).

[openjdk]

@kevinrushforth has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@hjohn please create a CSR request for issue JDK-8322964 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

[kevinrushforth]

Left a couple inline comments.

[hjohn]

As @nlisker notes, the implications of any deprecation will need to be discussed. In particular, we need to consider what the consequences are for existing applications. If any of them might be used by apps, then simple deprecation (rather than deprecating for removal) might be best.

The methods cannot be reached easily even though they are public. It would involve creating a Selector using Selector#createSelector, and then casting it to SimpleSelector to get access to its public methods that are not inherited from Selector. SimpleSelector itself does not have a public constructor, and the type SimpleSelector is not returned anywhere directly (it is always just Selector in the public API).

The only use of one of the deprecated methods is to allow access for the internal class SelectorPartitioning which indeed does an instanceof to reach it. The other method is not used anywhere, and seems to have been an attempt to make the selectors available in the same form as you'd find them on nodes Styleable#getStyleClass (which is a List), but is hard to reach (probably an oversight that it got exposed in the final API).

Google searches turn up no hits in 3rd party code for either of these.

I've posted on the mailing list to discuss it further.

[mlbridge]

Mailing list message from Dirk Lemmermann on openjfx-dev:

I truly love the fact that you guys are starting to use JFXCentral for benchmarking / testing :-) If anyone encounters things bad for performance unrelated to JavaFX itself but caused by JFXCentral then please let me know.

Dirk

[mlbridge]

Mailing list message from John Hendrikx on openjfx-dev:

It's a very nice application, I had never seen it before.? As the
original bug was reported against it that caused a performance
regression, I started using it to also test a potential performance
improvement.? Although I think it does a back-end call every time I
"refresh", so I might have been hitting your servers a bit during that
period.

I'll let you know if I discover that's off.? Only thing I did was
disable a piece of code that was causing a stack trace to be logged each
time (a bug in one of the components, fx tray icon, I think).

--John

On 04/01/2024 22:49, Dirk Lemmermann wrote:

I truly love the fact that you guys are starting to use JFXCentral for benchmarking / testing :-) If anyone encounters things bad for performance unrelated to JavaFX itself but caused by JFXCentral then please let me know.

Dirk

[Maran23]

I can confirm that most of the code in my application uses 1-2 styleclasses.
Since all controls have one styleclass by default (some even more, think about TextInputControl with text-input -> TextField with text-field), does it make sense to also implement a specialized Triple class?

We also often add 2 more styleclasses, so 3 style classes seems like a usecase that happens quite often.
I also can confirm that 4 styleclasses or even more are very rare.
I only found one: (.label + 3 added from us)

[hjohn]

Thanks for having a look, it's good to have some more use cases from other applications! The sets are used both for the style classes on Nodes but also the set of style classes in CSS selectors (multiple style classes in selectors are even more rare I think than multiple style classes on Nodes).

I think from a performance perspective, Duo and Hashless are likely very closely matched, but I can do some testing in that area. The reason I say this is that I only got a very minor boost from including Duo, but kept it because it is a bit more memory efficient.

If you want, I'm also curious about how many styles are used in your application (you can see this by looking at the size of StyleClassSet#styleClasses after all styles are loaded).

[Maran23]

I see, thanks for checking that out.

I checked the application, here is the result:
After starting the app:

After using it a little bit:

---

I also checked out Scenebuilder after using it a little bit:

[hjohn]

Thanks, it looks like a bit less classes are in use when compared to JFXCentral (which has about 1000), but still quite a lot. With BitSet that means if style classes are referenced that happen to have a high bit set, it creates an array to hold 400 bits (using 8 longs). When few style classes are active, the sets provided by FixedCapacitySet will be often smaller and (due to the other optimizations as well) faster. 8 longs is sufficient to hold 8 or 16 style name references depending on the JVM (32/64 bit, compressed pointers y/n).

[bridgekeeper]

@hjohn This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[hjohn]

I think this PR now no longer needs a CSR.

[andy-goryachev-oracle]

looks good to me.

how do we undo the csr command?

[hjohn]

I think its /csroff or something...

[hjohn]

/csroff

[openjdk]

@hjohn Unknown command csroff - for a list of valid commands use /help.

[hjohn]

/csr unneeded

[openjdk]

@hjohn only Reviewers can determine that a CSR is not needed.

[kevinrushforth]

I was about to reply, but I see that the Skara bot beat me to it. :)

I'll double-check later (or Andy can), and if there are no public API implications, issue the /csr unneeded command.

[andy-goryachev-oracle]

@hjohn is right - no public API changes remain in this PR.

[andy-goryachev-oracle]

/csr unneeded

[openjdk]

@andy-goryachev-oracle determined that a CSR request is not needed for this pull request.

[openjdk]

@hjohn This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8322964: Optimize performance of CSS selector matching

Reviewed-by: kcr, angorya, mstrauss, mhanl

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 89 new commits pushed to the master branch:

• 94aa2b6: 8332313: Update code review guidelines
• b685db2: 8313138: Scrollbar Keyboard enhancement
• b5fe362: 8330590: TextInputControl: previous word fails with Bhojpuri characters
• 9dc4aa2: 8324327: ColorPicker shows a white rectangle on clicking on picker
• ebe3689: 8332251: javadoc: incorrect method references in Region and PopupControl
• d7ab551: 8332328: [GHA] GitHub Actions build fails on Linux: unable to find gcc-13
• a7627fa: 8329821: [Linux] When using i3 WM, menus are incorrectly sized
• 88aad93: 8279140: ComboBox can lose selected value on item change via setAll
• 97b1402: 8331748: Update libxml2 to 2.12.6
• 581e3a7: 8329011: Update SQLite to 3.45.3
• ... and 79 more: https://git.openjdk.org/jfx/compare/b43c4edf7590429fd051d1b0e2ccb6dd49a10b8b...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[bridgekeeper]

@hjohn This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[hjohn]

I think this is ready to be integrated. Let me know if there is anything still missing.

[mstr2]

Looks good to me.

[Maran23]

Is this a possible idea for the future?

[hjohn]

Yes, once this class is moved to private API (see #1333) then this method can be removed. The SimpleSelector class is already deprecated for removal since 23.

The method is almost impossible to reach (you'd have to cast to SimpleSelector), but removing it as part of this PR would have required a CSR making this harder to integrate.

[Maran23]

Code looks good to me. As mentioned above, I tested it already and everything was good, so I'm certain that there is no regression here.

I'm generally not a big fan of 'reimplementing' Collections, but I can see why it is needed here and it also makes sense, especially for something like CSS which needs to be fast.
Something I saw as well in Swing (just check the ArrayTable class 😄 ).

I wonder if we may want to add some tests for the FixedCapacitySet?

[kevinrushforth]

I'm in the process of testing this. I'll do that and report back.

[kevinrushforth]

Code looks good. Testing looks good.

[hjohn]

Code looks good to me. As mentioned above, I tested it already and everything was good, so I'm certain that there is no regression here.

I'm generally not a big fan of 'reimplementing' Collections, but I can see why it is needed here and it also makes sense, especially for something like CSS which needs to be fast. Something I saw as well in Swing (just check the ArrayTable class 😄 ).

I'm not a fan either, especially because custom collection implementations when referred to by their interface may sneak their way through the various classes and eventually be returned to users. Any deviation from the collection contracts then suddenly is a bug users may encounter. I would have done this with plain HashSet (or Set.of) were it not for the fact that a lot of performance gains were possible due to the limited way FX is using these sets.

I wonder if we may want to add some tests for the FixedCapacitySet?

Yeah, now that it is more likely that this will make it into FX, I will add a small set of unit tests for this class.

[kevinrushforth]

I wonder if we may want to add some tests for the FixedCapacitySet?

Yeah, now that it is more likely that this will make it into FX, I will add a small set of unit tests for this class.

Since this PR is ready to integrate, I think it would be fine to file a new test bug for the additional tests if you like. If you prefer to add the new tests now, that's fine, too (we can re-review it).

[hjohn]

I wonder if we may want to add some tests for the FixedCapacitySet?

Yeah, now that it is more likely that this will make it into FX, I will add a small set of unit tests for this class.

Since this PR is ready to integrate, I think it would be fine to file a new test bug for the additional tests if you like. If you prefer to add the new tests now, that's fine, too (we can re-review it).

I'm fine integrating this as-is and adding a test soon after. I will leave this over the weekend to give others time to review.

Also some clarification on the contributing rules: "all Reviewers who have requested the chance to review have done so" -- does the indication at the top right of the PR count towards this or should it be a comment? :) In the first case, @nlisker and @arapte, please indicate if you wish to review this still.

[nlisker]

The code looks good. I didn't test it, but I'm fine with integrating.

[tsayao]

I built it and tested on a retail self-checkout app that uses a lot of css. All good.

[kevinrushforth]

Also some clarification on the contributing rules: "all Reviewers who have requested the chance to review have done so" -- does the indication at the top right of the PR count towards this or should it be a comment? :) In the first case, @nlisker and @arapte, please indicate if you wish to review this still.

If someone wants you to wait for them, they should make it clear by adding a comment. Also if someone has given substantive feedback, but hasn't (re)approved, it's good to give them a change to review.

@arapte can add a comment if he wants to review, otherwise go ahead and integrate on Monday.