On Thu, 30 Nov 2023 23:24:40 GMT, Andy Goryachev <[email protected]> wrote:
>> modules/javafx.graphics/src/main/java/com/sun/javafx/css/CssUtil.java line
>> 75:
>>
>>> 73:
>>> 74: /** immutable list with random access backed by an array */
>>> 75: private static class ImmutableArrayList<T> extends AbstractList<T>
>>> implements RandomAccess {
>>
>> I'd like to point that a skeletal implementation of `AbstractList` will
>> perform worse than `ArrayList` for any method that needs to walk the list,
>> as the `AbstractList` will use iterators for methods like `indexOf`,
>> `hashCode` and `equals`. Now that will probably be irrelevant, but as this
>> seems to be a micro optimization, you should be aware of all the trade offs.
>
> 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**.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1293#discussion_r1411589101
