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

[Сергей Цыпанов]

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