RFR: JDK-8322964 Optimize performance of CSS selector matching [v2]

[John Hendrikx]

On Sun, 14 Jan 2024 04:20:56 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:

>> This is possible, although `List#get` would not perform too well when it is implemented for `FixedCapacitySet.OpenAddressed` as the array used as hash table in this class can have gaps (so we'd need to iterate to find the index).
>> 
>> However, I am very sure this method is not used anywhere (not even in 3rd party code as it requires casting to access), and I wouldn't encourage its use, so I'd be more inclined to remove it completely.
>
> If this method is not used anywhere, why do we need to expose `getStyleClassNames()` as new API to replace this one? I'm a bit puzzled by that, especially since you're saying that the API shouldn't be used. Why create something that shouldn't be used?
> 
> I'd rather just document that you shouldn't expect great performance from this method, and be done with it. Changing API in a performance optimization PR seems out of scope.

You were talking about `List<String> getStyleClasses()`.  This is not used by FX, and unlikely to be used anywhere by 3rd parties (see reasoning in other comment).

Then there is a 2nd method: `Set<StyleClass> getStyleClassSet()`.  This is/was called by `SelectorPartitioning`, an internal FX class. However, this method makes use of the `StyleClass` wrapper that is hurting performance.

To ensure `SelectorPartitioning` has a fast way to access the styles, I needed a new method: `Set<String> getStyleClassNames`.  As it is called from outside the `javafx.css` package, it has to be `public`.

**Now perhaps you were eluding to something else:** 

Would `SelectorPartitioning` mind if the styles were provided as `List` or `Collection`?  Looking at that code, I think it doesn't really care. It just uses the set as a key, and calls `containsAll` on it.  So perhaps we don't need to deprecate `List<String> getStyleClasses()` then, and we don't need a new API.  Instead I'll have `SelectorPartitioning` be happy with a `Collection` so I also don't have to make make a set implement a `List` (which you can't do without violating the contract of either `List` or `Set` as `hashCode` is defined differently).

Edit: it would still need to be a `List` as I can't return a `Set<String>` from a method that returns `List<String>`... that means either change that method to return `Collection`, so still need an API change, or store everything as `List`...

The only thing that may land us into trouble here (reusing `List<String> getStyleClasses()`) is that there may be an expectation that it still contains duplicates, and retains the original order with which `SimpleSelector` was created.  This is already not the case (as it is based on the old `BitSet` that was created) -- it is deduplicated, and its order is changed (whether that was the intention is unclear, but that's what it does right now).  Then again, the method is currently not in use...

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1316#discussion_r1451656563