Re: RFR(s): JDK-8214687 Optimize Collections.nCopies().hashCode()
Hello!
I managed to push the change by myself:
http://hg.openjdk.java.net/jdk/jdk/rev/cfceb4df2499
This is my first push in a committer role. Hopefully I did not mess anything up.
With best regards,
Tagir Valeev.
On Thu, Dec 13, 2018 at 12:33 PM Tagir Valeev wrote:
>
> Hello, Zheka!
>
> I'm not sure whether it's possible to commit a patch which is
> partially contributed by another person. Probably you should submit it
> separately? Also for complete patch a testcase would be necessary.
>
> With best regards,
> Tagir Valeev.
> On Thu, Dec 13, 2018 at 11:48 AM Zheka Kozlov wrote:
> >
> > OK, this is a fixed version:
> >
> > @Override
> > public void forEach(Consumer action) {
> > Objects.requireNonNull(action);
> > final int n = this.n;
> > final E e = this.element;
> > for (int i = 0; i < n; i++) {
> > action.accept(e);
> > }
> > }
> >
> > Tagir, can you add this to your patch? I signed the OCA.
> >
> >
> > ср, 12 дек. 2018 г. в 11:25, Martin Buchholz :
> >>
> >> I used to believe that, but apparently I was wrong.
> >> https://openjdk.markmail.org/thread/rfqfultw35i2az45
> >>
> >> On Tue, Dec 11, 2018 at 8:14 PM Zheka Kozlov wrote:
> >>>
> >>> Would be better to add @Stable to the fields instead? (`n` and `element`
> >>> are final, so @Stable is OK here)
> >>>
> >>> ср, 12 дек. 2018 г. в 11:02, Martin Buchholz :
> >
> > In performance critical code, we don't trust hotspot to not reload
> > final fields. Other forEach methods do this, e.g.
>
>
> final Object[] es = queue;
> for (int i = 0, n = size; i < n; i++)
> action.accept((E) es[i]);
>
Re: RFR(s): JDK-8214687 Optimize Collections.nCopies().hashCode()
Hello, Zheka!
I'm not sure whether it's possible to commit a patch which is
partially contributed by another person. Probably you should submit it
separately? Also for complete patch a testcase would be necessary.
With best regards,
Tagir Valeev.
On Thu, Dec 13, 2018 at 11:48 AM Zheka Kozlov wrote:
>
> OK, this is a fixed version:
>
> @Override
> public void forEach(Consumer action) {
> Objects.requireNonNull(action);
> final int n = this.n;
> final E e = this.element;
> for (int i = 0; i < n; i++) {
> action.accept(e);
> }
> }
>
> Tagir, can you add this to your patch? I signed the OCA.
>
>
> ср, 12 дек. 2018 г. в 11:25, Martin Buchholz :
>>
>> I used to believe that, but apparently I was wrong.
>> https://openjdk.markmail.org/thread/rfqfultw35i2az45
>>
>> On Tue, Dec 11, 2018 at 8:14 PM Zheka Kozlov wrote:
>>>
>>> Would be better to add @Stable to the fields instead? (`n` and `element`
>>> are final, so @Stable is OK here)
>>>
>>> ср, 12 дек. 2018 г. в 11:02, Martin Buchholz :
>
> In performance critical code, we don't trust hotspot to not reload final
> fields. Other forEach methods do this, e.g.
final Object[] es = queue;
for (int i = 0, n = size; i < n; i++)
action.accept((E) es[i]);
Re: RFR(s): JDK-8214687 Optimize Collections.nCopies().hashCode()
OK, this is a fixed version:
@Override
public void forEach(Consumer action) {
Objects.requireNonNull(action);
final int n = this.n;
final E e = this.element;
for (int i = 0; i < n; i++) {
action.accept(e);
}
}
Tagir, can you add this to your patch? I signed the OCA.
ср, 12 дек. 2018 г. в 11:25, Martin Buchholz :
> I used to believe that, but apparently I was wrong.
> https://openjdk.markmail.org/thread/rfqfultw35i2az45
>
> On Tue, Dec 11, 2018 at 8:14 PM Zheka Kozlov
> wrote:
>
>> Would be better to add @Stable to the fields instead? (`n` and `element`
>> are final, so @Stable is OK here)
>>
>> ср, 12 дек. 2018 г. в 11:02, Martin Buchholz :
>>
>>> In performance critical code, we don't trust hotspot to not reload final
fields. Other forEach methods do this, e.g.
>>>
>>>
>>> final Object[] es = queue;
>>> for (int i = 0, n = size; i < n; i++)
>>> action.accept((E) es[i]);
>>>
>>>
>>
Re: RFR(s): JDK-8214687 Optimize Collections.nCopies().hashCode()
I used to believe that, but apparently I was wrong. https://openjdk.markmail.org/thread/rfqfultw35i2az45 On Tue, Dec 11, 2018 at 8:14 PM Zheka Kozlov wrote: > Would be better to add @Stable to the fields instead? (`n` and `element` > are final, so @Stable is OK here) > > ср, 12 дек. 2018 г. в 11:02, Martin Buchholz : > >> In performance critical code, we don't trust hotspot to not reload final >>> fields. Other forEach methods do this, e.g. >> >> >> final Object[] es = queue; >> for (int i = 0, n = size; i < n; i++) >> action.accept((E) es[i]); >> >> >
Re: RFR(s): JDK-8214687 Optimize Collections.nCopies().hashCode()
Would be better to add @Stable to the fields instead? (`n` and `element` are final, so @Stable is OK here) ср, 12 дек. 2018 г. в 11:02, Martin Buchholz : > In performance critical code, we don't trust hotspot to not reload final >> fields. Other forEach methods do this, e.g. > > > final Object[] es = queue; > for (int i = 0, n = size; i < n; i++) > action.accept((E) es[i]); > >
Re: RFR(s): JDK-8214687 Optimize Collections.nCopies().hashCode()
> > In performance critical code, we don't trust hotspot to not reload final > fields. Other forEach methods do this, e.g. final Object[] es = queue; for (int i = 0, n = size; i < n; i++) action.accept((E) es[i]);
Re: RFR(s): JDK-8214687 Optimize Collections.nCopies().hashCode()
Hi Sergey.
`n` and `element` are final fields. Extracting it into local vars will not
make any difference.
ср, 12 дек. 2018 г. в 02:25, Сергей Цыпанов :
> Hi Zheka and Tagir,
>
> @Override
> public void forEach(final Consumer 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" :
> > What about forEach()?
> >
> > @Override
> > public void forEach(final Consumer 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 :
> >
> >> 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
> 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 :
> >> >>
> >> >> 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.ma...@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
>
Re: RFR(s): JDK-8214687 Optimize Collections.nCopies().hashCode()
Hi Zheka and Tagir,
@Override
public void forEach(final Consumer 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" :
> What about forEach()?
>
> @Override
> public void forEach(final Consumer 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 :
>
>> 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 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 :
>> >>
>> >> 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
>> 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
Re: RFR(s): JDK-8214687 Optimize Collections.nCopies().hashCode()
What about forEach()?
@Override
public void forEach(final Consumer 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 :
> 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 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 :
> >>
> >> 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
> 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
>
Re: RFR(s): JDK-8214687 Optimize Collections.nCopies().hashCode()
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 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 : >> >> 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 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
Re: RFR(s): JDK-8214687 Optimize Collections.nCopies().hashCode()
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 : > 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 > 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 >
Re: RFR(s): JDK-8214687 Optimize Collections.nCopies().hashCode()
Hi Tagir! I think what you have in the last webrev looks good! In CopiesList.equals() please use eq() instead of Objects.equal() (see a comment at the line 5345). With kind regards, Ivan On 12/3/18 9:22 PM, Tagir Valeev wrote: 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 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 -- With kind regards, Ivan Gerasimov
Re: RFR(s): JDK-8214687 Optimize Collections.nCopies().hashCode()
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 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
Re: RFR(s): JDK-8214687 Optimize Collections.nCopies().hashCode()
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
Re: RFR(s): JDK-8214687 Optimize Collections.nCopies().hashCode()
The general rule for JDK core classes (including collections, even private implementations) is that we try to preserve backward *and* forward serialization compatibility. This doesn't apply to all classes in the JDK, though. For example, javax.swing.JComponent is serializable, but it includes a disclaimer about potential serialization incompatibilities with future versions of itself. s'marks On 12/3/18 11:06 AM, Andrew Luo wrote: CopiesList is private anyways, so are you suggesting that someone might call nCopies(0) in a previous version of the JDK, serialize the return value, and then unserialize in a later version of the JRE? Is this even a supported use case (serialization/deserialization of JRE classes across versions)? It was always my understanding that you can only unserialize JRE classes of the same version, since the internal class definition can change at any time... Thanks, -Andrew -Original Message- From: core-libs-dev On Behalf Of Zheka Kozlov Sent: Monday, December 3, 2018 12:43 AM To: Ivan Gerasimov Cc: core-libs-dev Subject: Re: RFR(s): JDK-8214687 Optimize Collections.nCopies().hashCode() Would it make sense to use CopiesList only for n > 0 This can break serialization. When CopiesList(0, e) is serialized and deserialized back, it will be in an invalid state. I believe it makes sense to override CopiesList.equals() Also: contains(), iterator(), listIterator() пн, 3 дек. 2018 г. в 14:19, Ivan Gerasimov : Thank you Tagir! I think your solution is quite clever, and the fix looks good. While we are here: Would it make sense to use CopiesList only for n > 0, and make nCopies() and nCopies().subList() return Collection.emptyList() otherwise? This would allow to remove the check for n == 0 in a couple of places. Also an unnecessary reference to the `element` wouldn't be kept. With kind regards, Ivan On 12/2/18 9:27 PM, Tagir Valeev wrote: Hello! Please review and sponsor the optimized implementation of Collections.nCopies().hashCode(): https://bugs.openjdk.java.net/browse/JDK-8214687 http://cr.openjdk.java.net/~tvaleev/webrev/8214687/r1/ Previous discussion thread: http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-November/056 843.html Thanks to Zheka Kozlov for the original proposal. Also thanks to Ivan Gerasimov for the simplification idea: (x & 0x8000_) != 0 => x < 0. With best regards, Tagir Valeev -- With kind regards, Ivan Gerasimov
RE: RFR(s): JDK-8214687 Optimize Collections.nCopies().hashCode()
CopiesList is private anyways, so are you suggesting that someone might call nCopies(0) in a previous version of the JDK, serialize the return value, and then unserialize in a later version of the JRE? Is this even a supported use case (serialization/deserialization of JRE classes across versions)? It was always my understanding that you can only unserialize JRE classes of the same version, since the internal class definition can change at any time... Thanks, -Andrew -Original Message- From: core-libs-dev On Behalf Of Zheka Kozlov Sent: Monday, December 3, 2018 12:43 AM To: Ivan Gerasimov Cc: core-libs-dev Subject: Re: RFR(s): JDK-8214687 Optimize Collections.nCopies().hashCode() > Would it make sense to use CopiesList only for n > 0 This can break serialization. When CopiesList(0, e) is serialized and deserialized back, it will be in an invalid state. > I believe it makes sense to override CopiesList.equals() Also: contains(), iterator(), listIterator() пн, 3 дек. 2018 г. в 14:19, Ivan Gerasimov : > Thank you Tagir! > > I think your solution is quite clever, and the fix looks good. > > While we are here: Would it make sense to use CopiesList only for n > > 0, and make nCopies() and nCopies().subList() return > Collection.emptyList() otherwise? > > This would allow to remove the check for n == 0 in a couple of places. > Also an unnecessary reference to the `element` wouldn't be kept. > > With kind regards, > Ivan > > > On 12/2/18 9:27 PM, Tagir Valeev wrote: > > Hello! > > > > Please review and sponsor the optimized implementation of > > Collections.nCopies().hashCode(): > > https://bugs.openjdk.java.net/browse/JDK-8214687 > > http://cr.openjdk.java.net/~tvaleev/webrev/8214687/r1/ > > > > Previous discussion thread: > > > http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-November/056 > 843.html > > > > Thanks to Zheka Kozlov for the original proposal. Also thanks to > > Ivan Gerasimov for the simplification idea: (x & 0x8000_) != 0 > > => x < 0. > > > > With best regards, > > Tagir Valeev > > > > -- > With kind regards, > Ivan Gerasimov > >
Re: RFR(s): JDK-8214687 Optimize Collections.nCopies().hashCode()
> Would it make sense to use CopiesList only for n > 0 This can break serialization. When CopiesList(0, e) is serialized and deserialized back, it will be in an invalid state. > I believe it makes sense to override CopiesList.equals() Also: contains(), iterator(), listIterator() пн, 3 дек. 2018 г. в 14:19, Ivan Gerasimov : > Thank you Tagir! > > I think your solution is quite clever, and the fix looks good. > > While we are here: Would it make sense to use CopiesList only for n > > 0, and make nCopies() and nCopies().subList() return > Collection.emptyList() otherwise? > > This would allow to remove the check for n == 0 in a couple of places. > Also an unnecessary reference to the `element` wouldn't be kept. > > With kind regards, > Ivan > > > On 12/2/18 9:27 PM, Tagir Valeev wrote: > > Hello! > > > > Please review and sponsor the optimized implementation of > > Collections.nCopies().hashCode(): > > https://bugs.openjdk.java.net/browse/JDK-8214687 > > http://cr.openjdk.java.net/~tvaleev/webrev/8214687/r1/ > > > > Previous discussion thread: > > > http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-November/056843.html > > > > Thanks to Zheka Kozlov for the original proposal. Also thanks to Ivan > > Gerasimov for the simplification idea: (x & 0x8000_) != 0 => x < > > 0. > > > > With best regards, > > Tagir Valeev > > > > -- > With kind regards, > Ivan Gerasimov > >
Re: RFR(s): JDK-8214687 Optimize Collections.nCopies().hashCode()
Also, I believe it makes sense to override CopiesList.equals() to at least optimize the case when the other list is a CopiesList too. With kind regards, Ivan On 12/2/18 11:23 PM, Ivan Gerasimov wrote: Thank you Tagir! I think your solution is quite clever, and the fix looks good. While we are here: Would it make sense to use CopiesList only for n > 0, and make nCopies() and nCopies().subList() return Collection.emptyList() otherwise? This would allow to remove the check for n == 0 in a couple of places. Also an unnecessary reference to the `element` wouldn't be kept. With kind regards, Ivan On 12/2/18 9:27 PM, Tagir Valeev wrote: Hello! Please review and sponsor the optimized implementation of Collections.nCopies().hashCode(): https://bugs.openjdk.java.net/browse/JDK-8214687 http://cr.openjdk.java.net/~tvaleev/webrev/8214687/r1/ Previous discussion thread: http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-November/056843.html Thanks to Zheka Kozlov for the original proposal. Also thanks to Ivan Gerasimov for the simplification idea: (x & 0x8000_) != 0 => x < 0. With best regards, Tagir Valeev -- With kind regards, Ivan Gerasimov
Re: RFR(s): JDK-8214687 Optimize Collections.nCopies().hashCode()
Thank you Tagir! I think your solution is quite clever, and the fix looks good. While we are here: Would it make sense to use CopiesList only for n > 0, and make nCopies() and nCopies().subList() return Collection.emptyList() otherwise? This would allow to remove the check for n == 0 in a couple of places. Also an unnecessary reference to the `element` wouldn't be kept. With kind regards, Ivan On 12/2/18 9:27 PM, Tagir Valeev wrote: Hello! Please review and sponsor the optimized implementation of Collections.nCopies().hashCode(): https://bugs.openjdk.java.net/browse/JDK-8214687 http://cr.openjdk.java.net/~tvaleev/webrev/8214687/r1/ Previous discussion thread: http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-November/056843.html Thanks to Zheka Kozlov for the original proposal. Also thanks to Ivan Gerasimov for the simplification idea: (x & 0x8000_) != 0 => x < 0. With best regards, Tagir Valeev -- With kind regards, Ivan Gerasimov
