RFR(s): JDK-8214687 Optimize Collections.nCopies().hashCode()

[Zheka Kozlov]

Hi Sergey.

`n` and `element` are final fields. Extracting it into local vars will not
make any difference.

ср, 12 дек. 2018 г. в 02:25, Сергей Цыпанов <sergei.tsypanov at yandex.ru>:

> Hi Zheka and Tagir,
>
> @Override
> public void forEach(final Consumer<? super E> action) {
>   Objects.requireNonNull(action);
>   for (int i = 0; i < this.n; i++) {
>     action.accept(this.element);
>   }
> }
>
> I think here we should extract this.element and this.n into a local vars,
> as Consumer may have interaction with volatile field inside (e.g.
> AtomicInteger::addAndGet) which would cause a load of consumed field at
> each iteration. See original post by Nitsan Wakart
> https://psy-lob-saw.blogspot.com/2014/08/the-volatile-read-suprise.html
>
> Regards,
> Sergey Tsypanov
>
>
> 07.12.2018, 15:22, "Zheka Kozlov" <orionllmain at gmail.com>:
> > What about forEach()?
> >
> > @Override
> > public void forEach(final Consumer<? super E> action) {
> >     Objects.requireNonNull(action);
> >     for (int i = 0; i < this.n; i++) {
> >         action.accept(this.element);
> >     }
> > }
> >
> > I think this version has better chances to be faster than the default
> > implementation (did not measure though).
> >
> > вт, 4 дек. 2018 г. в 14:40, Tagir Valeev <amaembo at gmail.com>:
> >
> >>  Hello!
> >>
> >>  > In CopiesList.equals() please use eq() instead of Objects.equal()
> (see a
> >>  comment at the line 5345).
> >>
> >>  Ok
> >>
> >>  > I think you should use iterator() instead of listIterator(). See the
> >>  explanation here:
> >>
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-April/052472.html
> >>
> >>  Ok. I wonder why this message received no attention.
> >>
> >>  Here's updated webrev:
> >>  http://cr.openjdk.java.net/~tvaleev/webrev/8214687/r3/
> >>
> >>  With best regards,
> >>  Tagir Valeev
> >>  On Tue, Dec 4, 2018 at 1:10 PM Zheka Kozlov <orionllmain at gmail.com>
> wrote:
> >>  >
> >>  > I think you should use iterator() instead of listIterator(). See the
> >>  explanation here:
> >>
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-April/052472.html
> >>  >
> >>  > вт, 4 дек. 2018 г. в 12:23, Tagir Valeev <amaembo at gmail.com>:
> >>  >>
> >>  >> Hello!
> >>  >>
> >>  >> Thank you for your comments!
> >>  >>
> >>  >> Yes, deserialization will be broken if we assume that size is never
> 0.
> >>  >> Also we'll introduce referential identity Collections.nCopies(0, x)
> ==
> >>  >> Collections.nCopies(0, y) which might introduce slight semantics
> >>  >> change in existing programs. Once I suggested to wire
> Arrays.asList()
> >>  >> (with no args) to Collections.emptyList(), but it was rejected for
> the
> >>  >> same reason: no need to introduce a risk of possible semantics
> change.
> >>  >>
> >>  >> I updated webrev with equals implementation and test:
> >>  >> http://cr.openjdk.java.net/~tvaleev/webrev/8214687/r2/
> >>  >> Comparing two CopiesList is much faster now indeed. Also we can
> spare
> >>  >> an iterator in the common case and hoist the null-check out of the
> >>  >> loop. Not sure whether we can rely that JIT will always do this for
> >>  >> us, but if you think that it's unnecessary, I can merge the loops
> >>  >> back. Note that now copiesList.equals(arrayList) could be faster
> than
> >>  >> arrayList.equals(copiesList). I don't think it's an issue. On the
> >>  >> other hand we could keep simpler and delegate to
> super-implementation
> >>  >> if the other object is not a CopiesList (like it's implemented in
> >>  >> java.util.RegularEnumSet::equals for example). What do you think?
> >>  >>
> >>  >> With best regards,
> >>  >> Tagir Valeev.
> >>  >> On Tue, Dec 4, 2018 at 10:56 AM Stuart Marks <
> stuart.marks at oracle.com>
> >>  wrote:
> >>  >> >
> >>  >> >
> >>  >> > >> I believe it makes sense to override CopiesList.equals()
> >>  >> > > Also: contains(), iterator(), listIterator()
> >>  >> >
> >>  >> > equals(): sure
> >>  >> >
> >>  >> > contains() is already overridden. Not sure there's much benefit to
> >>  overriding
> >>  >> > the iterators.
> >>  >> >
> >>  >> > s'marks
>