On Thu, 4 Jan 2024 14:17:48 GMT, Nir Lisker <[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).
>
> 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 `!`.
I've kept the style the same, there is another line like this just below it.
Don't like it either personally :)
> 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?
I put a debug print in to this method, and then started JFXCentral application
(which is quite big) -- it's never called, so doesn't look like it is used
anywhere. Selectors are most often stored in `List`s, didn't find any `Map`s
or `Set`s that use it as a key.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1316#discussion_r1442407347
PR Review Comment: https://git.openjdk.org/jfx/pull/1316#discussion_r1442406830
