On Thu, 4 Jan 2024 12:21:15 GMT, John Hendrikx <[email protected]> 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
