[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