On Fri, 1 Dec 2023 04:32:16 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> Good point, thanks! >> This also applies to UnmodifiableArrayList. >> >> For completeness sake, I wanted to mention a few issues (not in scope for >> this PR) that came out of the code review: >> >> - could use `UnmodifiableArrayList` but it stores extra int size. perhaps a >> factory method can be added to it for when `size != elements.length`. >> - could improve UnmodifiableArrayList with fast(er) `indexOf`, `hashCode`, >> `equals` per @hjohn 's comment earlier >> - `Control.getCssMetaData()` can be improved to skip merging of two lists if >> skinBase is null >> - the same immutable list should be used in `Control.getCssMetaData()` >> instead of `ArrayList()` > > I wouldn't do any of the above unless there is a very good reason (and I'm > not seeing one). Just use standard `List.of` as the last step (or use the > `Collections.unmodifableList` wrapper); you'll get the most optimized, > automatically maintained, bugfree, immutable `List` implementation Java has > to offer. It means another copy will be made; that's fine, this only happens > once -- it's not in a hot path. > > If you feel like optimizing something, don't bother with > `Control.getCssMetaData` either; instead, deduplicate the property lists so > there is only one list per unique `Skin` + `Control` combo. That saves a > **complete** list per control **instance**. Good point, though I would still not use List.of() because of the unnecessary copy. I agree on `Skin` + `Control` copy. Just not sure how... a static hash table perhaps? ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1293#discussion_r1412253329
