RFR: JDK-8199216: Memory leak and quadratic layout time with nested nodes (hbox) and pseudo-class in style sheet [v3]

[Michael Strauß]

On Mon, 3 Apr 2023 07:11:07 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

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

Couldn't you just add a fast path by calculating the hash code of the immutable set eagerly and storing it in a field? This entire method could probably be written to be allocation-free if the set passed into the method is already an immutable set.

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1076#discussion_r1156550443