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

[Michael Strauß]

On Thu, 30 Mar 2023 12:49:39 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

> This fix introduces immutable sets of `PseudoClass` almost everywhere, as they are rarely modified.  These are re-used by caching them in a new class `ImmutablePseudoClassSetsCache`.
> 
> In order to make this work, `BitSet` had to be cleaned up.  It made assumptions about the collections it is given (which may no longer always be another `BitSet`).  I also added the appropriate null checks to ensure there weren't any other bugs lurking.
> 
> Then there was a severe bug in `toArray` in both the subclasses that implement `BitSet`.
> 
> The bug in `toArray` was incorrect use of the variable `index` which was used for both advancing the pointer in the array to be generated, as well as for the index to the correct `long` in the `BitSet`.  This must have resulted in other hard to reproduce problems when dealing with `Set<PseudoClass>` or `Set<StyleClass>` if directly or indirectly calling `toArray` (which is for example used by `List.of` and `Set.of`).
> 
> The net result of this change is that there are far fewer `PseudoClassState` objects created; the majority of these are never modified, and the few that are left are where you'd expect to see them modified.
> 
> A test with 160 nested HBoxes which were given the hover state shows a 99.99% reduction in `PseudoClassState` instances and a 70% reduction in heap use (220 MB -> 68 MB), see the linked ticket for more details.
> 
> Although the test case above was extreme, this change should have positive effects for most applications.

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

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

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