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

[John Hendrikx]

On Mon, 3 Apr 2023 23:36:32 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:

>> I don't think there is much of an impact, scene graph size should not affect it.  The sets involved are tiny, usually consisting of 1 or 2 items and very rarely 3 or possibly more(*). The hash code of `PseudoClass` is very fast, as they're singletons which don't override their hash code.
>> 
>> (*) The sets are based on pseudo class combinations encountered in a Scene that would have an active role in changing the control's visual appearance.  A pseudo class that doesn't affect appearance is filtered.  This means usually only states like `hovered`, `focused` or `armed` have an affect.  A class like `focused-within` would only be part of the set if the control involved has styles based on that class that would affect its appearance.
>
> Couldn't you just add a fast path by calculating the hash code of the immutable set eagerly and storing it in a field? This entire method could probably be written to be allocation-free if the set passed into the method is already an immutable set.

I think I am missing something :)

The set that is passed in may indeed be immutable, but it may not be the set we have cached.  The memory savings only work when I then return the cached set; if I return the set you pass in (even if it is immutable) then we would still have large numbers of `Set<PseudoClass>` that are duplicates.

So, at a minimum, the `Set` that is passed in would need its hash code calculated so I can check if it is the cached version, or otherwise return the cached version.

I could cache the hash code for the sets that are returned here (they are immutable sets returned by `Set.copyOf` -- I checked their code, and they don't cache the hash codes).  That would however only help if you often pass in a set that is already cached, to find out if you got the same one.  Perhaps this happens a lot, I could test that.

Note that `HashMap` already caches hash codes for things that are part of the map, so only the hash code of the input needs to be calculated on this call.

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

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