RFR: 8320359: ImageView: add styleable fitWidth, fitHeight, preserveRatio, smooth properties [v3]

Kevin Rushforth kcr at openjdk.org
Fri Dec 1 17:42:23 UTC 2023


On Fri, 1 Dec 2023 15:32:32 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> 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?

I will approve this as is, but I agree with John that it would be better to use one of the existing implementations of List: `new UnmodifiableArrayList`, `List.of`, or `Collections.unmodifiableList`. The cost of the extra copy for `List.of` is in the noise compared to the benefit of not having to maintain yet another special case List class. Similarly, the extra word of data storage in `UnmodifiableArrayList` is in the noise. This is a one-per-class not one-per instance list.

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1293#discussion_r1412400728


More information about the openjfx-dev mailing list