RFR: 8336274: MutableBigInteger.leftShift(int) optimization [v7]

[Raffaello Giulietti]

On Sat, 7 Sep 2024 20:39:39 GMT, fabioromano1 <duke at openjdk.org> wrote:

>> This implementation of MutableBigInteger.leftShift(int) optimizes the current version, avoiding unnecessary copy of the MutableBigInteger's value content and performing the primitive shifting only in the original portion of the value array rather than in the value yet extended with trailing zeros.
>
> fabioromano1 has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Merge branch 'patchLeftShift' of https://github.com/fabioromano1/jdk into patchLeftShift
>  - Removed redundant code

test/jdk/java/math/BigInteger/MutableBigIntegerShiftTests.java line 43:

> 41:  * @build java.base/java.math.MutableBigIntegerBox
> 42:  * @key randomness
> 43:  * @run junit/othervm -DmaxDurationMillis=3000 MutableBigIntegerShiftTests

There's no need to have a max duration here, as there's no great risk of CPU consumption.

Suggestion:

 * @run junit MutableBigIntegerShiftTests

test/jdk/java/math/BigInteger/MutableBigIntegerShiftTests.java line 47:

> 45: public class MutableBigIntegerShiftTests {
> 46: 
> 47:     private static final int DEFAULT_MAX_DURATION_MILLIS = 3_000;

Remove

test/jdk/java/math/BigInteger/MutableBigIntegerShiftTests.java line 52:

> 50:     static final int ORDER_MEDIUM = 100;
> 51: 
> 52:     private static int maxDurationMillis;

Remove

test/jdk/java/math/BigInteger/MutableBigIntegerShiftTests.java line 53:

> 51: 
> 52:     private static int maxDurationMillis;
> 53:     private static Random random = RandomFactory.getRandom();

Suggestion:

    private static final Random random = RandomFactory.getRandom();

test/jdk/java/math/BigInteger/MutableBigIntegerShiftTests.java line 55:

> 53:     private static Random random = RandomFactory.getRandom();
> 54: 
> 55:     static boolean failure = false;

Remove

test/jdk/java/math/BigInteger/MutableBigIntegerShiftTests.java line 60:

> 58:     static void setMaxDurationMillis() {
> 59:         maxDurationMillis = Math.max(maxDurationMillis(), 0);
> 60:     }

Remove

test/jdk/java/math/BigInteger/MutableBigIntegerShiftTests.java line 65:

> 63:         for (int i = 0; i < 100; i++) {
> 64:             MutableBigIntegerBox x = fetchNumber(order);
> 65:             int n = Math.abs(random.nextInt() % 200);

Suggestion:

            int n = random.nextInt(200);

test/jdk/java/math/BigInteger/MutableBigIntegerShiftTests.java line 67:

> 65:             int n = Math.abs(random.nextInt() % 200);
> 66: 
> 67:             assertTrue(x.shiftLeft(n).compare

Better to use `assertEquals()`: better messages in case of failure, with an "expected" and an "actual" value.

test/jdk/java/math/BigInteger/MutableBigIntegerShiftTests.java line 71:

> 69:                     "Inconsistent left shift: " + x + "<<" + n + " != " + x + "*2^" + n);
> 70: 
> 71:             assertTrue(x.shiftLeft(n).shiftRight(n).compare(x) == 0,

Same

test/jdk/java/math/BigInteger/MutableBigIntegerShiftTests.java line 77:

> 75: 
> 76:     @Test
> 77:     public static void testShift() {

A JUnit test should be non-static.

test/jdk/java/math/BigInteger/MutableBigIntegerShiftTests.java line 79:

> 77:     public static void testShift() {
> 78:         shift(ORDER_SMALL);
> 79:         shift(ORDER_MEDIUM);

JUnit has parameterized tests, which help to identify the parameter value in case of failure.

test/jdk/java/math/BigInteger/MutableBigIntegerShiftTests.java line 107:

> 105:                 int numInts = (order + 31) >> 5;
> 106:                 int[] fullBits = new int[numInts];
> 107:                 for(int i = 0; i < numInts; i++)

`Arrays.fill()`?

test/jdk/java/math/BigInteger/MutableBigIntegerShiftTests.java line 153:

> 151: 
> 152:         return result;
> 153:     }

I think this is getting too complex for a test, there's a risk that it is incorrect...

Is there perhaps a way to have simpler code?

test/jdk/java/math/BigInteger/MutableBigIntegerShiftTests.java line 163:

> 161:         return DEFAULT_MAX_DURATION_MILLIS;
> 162:     }
> 163: }

Remove

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/20008#discussion_r1754212513
PR Review Comment: https://git.openjdk.org/jdk/pull/20008#discussion_r1754214453
PR Review Comment: https://git.openjdk.org/jdk/pull/20008#discussion_r1754214570
PR Review Comment: https://git.openjdk.org/jdk/pull/20008#discussion_r1754222080
PR Review Comment: https://git.openjdk.org/jdk/pull/20008#discussion_r1754214679
PR Review Comment: https://git.openjdk.org/jdk/pull/20008#discussion_r1754214930
PR Review Comment: https://git.openjdk.org/jdk/pull/20008#discussion_r1754215136
PR Review Comment: https://git.openjdk.org/jdk/pull/20008#discussion_r1754215802
PR Review Comment: https://git.openjdk.org/jdk/pull/20008#discussion_r1754215995
PR Review Comment: https://git.openjdk.org/jdk/pull/20008#discussion_r1754216164
PR Review Comment: https://git.openjdk.org/jdk/pull/20008#discussion_r1754216332
PR Review Comment: https://git.openjdk.org/jdk/pull/20008#discussion_r1754216672
PR Review Comment: https://git.openjdk.org/jdk/pull/20008#discussion_r1754216920
PR Review Comment: https://git.openjdk.org/jdk/pull/20008#discussion_r1754217053