RFR [9] 8041972: Add improved parse/format methods for Long/Integer

[Remi Forax]

On 06/15/2014 02:29 PM, Claes Redestad wrote:
>
> On 2014-06-15 13:48, Remi Forax wrote:
>>
>> On 06/15/2014 12:36 AM, Claes Redestad wrote:
>>> Hi,
>>>
>>> please review this patch to add offset based variants of 
>>> Integer.parseInt/parseUnsignedInt, Long.parseLong/parseUnsignedLong 
>>> and expose them through JavaLangAccess along with 
>>> formatUnsignedInt/-Long. This is proposed to enable a number of 
>>> minor optimizations, starting with 
>>> https://bugs.openjdk.java.net/browse/JDK-8006627
>>>
>>> Webrev: http://cr.openjdk.java.net/~redestad/8041972/webrev.0/
>>>
>>> Thanks!
>>>
>>> /Claes
>>
>> In my opinion, and as suggested in the description of 8041972, these 
>> methods should be public.
> Ultimately, yes. The ulterior motive here is to be able to backport 
> these to 8u40 (maybe even 7) for internal JDK use, while updating to 
> public (and dropping the SharedSecrets) should be done in a later, 
> 9-only update. Adding public methods would require CCC approval, more 
> detailed javadocs and possibly be part of a JEP, so pushing a 
> back-portable internal patch as a first step seems reasonable.

Ok,

>> Having written my share of scanners/parsers in Java, these methods 
>> are really helpful but
>> given that they only rely on charAt/length of String, I think they 
>> should take a CharSequence and not a String as first parameter,
>> in order to support other storages of characters like by example a 
>> java.nio.CharBuffer.
>>
>> So for me, instead of adding a bunch of method in shared secrets, 
>> those method should be public and take a CharSequence.
> I wouldn't mind switching over to CharSequence for the newly added 
> methods. I wasn't around last time[1] this was proposed for the 
> existing methods, but I know there were arguments that changing to a 
> (possibly) mutable input type might have problematic side effects that 
> would form a valid argument against this switch, while I personally 
> share the opinion that it's up to the caller to enforce immutability 
> when necessary and that text parsing methods should all use 
> CharSequence when possible.
>>
>> Miinor nits, Integer.parseInt(String,int,int) don't need to test if s 
>> is null given it delegated to parseInt(String, int, int, int),
>> and Integer.parseInt(String) should directly call parseInt(s, 10, 0, 
>> s.length()) instead of calling parseInt(s, 10) that calls
>> parseInt(s, 10, 0) that calls parseInt(s, 10, 0, s.length()).
>>
>> cheers,
>> Rémi
> Not checking for null in parseInt(String, int, int) would mean we 
> would get a NPE calling s.length() in the call to parseInt(String, 
> int, int, int), so this is needed for compatibility. 

I see, parseInt throws a NumberFormatException instead of a NPE,

> Microbenchmarks suggests the extra check does not have any performance 
> impact since the JIT can easily prove that the inner null checks can 
> be removed when an outer method.
>
> Calling parseInt(s, 10, 0, s.length()) directly in place of 
> parseInt(s, 10, 0) would likewise require an earlier null check 
> (slightly more code duplication) while being performance neutral 
> either way.

It's not performance neutral. parseInt is already used in converter 
(objects that does conversions) as a leaf call,
if you add several layers of calls, the JIT will hit the maximum depth 
threshold when inlining (10 by default),
so I think it's better to do the check in parseInt(String) (like you do 
in parseInt(String, int radix, int start))
to avoid performance regression.


>
> /Claes
>
> [1] 
> https://www.mail-archive.com/core-libs-dev@openjdk.java.net/msg09694.html

Rémi