On Thu, 30 Mar 2023 20:23:59 GMT, John Hendrikx <jhendr...@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
