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

[John Hendrikx]

On Thu, 30 Mar 2023 20:23:59 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> modules/javafx.graphics/src/main/java/com/sun/javafx/css/ImmutablePseudoClassSetsCache.java line 55:
>> 
>>> 53:          *
>>> 54:          * If the given set was already immutable; the copyOf will return the same
>>> 55:          * set; however, that may or may not be the set that is already in the cache.
>> 
>> Do we want to depend on this behavior? The specification of `Set.copyOf` says:
>> 
>> @implNote
>> If the given Collection is an unmodifiable Set, calling copyOf will generally not create a copy.
>> 
>> 
>> While the actual implementation _never_ creates a copy if the given collection is immutable, the specification does not guarantee that in all cases, but only in the general case. This behavior could theoretically change without changing the specification of `Set.copyOf`.
>
>> Do we want to depend on this behavior? The specification of `Set.copyOf` says:
>> 
>> ```
>> @implNote
>> If the given Collection is an unmodifiable Set, calling copyOf will generally not create a copy.
>> ```
>> 
>> While the actual implementation _never_ creates a copy if the given collection is immutable, the specification does not guarantee that in all cases, but only in the general case. This behavior could theoretically change without changing the specification of `Set.copyOf`.
> 
> If it ever changes implementation, I suppose we could do this logic ourselves -- however, you did give me an idea... before making the copy, I could check if it is in the cache already.  In the case that the set you pass is mutable, but I have an immutable copy already then no copy would need to be made.  The only time a copy is made then is if it wasn't in the cache yet.
> 
> Before arriving at this solution I had a class `ImmutablePseudoClassState` which extended `AbstractSet`.  But after stripping everything out that I didn't need, the only thing that was left was a single private field... which was also a `Set`.  I then concluded that there is no need for this wrapper at all, as everything in the CSS code can deal with `Set<PseudoClass>` directly.
> 
> EDIT: I changed this, and the `computeIfAbsent` now looks much better, no need for the long explanatory comment... not sure what I was thinking at the time :)

I've looked into this part a bit further, and I noticed that `BitSet` is also violating the equals/hashCode contract that are supposed to apply to all `Set` implementations.  As I'm now using a `Set` as key for the cache, this contract needs to be adhered to.  I'll see if I can test if this is indeed causing problems as I suspect, and fix it if so.

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

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