RFR  8041972: Add improved parse/format methods for Long/Integer
forax at univ-mlv.fr
Sun Jun 15 23:14:19 UTC 2014
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:
>>> 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
>>> Webrev: http://cr.openjdk.java.net/~redestad/8041972/webrev.0/
>> 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.
>> 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
>> 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 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()).
> 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.
More information about the core-libs-dev