More than happy to provide feedback regarding performance improvements in our applications due to this change.
Dirk > Am 05.09.2023 um 21:25 schrieb Johan Vos <j...@openjdk.org>: > > On Fri, 9 Jun 2023 12:45:02 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`) -- I fixed >>> this bug because I need to call `Set.copyOf` which uses `toArray` -- as the >>> same bug was also present in `StyleClassSet`, I fixed it there as well. >>> >>> 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. >> >> John Hendrikx has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 16 commits: >> >> - Merge branch 'master' of https://git.openjdk.org/jfx into >> feature/immutable-pseudoclassstate >> - Merge remote-tracking branch 'upstream/master' into >> feature/immutable-pseudoclassstate >> - Avoid using Lambda in ImmutablePseudoClassSetsCache.of() >> - Merge branch 'master' of https://git.openjdk.org/jfx into >> feature/immutable-pseudoclassstate >> - Fix another edge case in BitSet equals >> >> When arrays are not the same size, but there are no set bits in the ones >> the other set doesn't have, two bit sets can still be considered equal >> - Take element type into account for BitSet.equals() >> - Base BitSet on AbstractSet to inherit correct equals/hashCode/toArray >> >> - Removed faulty toArray implementations in PseudoClassState and >> StyleClassSet >> - Added test that verifies equals/hashCode for PseudoClassState respect >> Set contract now >> - Made getBits package private so it can't be inherited >> - Remove unused code >> - Ensure Match doesn't allow modification >> - Simplify ImmutablePseudoClassSetsCache and avoid an unnecessary copy >> - ... and 6 more: https://git.openjdk.org/jfx/compare/9ad0e908...7975ae99 > > The caching means a huge performance improvement in real-world applications. > This looks like one of the most relevant performance enhancements we can > achieve. > I tried hard but could not spot regression. I'd love to see this PR merged > and used in ea-releases so that we get hopefully feedback from real-world > libraries and applications soon. > > ------------- > > Marked as reviewed by jvos (Reviewer). > > PR Review: https://git.openjdk.org/jfx/pull/1076#pullrequestreview-1611779656
