Thanks Paul for taking some time and thinking about it. I will make the change you have mentioned and send the patch again.
Regards, Vivek -----Original Message----- From: Paul Sandoz [mailto:paul.san...@oracle.com] Sent: Thursday, June 21, 2018 3:32 PM To: Deshpande, Vivek R <vivek.r.deshpa...@intel.com> Cc: roger.ri...@oracle.com; David Holmes <david.hol...@oracle.com>; core-libs-dev@openjdk.java.net; Viswanathan, Sandhya <sandhya.viswanat...@intel.com>; Ivan Gerasimov <ivan.gerasi...@oracle.com> Subject: Re: RFR(S): 8205194: Improve the Array Comparison when there is a mismatch at first element. Hi Vivek, This looks better. I thought a bit more about NaNs. I am concerned about edge case performance regressions if the first two values are NaNs (with the same bit pattern). Here’s a patch on top of your patch that compares the raw bits for float/double elements. If you are ok with this and apply it on top of your patch then i don’t think there is a need for another review round. Thanks, Paul. diff -r ef8fd07e4440 src/java.base/share/classes/java/nio/BufferMismatch.java --- a/src/java.base/share/classes/java/nio/BufferMismatch.java Thu Jun 21 13:43:04 2018 -0700 +++ b/src/java.base/share/classes/java/nio/BufferMismatch.java Thu Jun 21 14:02:19 2018 -0700 @@ -118,7 +118,7 @@ static int mismatch(FloatBuffer a, int aOff, FloatBuffer b, int bOff, int length) { int i = 0; if (length > 1 && a.order() == b.order()) { - if (a.get(aOff) == b.get(bOff)) { + if (Float.floatToRawIntBits(a.get(aOff)) == + Float.floatToRawIntBits(b.get(bOff))) { i = ArraysSupport.vectorizedMismatch( a.base(), a.address + (aOff << ArraysSupport.LOG2_ARRAY_FLOAT_INDEX_SCALE), b.base(), b.address + (bOff << ArraysSupport.LOG2_ARRAY_FLOAT_INDEX_SCALE), @@ -175,7 +175,7 @@ static int mismatch(DoubleBuffer a, int aOff, DoubleBuffer b, int bOff, int length) { int i = 0; if (length > 0 && a.order() == b.order()) { - if (a.get(aOff) == b.get(bOff)) { + if (Double.doubleToRawLongBits(a.get(aOff)) == + Double.doubleToRawLongBits(b.get(bOff))) { i = ArraysSupport.vectorizedMismatch( a.base(), a.address + (aOff << ArraysSupport.LOG2_ARRAY_DOUBLE_INDEX_SCALE), b.base(), b.address + (bOff << ArraysSupport.LOG2_ARRAY_DOUBLE_INDEX_SCALE), diff -r ef8fd07e4440 src/java.base/share/classes/jdk/internal/util/ArraysSupport.java --- a/src/java.base/share/classes/jdk/internal/util/ArraysSupport.java Thu Jun 21 13:43:04 2018 -0700 +++ b/src/java.base/share/classes/jdk/internal/util/ArraysSupport.java Thu Jun 21 14:02:19 2018 -0700 @@ -461,7 +461,7 @@ int length) { int i = 0; if (length > 1) { - if (a[aFromIndex] == b[bFromIndex]) { + if (Float.floatToRawIntBits(a[aFromIndex]) == + Float.floatToRawIntBits(b[bFromIndex])) { int aOffset = Unsafe.ARRAY_FLOAT_BASE_OFFSET + (aFromIndex << LOG2_ARRAY_FLOAT_INDEX_SCALE); int bOffset = Unsafe.ARRAY_FLOAT_BASE_OFFSET + (bFromIndex << LOG2_ARRAY_FLOAT_INDEX_SCALE); i = vectorizedMismatch( @@ -545,7 +545,7 @@ return -1; } int i = 0; - if (a[aFromIndex] == b[bFromIndex]) { + if (Double.doubleToRawLongBits(a[aFromIndex]) == + Double.doubleToRawLongBits(b[bFromIndex])) { int aOffset = Unsafe.ARRAY_DOUBLE_BASE_OFFSET + (aFromIndex << LOG2_ARRAY_DOUBLE_INDEX_SCALE); int bOffset = Unsafe.ARRAY_DOUBLE_BASE_OFFSET + (bFromIndex << LOG2_ARRAY_DOUBLE_INDEX_SCALE); i = vectorizedMismatch( > On Jun 21, 2018, at 11:50 AM, Deshpande, Vivek R > <vivek.r.deshpa...@intel.com> wrote: > > Hi Paul > > The folding of if/else is the right way in this patch which I missed in > earlier webrev. > http://cr.openjdk.java.net/~vdeshpande/vectorizedMismatch_jdk/webrev.0 > 3/ I think I would keep the same earlier path if both the values are > NaN. > > Thanks. > Regards, > Vivek > From: Paul Sandoz [mailto:paul.san...@oracle.com] > Sent: Wednesday, June 20, 2018 5:29 PM > To: Deshpande, Vivek R <vivek.r.deshpa...@intel.com> > Cc: roger.ri...@oracle.com; David Holmes <david.hol...@oracle.com>; > core-libs-dev@openjdk.java.net; Viswanathan, Sandhya > <sandhya.viswanat...@intel.com> > Subject: Re: RFR(S): 8205194: Improve the Array Comparison when there is a > mismatch at first element. > > Hi Vivek, > > 459 public static int mismatch(float[] a, int aFromIndex, > 460 float[] b, int bFromIndex, > 461 int length) { > 462 int i = 0; > 463 if (length > 1) { > 464 if (a[aFromIndex] != b[bFromIndex]) { > 465 i = 0; > 466 } > 467 if (a[aFromIndex] == b[bFromIndex]) { > > Sorry, i don’t think i was clear. You can reduce down to a single equality > check, so Lines #464-466 are redundant. This does result in taking the slow > path if both values are NaN, which i think was the same for your previous > patch. If that is important we can mitigate that by performing the negation > of the same checks in the slow loop. > > WDYT? > > Paul. > > > On Jun 20, 2018, at 5:05 PM, Deshpande, Vivek R <vivek.r.deshpa...@intel.com> > wrote: > > Hi Paul > > I have made the change you have suggested. > Please find the updated webrev at this location: > http://cr.openjdk.java.net/~vdeshpande/vectorizedMismatch_jdk/webrev.0 > 2/ > > Regards, > Vivek > From: Paul Sandoz [mailto:paul.san...@oracle.com] > Sent: Wednesday, June 20, 2018 2:30 PM > To: Deshpande, Vivek R <vivek.r.deshpa...@intel.com> > Cc: roger.ri...@oracle.com; David Holmes <david.hol...@oracle.com>; > core-libs-dev@openjdk.java.net; Viswanathan, Sandhya > <sandhya.viswanat...@intel.com> > Subject: Re: RFR(S): 8205194: Improve the Array Comparison when there is a > mismatch at first element. > > 459 public static int mismatch(float[] a, int aFromIndex, > 460 float[] b, int bFromIndex, > 461 int length) { > 462 int i = 0; > 463 if (length > 1) { > 464 if (a[aFromIndex] != b[bFromIndex]) { > 465 i = 0; > 466 } else { > > You fold the if/else to; > > if (a[aFromIndex] == b[bFromIndex]) { > … > } > > same applies in other cases in both source files. > > > > > On Jun 20, 2018, at 1:21 PM, Deshpande, Vivek R <vivek.r.deshpa...@intel.com> > wrote: > > Hi All > > I have updated the webrev with all the suggestions, which passes > ArraysEqCmpTest.java. > Earlier webrev failed the same test. > > Thanks for confirming. > > > > This webrev also modifies the BufferMismatch.java in similar way. > > Great! > > Paul. > > > > Please take a look and let me know what you think. > > Updated webrev is here: > http://cr.openjdk.java.net/~vdeshpande/vectorizedMismatch_jdk/webrev.0 > 1/ I have also modified the bug with updated webrev. > Regards, > Vivek > > From: Deshpande, Vivek R > Sent: Tuesday, June 19, 2018 10:57 AM > To: Paul Sandoz <paul.san...@oracle.com>; roger.ri...@oracle.com > Cc: David Holmes <david.hol...@oracle.com>; > core-libs-dev@openjdk.java.net; Viswanathan, Sandhya > <sandhya.viswanat...@intel.com> > Subject: RE: RFR(S): 8205194: Improve the Array Comparison when there is a > mismatch at first element. > > Hi Roger > > I will also test with zero length arrays and let you know. > Thanks for the input. > > Regards, > Vivek > > From: Deshpande, Vivek R > Sent: Tuesday, June 19, 2018 10:17 AM > To: 'Paul Sandoz' <paul.san...@oracle.com> > Cc: David Holmes <david.hol...@oracle.com>; > core-libs-dev@openjdk.java.net; Viswanathan, Sandhya > <sandhya.viswanat...@intel.com> > Subject: RE: RFR(S): 8205194: Improve the Array Comparison when there is a > mismatch at first element. > > Thanks Paul for quick review. I will work on the things you have mentioned > and get back soon. > I will also test this with test/jdk/java/util/Arrays/ArraysEqCmpTest.java. > > Regards, > Vivek > > From: Paul Sandoz [mailto:paul.san...@oracle.com] > Sent: Tuesday, June 19, 2018 9:55 AM > To: Deshpande, Vivek R <vivek.r.deshpa...@intel.com> > Cc: David Holmes <david.hol...@oracle.com>; > core-libs-dev@openjdk.java.net; Viswanathan, Sandhya > <sandhya.viswanat...@intel.com> > Subject: Re: RFR(S): 8205194: Improve the Array Comparison when there is a > mismatch at first element. > > Hi Vivek, > > Thanks for investigating this. > > 164 public static int mismatch(boolean[] a, > 165 boolean[] b, > 166 int length) { > 167 int i = 0; > 168 if (a[i] != b[i]) > 169 return i; > You might as well replace the use if i with 0 and i think you can move this > to be performed when the length is greater than the threshold, that way you > don’t impact small arrays below the threshold. > > 186 public static int mismatch(boolean[] a, int aFromIndex, > 187 boolean[] b, int bFromIndex, > 188 int length) { > 189 int i = 0; > 190 if (a[i] != b[i]) > 191 return i; > This is incorrect. You need to use aFromIndex and bFromIndex. > > Do you run the test test/jdk/java/util/Arrays/ArraysEqCmpTest.java? If that > passed then we need to strengthen for the case of a mismatch on the first > relative element in each array. > > Paul. > > > On Jun 19, 2018, at 9:36 AM, Deshpande, Vivek R <vivek.r.deshpa...@intel.com> > wrote: > > Thanks David. > Sending it to core-libs-dev. > > I would like to contribute a patch which improves the array comparison when > there is a mismatch for the first element. > This avoids call to vectorizedMismatch method and gives ~80x speed up. > Could you please review and sponsor the patch. > Link to bug: > https://bugs.openjdk.java.net/browse/JDK-8205194 > webrev: > http://cr.openjdk.java.net/~vdeshpande/vectorizedMismatch_jdk/webrev.0 > 0/ > > Regards, > Vivek > > -----Original Message----- > From: David Holmes [mailto:david.hol...@oracle.com] > Sent: Monday, June 18, 2018 10:32 PM > To: Deshpande, Vivek R <vivek.r.deshpa...@intel.com>; > jdk-...@openjdk.java.net > Subject: Re: RFR(S): 8205194: Improve the Array Comparison when there is a > mismatch at first element. > > Hi Vivek, > > Reviews should take place on the appropriate mailing list for the code being > changed, not on the jdk-dev list. Please takes this to core-libs-dev. > > Thanks, > David > > On 19/06/2018 9:52 AM, Deshpande, Vivek R wrote: > > Hi All > > Forgot to add the links: > https://bugs.openjdk.java.net/browse/JDK-8205194 > webrev: > http://cr.openjdk.java.net/~vdeshpande/vectorizedMismatch_jdk/webrev.0 > 0/ > > Regards. > Vivek > > From: Deshpande, Vivek R > Sent: Monday, June 18, 2018 4:50 PM > To: 'jdk-...@openjdk.java.net' <jdk-...@openjdk.java.net> > Cc: 'Paul Sandoz' <paul.san...@oracle.com>; Viswanathan, Sandhya > <sandhya.viswanat...@intel.com> > Subject: RFR(S): 8205194: Improve the Array Comparison when there is a > mismatch at first element. > > Hi All > > I would like to contribute a patch which improves the array comparison when > there is a mismatch for the first element. > This avoids call to vectorizedMismatch method and gives ~80x speed up. > Please review and sponsor the patch. > > Regards, > Vivek >
