On Mon, 3 Apr 2023 06:22:39 GMT, Michael Strauß <mstra...@openjdk.org> wrote:
>> John Hendrikx has updated the pull request incrementally with three >> additional commits since the last revision: >> >> - Base BitSet on AbstractSet to inherit correct equals/hashCode/toArray >> >> - Removed faulty toArray implementations in PseudoClassState and >> StyleClassSet >> - Added test that verifies equals/hashCode for PseudoClassState respect >> Set contract now >> - Made getBits package private so it can't be inherited >> - Remove unused code >> - Ensure Match doesn't allow modification > > modules/javafx.graphics/src/main/java/com/sun/javafx/css/ImmutablePseudoClassSetsCache.java > line 53: > >> 51: return CACHE.computeIfAbsent( >> 52: Objects.requireNonNull(pseudoClasses, "pseudoClasses cannot >> be null"), >> 53: k -> Set.copyOf(pseudoClasses) > > The call to `computeIfAbsent` will always do the following things: > 1. Capture the `pseudoClasses` argument > 2. Call `Map.get` internally > 3. ...which calls `Set.hashCode` > 4. ...which, for `AbstractSet.hashCode`, iterates over the entire set using > an iterator and calls `hashCode` for each element. > > Since there is no fast path, I wonder whether this may have negative > performance implications for larger scene graphs. I don't think there is much of an impact, scene graph size should not affect it. The sets involved are tiny, usually consisting of 1 or 2 items and very rarely 3 or possibly more(*). The hash code of `PseudoClass` is very fast, as they're singletons which don't override their hash code. (*) The sets are based on pseudo class combinations encountered in a Scene that would have an active role in changing the control's visual appearance. A pseudo class that doesn't affect appearance is filtered. This means usually only states like `hovered`, `focused` or `armed` have an affect. A class like `focused-within` would only be part of the set if the control involved has styles based on that class that would affect its appearance. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1076#discussion_r1155561513
