On Mon, 3 Apr 2023 23:36:32 GMT, Michael Strauß <mstra...@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
