On Tue, 12 Sep 2023 23:28:37 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 19 commits: > > - Merge branch 'master' of https://git.openjdk.org/jfx into > feature/immutable-pseudoclassstate > - Use standard asserts in test > - Add copyright header > - 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 > - ... and 9 more: https://git.openjdk.org/jfx/compare/624fe86f...962c43ea Hi Daniel, There are no strict rules on what can be backported or not, but in general, the focus is on the security fixes. Those are critical, and are needed in LTS releases. The purpose of LTS releases is to provide stability over a long time. Even though some fixed issues provide a better performance/UX, it is often not desired to backport those to LTS releases, as it may change the behavior. The fix you're talking about here is a great performance improvement, but also a real big conceptual change that might have unexpected consequences, so I am personally a bit hesitant to backport this to 21 -- but more than happy to discuss this of course. The next LTS release will be JavaFX 25, which will be released in less than a year, hence it's not that far out. - Johan Op vr 25 okt 2024 om 20:51 schreef Daniel Godino ***@***.***>: > Hello! This might not be the right place to ask, so sorry about it in > advance, I've found this pr after investigating a memory problem in an > application with a lot of data in tables, I can see is available in openjfx > 22 and later but still no LTS version above 21, are you planning to include > this change in a future version of openjfx21? I find this a big > improvement. Thank you! > > — > Reply to this email directly, view it on GitHub > <https://github.com/openjdk/jfx/pull/1076#issuecomment-2438587205>, or > unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAF3PBA65FSGYIP7CLS64U3Z5KHJVAVCNFSM6AAAAABQT27PAGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMZYGU4DOMRQGU> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> > ------------- PR Comment: https://git.openjdk.org/jfx/pull/1076#issuecomment-2440120328
