Integrated: 8229845: Decrease memory consumption of BigInteger.toString()

dk/commit/2ae3e51f Stats: 115 lines in 2 files changed: 57 ins; 21 del; 37 mod 8229845: Decrease memory consumption of BigInteger.toString() Reviewed-by: redestad - PR: https://git.openjdk.java.net/jdk/pull/819

Re: RFR: 8229845: Decrease memory consumption of BigInteger.toString()

On Fri, 23 Oct 2020 08:19:01 GMT, Claes Redestad wrote: >> Please review this proposed fix. The review was initially in this thread >> >> http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-August/061914.html >> >> under the old Hg SCM. The changes proposed here are derived from the final

Re: RFR: 8229845: Decrease memory consumption of BigInteger.toString()

On Thu, 22 Oct 2020 20:56:44 GMT, Brian Burkhalter wrote: > Please review this proposed fix. The review was initially in this thread > > http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-August/061914.html > > under the old Hg SCM. The changes proposed here are derived from the final >

RFR: 8229845: Decrease memory consumption of BigInteger.toString()

://mail.openjdk.java.net/pipermail/core-libs-dev/2019-August/062012.html. - Commit messages: - 8229845: Decrease memory consumption of BigInteger.toString() Changes: https://git.openjdk.java.net/jdk/pull/819/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=819&range=00

Re: 8229845: Decrease memory consumption of BigInteger.toString()

Thank you Brian! This looks good to me. With kind regards, Ivan On 8/23/19 1:33 PM, Brian Burkhalter wrote: Hi Ivan, On Aug 23, 2019, at 12:41 PM, Ivan Gerasimov mailto:ivan.gerasi...@oracle.com>> wrote: One minor comment.  Here: 3990 if (digits > 0) { 3991 padWithZeros(buf, digits); 3992

Re: 8229845: Decrease memory consumption of BigInteger.toString()

Hi Ivan, > On Aug 23, 2019, at 12:41 PM, Ivan Gerasimov > wrote: > > One minor comment. Here: > 3990 if (digits > 0) { > 3991 padWithZeros(buf, digits); > 3992 } else { > 3993 buf.append('0'); > 3994 } > > I cannot really see

Re: 8229845: Decrease memory consumption of BigInteger.toString()

Hi Brian. This looks good to me, thanks! One minor comment.  Here: 3990 if (digits > 0) { 3991 padWithZeros(buf, digits); 3992 } else { 3993 buf.append('0'); 3994 } I cannot really see how digits may be <= 0, so it seems to me i

Re: 8229845: Decrease memory consumption of BigInteger.toString()

Hi Ivan, > On Aug 22, 2019, at 1:24 PM, Ivan Gerasimov wrote: > > I have a few further suggestions how to simplify the code. > > […] Those are all good suggestions: thanks! > Here's the patch, which illustrates all the suggestions above: > http://cr.openjdk.java.net/~igerasim/8229845/00/webre

Re: 8229845: Decrease memory consumption of BigInteger.toString()

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 smallT

Re: 8229845: Decrease memory consumption of BigInteger.toString()

Hi Ivan, Please see the delta [1] and absolute [2] webrevs. > On Aug 21, 2019, at 7:38 PM, Ivan Gerasimov wrote: > > A couple of comments > > 1) > 3974 if (signum == 0) { > 3975 buf.append('0'); > 3976 return; > 3977 } > > Shouldn't it be padded with ze

Re: 8229845: Decrease memory consumption of BigInteger.toString()

Hi Brian! 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. It tha

Re: 8229845: Decrease memory consumption of BigInteger.toString()

Hello, > On Aug 20, 2019, at 4:31 PM, Brian Burkhalter > wrote: > >> On Aug 20, 2019, at 2:45 PM, Ivan Gerasimov > > wrote: >> >> Would it make sense to add an argument `digits` to smallToString (like the >> same named argument of toString, the minimum number

Re: 8229845: Decrease memory consumption of BigInteger.toString()

Hi Ivan, > On Aug 20, 2019, at 2:45 PM, Ivan Gerasimov wrote: > > Would it make sense to add an argument `digits` to smallToString (like the > same named argument of toString, the minimum number of digits to pad to), so > it could zero-pad the result itself? > > This way we could avoid insert

Re: 8229845: Decrease memory consumption of BigInteger.toString()

> On Aug 20, 2019, at 2:58 PM, Claes Redestad wrote: > > On 2019-08-20 23:56, Claes Redestad wrote: >> On 2019-08-20 10:05, Aleksey Shipilev wrote: >>> >>> *) This might be "static final", while we are at it: >>> >>> 4109 private static String[] zeros = new String[64]; >> Addressed by h

Re: 8229845: Decrease memory consumption of BigInteger.toString()

On 2019-08-20 23:56, Claes Redestad wrote: On 2019-08-20 10:05, Aleksey Shipilev wrote:   *) This might be "static final", while we are at it: 4109 private static String[] zeros = new String[64]; Addressed by http://cr.openjdk.java.net/~redestad/8228581/open.00/ Meant to link the re

Re: 8229845: Decrease memory consumption of BigInteger.toString()

On 2019-08-20 10:05, Aleksey Shipilev wrote: *) This might be "static final", while we are at it: 4109 private static String[] zeros = new String[64]; Addressed by http://cr.openjdk.java.net/~redestad/8228581/open.00/ /Claes

Re: 8229845: Decrease memory consumption of BigInteger.toString()

Hi Brian! Would it make sense to add an argument `digits` to smallToString (like the same named argument of toString, the minimum number of digits to pad to), so it could zero-pad the result itself? This way we could avoid inserting zeros into the middle of a StringBuilder after a call to sm

Re: 8229845: Decrease memory consumption of BigInteger.toString()

Delta [1] and revised [2] patches: [1] http://cr.openjdk.java.net/~bpb/8229845/webrev.00-01/ [2] http://cr.openjdk.java.net/~bpb/8229845/webrev.01/ > On Aug 20, 2019, at 1:05 AM, Aleksey Shipilev wrote: > > On 8/19/19 10:08 PM, Brian Burk

Re: 8229845: Decrease memory consumption of BigInteger.toString()

While we're here. On 8/20/19 1:05 AM, Aleksey Shipilev wrote: *) This might be "static final", while we are at it: 4109 private static String[] zeros = new String[64]; It may make sense to create the only string as private static final String zeros = "0".repeat(63); and then replace a

Re: 8229845: Decrease memory consumption of BigInteger.toString()

On 8/19/19 10:08 PM, Brian Burkhalter wrote: > [2] http://cr.openjdk.java.net/~bpb/8229845/webrev.00/ Two drive-by comments: *) It seems the "signum == 0" case in smallToString is regressing? Before, it just returned the constant-pooled "0", now it goes via StringBuilder; *) This might be "st

8229845: Decrease memory consumption of BigInteger.toString()

Please review a potential fix of [1]. BigInteger.toString(int radix) for instances with more than 20 ints in the magnitude array works by recursively decomposing the BigInteger into smaller BigIntegers which have no more than 20 ints in their magnitude array and then uses the internal method sm