On Fri, 31 Mar 2023 21:56:08 GMT, John Hendrikx <[email protected]> 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 incrementally with three
> additional commits since the last revision:
>
> - 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
does my comment for equals() make sense?
Match is a public class, does this PR need a CSR?
modules/javafx.graphics/src/main/java/com/sun/javafx/css/BitSet.java line 243:
> 241: public boolean containsAll(Collection<?> c) {
> 242: if (c == null) {
> 243: throw new NullPointerException("c cannot be null");
I wonder if this is necessary - line 246 will produce an NPE with a similar
message, right?
(here and in some other places)
modules/javafx.graphics/src/main/java/com/sun/javafx/css/BitSet.java line 546:
> 544: }
> 545: if (obj instanceof BitSet<?> bitSet) { // fast path if other is
> a BitSet
> 546: return equalsBitSet(bitSet);
I suspect this will not work
BitSet<Integer> will never be equal to BitSet<String>, does it mean we have to
do a per-element comparison?
-------------
Changes requested by angorya (Committer).
PR Review: https://git.openjdk.org/jfx/pull/1076#pullrequestreview-1398765095
PR Comment: https://git.openjdk.org/jfx/pull/1076#issuecomment-1520846721
PR Review Comment: https://git.openjdk.org/jfx/pull/1076#discussion_r1175764244
PR Review Comment: https://git.openjdk.org/jfx/pull/1076#discussion_r1175782050
