[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: RFR: 8320359: ImageView: add styleable fitWidth, fitHeight, preserveRatio, smooth properties [v3
From:       Andy Goryachev <angorya () openjdk ! org>
Date:       2023-11-30 23:27:19
Message-ID: b_sg3EiyFey4CGkSHVq_yILQeKB7hHdDbnEswxve_Lk=.34f15be3-addc-42b3-b06f-4edf63aaf355 () github ! com
[Download RAW message or body]

On Thu, 30 Nov 2023 23:02:10 GMT, John Hendrikx <jhendrikx@openjdk.org> wrote:

> > Andy Goryachev has updated the pull request incrementally with one additional \
> > commit since the last revision: 
> > save 8 bytes
> 
> 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()`

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

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


[prev in list] [next in list] [prev in thread] [next in thread]