[10?] RFR: 8193128: Reduce number of implementation classes returned by List/Set/Map.of()
Claes Redestad
claes.redestad at oracle.com
Thu Dec 7 00:12:24 UTC 2017
Hi Rémi,
On 2017-12-06 23:57, Remi Forax wrote:
> Hi Claes,
> - both constructors of SubList should be package private,
deal!
> - in listIterator, i can be declared outside of the ListIterator as a local variable that would be captured by the anonymous class,
> so index is not used inside the anonymous class. Also you can use the diamond syntax for anonymous class now.
>
> public ListIterator<E> listIterator(int index) {
> rangeCheck(index);
> ListIterator<E> i = root.listIterator(offset + index);
> return new ListIterator<>() {
> ...
Sure.
> - you can move "new IndexOutOfBoundsException" out of rangeCheck into outOfBoundsMsg, so the bytecode size of rangeCheck() will be smaller.
I had already done so for the outer class, and realized I had forgotten
to clean this part up:
SubList is now final, inherits from AbstractImmutableList instead of
AbstractList and reuses more of the shared code.
I also moved 'implements Serializable' from AbstractImmutableList to
List12 and ListN respectively to not
change the behavior that List.of(0,1) is serializable while
List.of(0,1).subList(0,1) isn't.
> - in Itr, please declare the constructor after the declaration of the fields,
> and assigning the cursor to zero is useless.
Done.
>
> - in equals, the code is weirdly asymmetric because e1 is typed as an Iterator<E>, declaring it as an Iterator<?> will make the code more symmetric.
I also realized I had missed an opportunity to take advantage of the
fact that elements returned from e1 are guaranteed to
be non-null. Might as well.
>
> - in List12, you have a lot of useless @SuppressWarnings("unchecked") ??
> in size(), get(), contains(), hashCode(), writeReplace().
>
> - in ListN, again some useless @SuppressWarnings("unchecked") ??
> in size(), get(), contains(), hashCode(), writeReplace()
I don't know how these snuck in, but my IDE was pleasantly hiding these
away. I think I cleaned out all the useless ones.
> - the constructor of MapN(K,V) can be made a little more efficient (less getfield opcodes) if written like this
>
> MapN(K key, V value) {
> table = new Object[] { key, value };
> size = 1;
> }
Oops, this was a leftover from my experiment where I adapted MapN to
implement Map1 when setting specializers=0. Removed.
>
> and i do not understand why the field size is not declared @Stable anymore,
> ok, it can be equals to zero, but in that case the JIT will emit a move
> so it's better than always asking for a move (or i do not understand the semantics of @Stable ?)
Hmm, I'm under the impression @Stable brings no additional value to a
final non-array fields (definitely important for arrays).
I might have been guilty of adding @Stable to more fields than necessary
in these implementations in the first place. I've
reverted this removal and will add a note to investigate separately if
we can more systematically clean them up.
http://cr.openjdk.java.net/~redestad/8193128/open.01/
Thanks!
/Claes
More information about the core-libs-dev
mailing list