On Thu, 30 Mar 2023 12:49:39 GMT, John Hendrikx <jhendr...@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.
One more optimization we could have a look at is too create the `BitSet` or
related subclasses with a predefined size, so we don't need to grow the `long[]
bits` array too often. `PseudoClassState` already has a constructor that
accepts another collection, but can not use this information to create a bigger
`bits` array.
(Like e.g. the `ArrayList` or `HashSet` constructor)
modules/javafx.graphics/src/main/java/com/sun/javafx/css/BitSet.java line 227:
> 225: final T t = cast(o);
> 226:
> 227: if (t == null) { // if cast failed, it can't be part of this set
If I understand the code correctly, this can't happen right now, right?
modules/javafx.graphics/src/main/java/com/sun/javafx/css/BitSet.java line 231:
> 229: }
> 230:
> 231: final int element = getIndex(t) / Long.SIZE;
`getIndex(t)` is called twice here and below, maybe we should consider calling
it once and save the result in a local variable instead?
-------------
PR Comment: https://git.openjdk.org/jfx/pull/1076#issuecomment-1490264725
PR Review Comment: https://git.openjdk.org/jfx/pull/1076#discussion_r1153276734
PR Review Comment: https://git.openjdk.org/jfx/pull/1076#discussion_r1153274025
