Delta [1] and revised [2] patches:

[1] http://cr.openjdk.java.net/~bpb/8229845/webrev.00-01/ 
<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 <sh...@redhat.com> wrote:
> 
> On 8/19/19 10:08 PM, Brian Burkhalter wrote:
>> [2] http://cr.openjdk.java.net/~bpb/8229845/webrev.00/ 
>> <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;

Yes, and I don’t see any way not to have it do so. Note that due to lines 
3933-3936 in [2] this case will never be hit for values which have no more than 
20 ints in their magnitude, i.e., those which do not recurse in 
toString(BigInteger,StringBuilder,int,int).

> *) This might be "static final", while we are at it:
> 
> 4109     private static String[] zeros = new String[64];

Above made final at L4111 in [2].

> On Aug 20, 2019, at 10:28 AM, Ivan Gerasimov <ivan.gerasi...@oracle.com> 
> wrote:
> 
> 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);

Changed at L4111 in [2].

> and then replace all
> 
> sb.append(zeros[x]) with sb.append(zeros, 0, x) and

L4006 in [2].

> sb.insert(pos, zeros[x]) with sb.insert(pos, zeros, 0, x).

L4049 and L4054 in [2].

> The difference would be in one extra bounds check, and as both append() and 
> insert() are relatively expensive, this check would hardly be noticeable.


Thanks,

Brian

Reply via email to