[8u] RFR 8214687: Optimize Collections.nCopies().hashCode() and equals()

[Andrew Hughes]

On Wed, 31 Jul 2019 at 17:09, Aleksey Shipilev <shade at redhat.com> wrote:
>
> On 7/31/19 5:56 PM, Andrew John Hughes wrote:
> > On 31/07/2019 10:02, Aleksey Shipilev wrote:
> >> On 7/16/19 9:25 AM, Andrew John Hughes wrote:
> >>> On 15/07/2019 09:27, Aleksey Shipilev wrote:
> >>>> On 7/2/19 9:53 PM, Aleksey Shipilev wrote:
> >>>>> Original RFE:
> >>>>>   https://bugs.openjdk.java.net/browse/JDK-8214687
> >>>>>   https://hg.openjdk.java.net/jdk/jdk/rev/cfceb4df2499
> >>>>>
> >>>>> Patch applies with usual reshufflings. But the test parts require touchups to compile and run on 8u:
> >>>>> type inference is not that rich, and there is no Objects.checkIndex. 8u webrev:
> >>>>>   https://cr.openjdk.java.net/~shade/8214687/webrev.8u.01/
> >>>
> >>> Objects.checkIndex is simply a wrapper around Preconditions.checkIndex:
> >>>
> >>>     public static
> >>>     int checkIndex(int index, int length) {
> >>>         return Preconditions.checkIndex(index, length, null);
> >>>     }
> >>>
> >>> which is in 8u. Probably worth adding the 2-argument version to
> >>> Preconditions.
> >>
> >> Getting back to this. Since we have moved Preconditions to private location in 8u, using it is
> >> impossible for this patch. So, I would keep the webrev as is:
> >>   https://cr.openjdk.java.net/~shade/8214687/webrev.8u.01
> >
> > We haven't moved anything as yet and my proposed patch leaves the class
> > as public, so shouldn't be a blocker. The test in that patch uses
> > Preconditions.
>
> Original patch needs Objects.checkIndex:
>   https://hg.openjdk.java.net/jdk/jdk/rev/cfceb4df2499#l2.32
>
> What you are suggesting is changing that line to Preconditions (with new method!). What webrev
> suggests is replacing it with the one-liner:
>
>   91  check(0 <= index && index < n, "Index is incorrect");

I thought I said everything here worth saying.

To repeat, if you look at Objects.checkIndex in 9+, you'll see it just
calls Preconditions.checkIndex.

So what I'm suggesting is to do the same as the original code, without
one level of indirection.

What you're suggesting is a completely new line of code,

>
> Given that we are changing the code anyway, I don't see why do we need to introduce another
> dependency to the about-to-be-moved class, do more code that diverges 8u from later releases,
> instead of doing the trivial one-liner.

The very point of my suggestion was about avoiding divergence. I can
see your argument about an additional dependency, but I didn't write
the original test code, and your argument applies equally to that as
well. Objects.checkIndex(index,
n)->Preconditions.checkIndex(index,n,null) or a direct
Preconditoins.checkIndex(index,n,null); it's the same thing.

If you're happy maintaining that divergence, then I have no problem
with the patch, but it's a fallacy to pretend what I'm suggesting is
more divergent.
-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Web Site: http://fuseyism.com
Twitter: https://twitter.com/gnu_andrew_java
PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222