JDK 8 core review request for 6989067 BigInteger's array copiers should be converted to System.arraycopy() (original) (raw)

David Holmes David.Holmes at oracle.com
Sat Sep 3 09:02:32 UTC 2011

On 3/09/2011 9:19 AM, joe.darcy at oracle.com wrote:

Modified as suggested to use Arrays.copyOf before being pushed.

The performance team recommend to me this class of change be implemented so I assume (and hope) the VM properly handles small arrays in a performance sense.

I was going to ask about performance here. Of course we should be using arraycopy and copyOf et al and if there are performance issues they should be fixed in those libraries, but in reality ...

I assume we have some performance benchmarks for BigInteger?

David

Thanks,

-Joe On 9/2/2011 3:30 PM, Rémi Forax wrote: On 09/03/2011 12:06 AM, joe.darcy at oracle.com wrote:

Hello.

Please review this simple patch to replace explicit array copy loops with System.arraycopy: 6989067 BigInteger's array copiers should be converted to System.arraycopy() http://cr.openjdk.java.net/~darcy/6989067.0/ Patch below. Thanks, -Joe Hi Joe, for me, some occurence you can use Arrays.copyOf instead of System.arraycopy in order to avoid to fill (or partially fill part of) the array with zero. manual patch inlined below :)

--- old/src/share/classes/java/math/BigInteger.java 2011-09-02 14:48:34.000000000 -0700 +++ new/src/share/classes/java/math/BigInteger.java 2011-09-02 14:48:34.000000000 -0700 @@ -1612,14 +1612,12 @@ } else { // Array must be resized if (nBits <= (32-bitsInHighWord)) {_ _int result[] = new int[nInts+len];_ _- for (int i=0; i<len; i++)_ _- result[i] = a[i];_ _+ System.arraycopy(a, 0, result, 0, len);_ _primitiveLeftShift(result, result.length, nBits);_ _return result;_ _} else {_ _int result[] = new int[nInts+len+1];_ _- for (int i=0; i<len; i++)_ _- result[i] = a[i];_ _+ System.arraycopy(a, 0, result, 0, len);_ _primitiveRightShift(result, result.length, 32 - nBits);_ _return result;_ _}_ _@@ -1908,8 +1906,7 @@_ _// Set t to high half of b_ _int[] t = new int[modLen];_ _- for(int i=0; i<modLen; i++)_ _- t[i] = b[i];_ _+ System.arraycopy(b, 0, t, 0, modLen);_ _can be simplified to:_ _int[] t = Array.copyOf(b, modLen);_ _// Fill in the table with odd powers of the base_ _for (int i=1; i<tblmask; i++) {_ _@@ -2006,14 +2003,12 @@_ _// Convert result out of Montgomery form and return_ _int[] t2 = new int[2*modLen];_ _- for(int i=0; i<modLen; i++)_ _- t2[i+modLen] = b[i];_ _+ System.arraycopy(b, 0, t2, modLen, modLen);_ _b = montReduce(t2, mod, modLen, inv);_ _t2 = new int[modLen];_ _- for(int i=0; i<modLen; i++)_ _- t2[i] = b[i];_ _+ System.arraycopy(b, 0, t2, 0, modLen);_ _can be simplified to:_ _t2 = Arrays.copyOf(b, modLen);_ _return new BigInteger(1, t2);_ _}_ _@@ -2154,8 +2149,7 @@_ _// Copy remaining ints of mag_ _int numInts = (p + 31) >>> 5; int[] mag = new int[numInts]; - for (int i=0; i<numInts; i++)_ _- mag[i] = this.mag[i + (this.mag.length - numInts)];_ _+ System.arraycopy(this.mag, (this.mag.length - numInts), mag, 0, numInts);_ _// Mask out any excess bits_ _int excessBits = (numInts << 5) - p;_ _@@ -2221,7 +2215,7 @@_ _return shiftRight(-n);_ _}_ _}_ _- int[] newMag = shiftLeft(mag,n);_ _+ int[] newMag = shiftLeft(mag, n);_ _return new BigInteger(newMag, signum);_ _}_ _@@ -2234,8 +2228,7 @@_ _if (nBits == 0) {_ _newMag = new int[magLen + nInts];_ _- for (int i=0; i<magLen; i++)_ _- newMag[i] = mag[i];_ _+ System.arraycopy(mag, 0, newMag, 0, magLen);_ _newMag = Arrays.copyOf(magLen + nInts);_ _} else {_ _int i = 0;_ _int nBits2 = 32 - nBits;_ _@@ -2289,8 +2282,7 @@_ _if (nBits == 0) {_ _int newMagLen = magLen - nInts;_ _newMag = new int[newMagLen];_ _- for (int i=0; i<newMagLen; i++)_ _- newMag[i] = mag[i];_ _+ System.arraycopy(mag, 0, newMag, 0, newMagLen);_ _newMag = Arrays.copyOf(newMagLen);_ _} else {_ _int i = 0;_ _int highBits = mag[0] >>> nBits; @@ -2561,7 +2553,7 @@ if (signum < 0) { // Check if magnitude is a power of two boolean pow2 = (Integer.bitCount(mag[0]) == 1); - for(int i=1; i< len && pow2; i++) + for (int i=1; i< len && pow2; i++) pow2 = (mag[i] == 0); n = (pow2 ? magBitLength -1 : magBitLength); Also, this change can have a negative impact because as far as I know system.arraycopy/Arrays.copyOf uses a loop even if the number of iteration is really small. A for loop will be unrolled by the VM. regards, Rémi

More information about the core-libs-dev mailing list