8229845: Decrease memory consumption of BigInteger.toString()

[Ivan Gerasimov]

Thank you Brian, this looks much better!

I have a few further suggestions how to simplify the code.

1)
If this BigInteger is negative, it is negated in two places: @ 3943 and 
either @ 4003 or @ 4061.
It is better to keep abs value at the very beginning and make private 
toString(...) and smallToString() to only accept non-negative numbers.

2)
Once the 1) is done, it is possible to remove lines 3948-3954, and 
always call private toString(...), which will immediately delegate to 
smallToString() if necessary.  This is just to make the code shorter.

3)
In padWithZeros(), the condition check `if (digits > 0 && len < digits) 
{` is not necessary.
Actually, I would suggest to make a single argument, say, 
numLeadingZeros (in the current code it is `m`).

4)
I think that logic with making the argument `digits` == -1 for the first 
chunk of digits is over-complication:  `digits` can always be safely set 
to 1 in the initial call to the private toString() or smallToString().
Then the variable atBeginning is not needed at all.

5)
If 3) and 4) are done, the two lines 3988 and 3989 can be reduced to 
`padWithZeros(buf, digits);`

Here's the patch, which illustrates all the suggestions above:
http://cr.openjdk.java.net/~igerasim/8229845/00/webrev/
(I have only run the tests from test/jdk/java/math/BigInteger to verify it).

With kind regards,
Ivan


On 8/22/19 11:04 AM, Brian Burkhalter wrote:
> Hi Ivan,
>
> Please see the delta [1] and absolute [2] webrevs.
>
>> On Aug 21, 2019, at 7:38 PM, Ivan Gerasimov 
>> <ivan.gerasimov at oracle.com <mailto:ivan.gerasimov at oracle.com>> wrote:
>>
>> A couple of comments
>>
>> 1)
>> 3974 if (signum == 0) {
>> 3975 buf.append('0');
>> 3976 return;
>> 3977 }
>>
>> Shouldn't it be padded with zeroes, if digits > 0?
>> If I'm not mistaken, it could happen if result[1] at the end of 
>> toString() happens to be ZERO.
>
> Good catch: you are absolutely correct.
>
>> It that's true, then it may be a good idea to add a testcase for a 
>> number with many trailing zeros in a string representation. 
>> Currently, test/jdk/java/math/BigInteger/BigIntegerTest.java in 
>> stringConv() operates on random numbers, which are unlikely to have 
>> huge number of trailing zeros.
>
> I added testing this to stringConv(). The test fails without the most 
> recent delta applied to the implementation.
>
>> 2)
>> 4120 /* zero is a string of NUM_ZEROS consecutive zeros. */
>> 4121 private static final String zeros = "0".repeat(NUM_ZEROS);
>>
>> Nit in the comment:  name of the constant should be zero*s*.
>
> I changed the variable name to ZEROS as it is a constant.
>
> Thanks,
>
> Brian
>
> [1] http://cr.openjdk.java.net/~bpb/8229845/webrev.02-03/
> [2] http://cr.openjdk.java.net/~bpb/8229845/webrev.03/
>
-- 
With kind regards,
Ivan Gerasimov