RFR: JDK-8322964 Optimize performance of CSS selector matching

[Nir Lisker]

On Thu, 4 Jan 2024 12:21:15 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

> 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).

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?

modules/javafx.graphics/src/main/java/com/sun/javafx/css/FixedCapacitySet.java line 81:

> 79:      * collection or making a read-only copy.
> 80:      */
> 81:     public abstract void freeze();

All the permitted subclasses have the same implementation of this method. Any reason not to pull up the `boolean frozen` field to this class and make this method concrete with `frozen = true;`?

modules/javafx.graphics/src/main/java/javafx/css/SimpleSelector.java line 81:

> 79:      * @deprecated for future removal, use {@link #getStyleClassNames()} instead
> 80:      */
> 81:     public List<String> getStyleClasses() {

Deprecating requires the `@Deprecate` annotation (possibly with setting `forRemoval = true`). The javadoc tag needs only to mention why (e.g., "no longer needed because...") and what to replace it with.

Same for the other deprecations.

modules/javafx.graphics/src/main/java/javafx/css/SimpleSelector.java line 82:

> 80:      */
> 81:     public List<String> getStyleClasses() {
> 82:         return Collections.unmodifiableList(new ArrayList<>(selectorStyleClassNames));

Is `List.copyOf` not good here?

modules/javafx.graphics/src/main/java/javafx/css/SimpleSelector.java line 291:

> 289: 
> 290:         if (matchOnStyleClass) {
> 291:             if (!matchesStyleClasses(styleable.getStyleClass())) {

Should the 2 `if` conditions not be combined instead of nested?

modules/javafx.graphics/src/main/java/javafx/css/SimpleSelector.java line 364:

> 362:             return false;
> 363:         }
> 364:         if (this.selectorStyleClassNames.equals(other.selectorStyleClassNames) == false) {

Maybe replace the `==false` with `!`.

modules/javafx.graphics/src/main/java/javafx/css/SimpleSelector.java line 383:

> 381:         hash = (id != null) ? 31 * (hash + id.hashCode()) : 0;
> 382:         hash = 31 * (hash + pseudoClassState.hashCode());
> 383:         return hash;

Is it worth caching the hash code for this class?

-------------

PR Review: https://git.openjdk.org/jfx/pull/1316#pullrequestreview-1804174369
PR Review Comment: https://git.openjdk.org/jfx/pull/1316#discussion_r1441758180
PR Review Comment: https://git.openjdk.org/jfx/pull/1316#discussion_r1441770930
PR Review Comment: https://git.openjdk.org/jfx/pull/1316#discussion_r1441772947
PR Review Comment: https://git.openjdk.org/jfx/pull/1316#discussion_r1441777174
PR Review Comment: https://git.openjdk.org/jfx/pull/1316#discussion_r1441798989
PR Review Comment: https://git.openjdk.org/jfx/pull/1316#discussion_r1441792235