RFR [9] 8041972: Add improved parse/format methods for Long/Integer
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
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. 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. 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
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.
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. 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. /Claes [1] https://www.mail-archive.com/core-libs-dev@openjdk.java.net/msg09694.html
I've updated the patch to use CharSequence in favor of String for all new methods, as well as ensuring all new methods are package private (for now): http://cr.openjdk.java.net/~redestad/8041972/webrev.2/ Reviews appreciated! /Claes On 2014-06-15 14:29, 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.
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. 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.
/Claes
[1] https://www.mail-archive.com/core-libs-dev@openjdk.java.net/msg09694.html
On 06/16/2014 12:22 AM, Claes Redestad wrote:
I've updated the patch to use CharSequence in favor of String for all new methods, as well as ensuring all new methods are package private (for now):
http://cr.openjdk.java.net/~redestad/8041972/webrev.2/
Reviews appreciated!
/Claes
Thanks, for that, Rémi
On 2014-06-15 14:29, 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.
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. 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.
/Claes
[1] https://www.mail-archive.com/core-libs-dev@openjdk.java.net/msg09694.html
Hi Claes, The descriptions of the new methods should take the same form as the coresponding existing methods. The rationalization about intermediary objects is not useful in describing the behavior of the method and should be omitted. /** * Parses the string argument starting at fromIndex as a signed integer in the radix * specified by the second argument. ... static int parseInt(CharSequence s, int radix, int fromIndex) The terminology used in java.lang.String for offsets and indexes into strings would be provide a consistent base for talking about substrings. Thanks, Roger For example, On 6/15/2014 6:22 PM, Claes Redestad wrote:
I've updated the patch to use CharSequence in favor of String for all new methods, as well as ensuring all new methods are package private (for now):
http://cr.openjdk.java.net/~redestad/8041972/webrev.2/
Reviews appreciated!
/Claes
On 2014-06-15 14:29, 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.
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. 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.
/Claes
[1] https://www.mail-archive.com/core-libs-dev@openjdk.java.net/msg09694.html
On 2014-06-16 21:50, roger riggs wrote:
Hi Claes,
The descriptions of the new methods should take the same form as the coresponding existing methods. The rationalization about intermediary objects is not useful in describing the behavior of the method and should be omitted. Point taken!
/** * Parses the string argument starting at fromIndex as a signed integer in the radix * specified by the second argument. ...
static int parseInt(CharSequence s, int radix, int fromIndex)
The terminology used in java.lang.String for offsets and indexes into strings would be provide a consistent base for talking about substrings. If we're taking cues from String.substring, I guess int beginIndex[, int fromIndex] would be more appropriate. How about:
/** * Parses the character sequence argument in the specified {@code radix}, * beginning at the specified {@code beginIndex} and extending to the * character at index {@code endIndex - 1}. * * @see java.lang.Integer#parseInt(String, int) * @param s the {@code CharSequence} containing the integer * representation to be parsed * @param radix the radix to be used while parsing {@code s}. * @param beginIndex the beginning index, inclusive. * @param endIndex the ending index, exclusive. * @return the integer represented by the subsequence in the * specified radix. */ static int parseInt(CharSequence s, int radix, int beginIndex, int endIndex) ? Thanks! /Claes
Thanks, Roger
For example,
On 6/15/2014 6:22 PM, Claes Redestad wrote:
I've updated the patch to use CharSequence in favor of String for all new methods, as well as ensuring all new methods are package private (for now):
http://cr.openjdk.java.net/~redestad/8041972/webrev.2/
Reviews appreciated!
/Claes
On 2014-06-15 14:29, 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.
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. 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.
/Claes
[1] https://www.mail-archive.com/core-libs-dev@openjdk.java.net/msg09694.html
Yes, that looks more consistent with the current versions. Though you want to see these for 8u, the preferred pattern is to make the changes in 9 and then backport the result (in this case adding the shared secrets aspect). Roger On 6/16/2014 4:13 PM, Claes Redestad wrote:
...
The terminology used in java.lang.String for offsets and indexes into strings would be provide a consistent base for talking about substrings. If we're taking cues from String.substring, I guess int beginIndex[, int fromIndex] would be more appropriate. How about:
/** * Parses the character sequence argument in the specified {@code radix}, * beginning at the specified {@code beginIndex} and extending to the * character at index {@code endIndex - 1}. * * @see java.lang.Integer#parseInt(String, int) * @param s the {@code CharSequence} containing the integer * representation to be parsed * @param radix the radix to be used while parsing {@code s}. * @param beginIndex the beginning index, inclusive. * @param endIndex the ending index, exclusive. * @return the integer represented by the subsequence in the * specified radix. */ static int parseInt(CharSequence s, int radix, int beginIndex, int endIndex)
?
Thanks!
/Claes
Ok, I'm working on improving code and comments based on feedback. I'll split the SharedSecrets part out, make the methods public and post a new webrev soon. /Claes On 06/17/2014 04:21 PM, roger riggs wrote:
Yes, that looks more consistent with the current versions.
Though you want to see these for 8u, the preferred pattern is to make the changes in 9 and then backport the result (in this case adding the shared secrets aspect).
Roger
On 6/16/2014 4:13 PM, Claes Redestad wrote:
...
The terminology used in java.lang.String for offsets and indexes into strings would be provide a consistent base for talking about substrings. If we're taking cues from String.substring, I guess int beginIndex[, int fromIndex] would be more appropriate. How about:
/** * Parses the character sequence argument in the specified {@code radix}, * beginning at the specified {@code beginIndex} and extending to the * character at index {@code endIndex - 1}. * * @see java.lang.Integer#parseInt(String, int) * @param s the {@code CharSequence} containing the integer * representation to be parsed * @param radix the radix to be used while parsing {@code s}. * @param beginIndex the beginning index, inclusive. * @param endIndex the ending index, exclusive. * @return the integer represented by the subsequence in the * specified radix. */ static int parseInt(CharSequence s, int radix, int beginIndex, int endIndex)
?
Thanks!
/Claes
Hi, an updated webrev with reworked, public methods is available here: http://cr.openjdk.java.net/~redestad/8041972/webrev.8/ Reviews are yet again appreciated! /Claes On 06/17/2014 05:43 PM, Claes Redestad wrote:
Ok, I'm working on improving code and comments based on feedback. I'll split the SharedSecrets part out, make the methods public and post a new webrev soon.
/Claes
On 06/17/2014 04:21 PM, roger riggs wrote:
Yes, that looks more consistent with the current versions.
Though you want to see these for 8u, the preferred pattern is to make the changes in 9 and then backport the result (in this case adding the shared secrets aspect).
Roger
On 6/16/2014 4:13 PM, Claes Redestad wrote:
...
The terminology used in java.lang.String for offsets and indexes into strings would be provide a consistent base for talking about substrings. If we're taking cues from String.substring, I guess int beginIndex[, int fromIndex] would be more appropriate. How about:
/** * Parses the character sequence argument in the specified {@code radix}, * beginning at the specified {@code beginIndex} and extending to the * character at index {@code endIndex - 1}. * * @see java.lang.Integer#parseInt(String, int) * @param s the {@code CharSequence} containing the integer * representation to be parsed * @param radix the radix to be used while parsing {@code s}. * @param beginIndex the beginning index, inclusive. * @param endIndex the ending index, exclusive. * @return the integer represented by the subsequence in the * specified radix. */ static int parseInt(CharSequence s, int radix, int beginIndex, int endIndex)
?
Thanks!
/Claes
On Jun 19, 2014, at 7:39 PM, Claes Redestad <claes.redestad@oracle.com> wrote:
Hi,
an updated webrev with reworked, public methods is available here: http://cr.openjdk.java.net/~redestad/8041972/webrev.8/
Reviews are yet again appreciated!
I think "if (s == null)" or "Objects.requireNonNull(s)" is preferable to s.getClass(). (I am informed by those more knowledgeable than i that the latter also has poorer performance in the client compiler.) Related to the above, it might be preferable to retain the existing semantics of throwing NumberFormatException on a null string value, since someone might switch between parseInt(String ) and parseInt(String, int, int, int) and the null will then slip through the catch of NumberFormatException. You could just use IndexOutOfBoundsException for the case of beginIndex < 0 and beginIndex >= endIndex and endIndex > s.length(), as is similarly the case for String.substring. Again, like previously, switching between parseInt(String, int, int) parseInt(String, int, int, int) requires no additional catching. You might want to add a comment in the code that some IndexOutOfBoundsException exceptions are implicit via calls to s.charAt (i did a double take before realizing :-) ). Integer.requireNonEmpty could be a static package private method on String (or perhaps even static public, it seems useful), plus i would not bother with the static import in this case. In Integer.java#643 you can combine the if statements like you have done for the equivalent method on Long. Hth, Paul.
On 06/25/2014 06:43 PM, Paul Sandoz wrote:
On Jun 19, 2014, at 7:39 PM, Claes Redestad <claes.redestad@oracle.com> wrote:
Hi,
an updated webrev with reworked, public methods is available here: http://cr.openjdk.java.net/~redestad/8041972/webrev.8/
Reviews are yet again appreciated!
I think "if (s == null)" or "Objects.requireNonNull(s)" is preferable to s.getClass(). (I am informed by those more knowledgeable than i that the latter also has poorer performance in the client compiler.)
Agreed. Using s.getClass() was necessitated to retain performance using default compiler (-XX:+TieredCompilation) in the microbenchmarks I've been using, and going back to testing with C1 (via means of -XX:TieredStartAtLevel=1-3), it's apparent that the patch can cause a regression with the client compiler that I hadn't checked.It even looks like C2 alone (-XX:-TieredCompilation) suffers slightly. Changing to Objects.requireNonNull doesn't seem to make things better, though. Rather the regression seem to be due to C1 (and in some ways even C2) not dealing very well with the increased degrees of freedoms in the new methods, so I'm currently considering splitting apart the implementations to keep the old implementations of parseX(String[, int]) untouched while duplicating some code to build the new methods. Ugly, but I guess it's anecessary evil here.
Related to the above, it might be preferable to retain the existing semantics of throwing NumberFormatException on a null string value, since someone might switch between parseInt(String ) and parseInt(String, int, int, int) and the null will then slip through the catch of NumberFormatException.
Consider Integer.parseInt(s.substring(1)) and Integer.parseInt(s, 10, 1): the first would throw NullPointerException currently if s == null, while the latter instead would start throwing NumberFormatException. I think we should favor throwing a NPE here. I'd argue that the risk that someone changes to any of the range-based alternatives when they aren't replacing a call to substring or similar are slim to none.
You could just use IndexOutOfBoundsException for the case of beginIndex < 0 and beginIndex >= endIndex and endIndex > s.length(), as is similarly the case for String.substring. Again, like previously, switching between parseInt(String, int, int) parseInt(String, int, int, int) requires no additional catching. You might want to add a comment in the code that some IndexOutOfBoundsException exceptions are implicit via calls to s.charAt (i did a double take before realizing :-) ).
Fair points. I could argue String.substring(int, int), StringBuilder.append(CharSequence, int, int)etc are wrong, but I guess that might be a losing battle. :-)
Integer.requireNonEmpty could be a static package private method on String (or perhaps even static public, it seems useful), plus i would not bother with the static import in this case.
The requireNonEmpty throws NumberFormatException to keep compatibility with the old methods, so it wouldn't make much sense to add that to String. If I split the parseX(String... and parseX(CharSequence... implementations as I intend to, this method will be redundant though.
In Integer.java#643 you can combine the if statements like you have done for the equivalent method on Long.
Will do, thanks! /Claes
Hth, Paul.
On Jun 26, 2014, at 6:53 PM, Claes Redestad <claes.redestad@oracle.com> wrote:
On 06/25/2014 06:43 PM, Paul Sandoz wrote:
On Jun 19, 2014, at 7:39 PM, Claes Redestad <claes.redestad@oracle.com> wrote:
Hi,
an updated webrev with reworked, public methods is available here: http://cr.openjdk.java.net/~redestad/8041972/webrev.8/
Reviews are yet again appreciated!
I think "if (s == null)" or "Objects.requireNonNull(s)" is preferable to s.getClass(). (I am informed by those more knowledgeable than i that the latter also has poorer performance in the client compiler.)
Agreed. Using s.getClass() was necessitated to retain performance using default compiler (-XX:+TieredCompilation) in the microbenchmarks I've been using, and going back to testing with C1 (via means of -XX:TieredStartAtLevel=1-3), it's apparent that the patch can cause a regression with the client compiler that I hadn't checked.It even looks like C2 alone (-XX:-TieredCompilation) suffers slightly.
Changing to Objects.requireNonNull doesn't seem to make things better, though. Rather the regression seem to be due to C1 (and in some ways even C2) not dealing very well with the increased degrees of freedoms in the new methods, so I'm currently considering splitting apart the implementations to keep the old implementations of parseX(String[, int]) untouched while duplicating some code to build the new methods. Ugly, but I guess it's anecessary evil here.
Ok. Perhaps it might be possible to place the specifics of constraint checking in public methods, but they defer to a general private method that can assume it's arguments are safe (or such arguments are checked by other method calls e.g. CS.charAt)
Related to the above, it might be preferable to retain the existing semantics of throwing NumberFormatException on a null string value, since someone might switch between parseInt(String ) and parseInt(String, int, int, int) and the null will then slip through the catch of NumberFormatException.
Consider Integer.parseInt(s.substring(1)) and Integer.parseInt(s, 10, 1): the first would throw NullPointerException currently if s == null, while the latter instead would start throwing NumberFormatException. I think we should favor throwing a NPE here. I'd argue that the risk that someone changes to any of the range-based alternatives when they aren't replacing a call to substring or similar are slim to none.
A good point.
You could just use IndexOutOfBoundsException for the case of beginIndex < 0 and beginIndex >= endIndex and endIndex > s.length(), as is similarly the case for String.substring. Again, like previously, switching between parseInt(String, int, int) parseInt(String, int, int, int) requires no additional catching. You might want to add a comment in the code that some IndexOutOfBoundsException exceptions are implicit via calls to s.charAt (i did a double take before realizing :-) ).
Fair points. I could argue String.substring(int, int), StringBuilder.append(CharSequence, int, int)etc are wrong, but I guess that might be a losing battle. :-)
Right, if we were starting again it might be different but i think consistency is important.
Integer.requireNonEmpty could be a static package private method on String (or perhaps even static public, it seems useful), plus i would not bother with the static import in this case.
The requireNonEmpty throws NumberFormatException to keep compatibility with the old methods, so it wouldn't make much sense to add that to String.
Doh! what was i thinking. If it stays perhaps place it as a package private method on Number.
If I split the parseX(String... and parseX(CharSequence... implementations as I intend to, this method will be redundant though.
Ok. Paul.
Hi, updated webrev:http://cr.openjdk.java.net/~redestad/8041972/webrev.11 Changes: - Remove use of IllegalArgumentException in favor of IndexOutOfBoundsException/NumberFormatException, making the new methods behave in line with how String.substring wouldat some edge cases: "100".substring(3)equals "", thus Integer.parseInt("100", 10, 3) now throw NumberFormatException, while Integer.parseInt("100", 10, 4)/"100".substring(4) will throw IOOB. - For performance reasons the old and new methodshave been split apart. This introduces some code duplication, but removes the need to add special checks in some places. - Added more tests /Claes On 06/27/2014 10:54 AM, Paul Sandoz wrote:
On Jun 26, 2014, at 6:53 PM, Claes Redestad <claes.redestad@oracle.com> wrote:
On 06/25/2014 06:43 PM, Paul Sandoz wrote:
On Jun 19, 2014, at 7:39 PM, Claes Redestad <claes.redestad@oracle.com> wrote:
Hi,
an updated webrev with reworked, public methods is available here: http://cr.openjdk.java.net/~redestad/8041972/webrev.8/
Reviews are yet again appreciated!
I think "if (s == null)" or "Objects.requireNonNull(s)" is preferable to s.getClass(). (I am informed by those more knowledgeable than i that the latter also has poorer performance in the client compiler.) Agreed. Using s.getClass() was necessitated to retain performance using default compiler (-XX:+TieredCompilation) in the microbenchmarks I've been using, and going back to testing with C1 (via means of -XX:TieredStartAtLevel=1-3), it's apparent that the patch can cause a regression with the client compiler that I hadn't checked.It even looks like C2 alone (-XX:-TieredCompilation) suffers slightly.
Changing to Objects.requireNonNull doesn't seem to make things better, though. Rather the regression seem to be due to C1 (and in some ways even C2) not dealing very well with the increased degrees of freedoms in the new methods, so I'm currently considering splitting apart the implementations to keep the old implementations of parseX(String[, int]) untouched while duplicating some code to build the new methods. Ugly, but I guess it's anecessary evil here.
Ok. Perhaps it might be possible to place the specifics of constraint checking in public methods, but they defer to a general private method that can assume it's arguments are safe (or such arguments are checked by other method calls e.g. CS.charAt)
Related to the above, it might be preferable to retain the existing semantics of throwing NumberFormatException on a null string value, since someone might switch between parseInt(String ) and parseInt(String, int, int, int) and the null will then slip through the catch of NumberFormatException. Consider Integer.parseInt(s.substring(1)) and Integer.parseInt(s, 10, 1): the first would throw NullPointerException currently if s == null, while the latter instead would start throwing NumberFormatException. I think we should favor throwing a NPE here. I'd argue that the risk that someone changes to any of the range-based alternatives when they aren't replacing a call to substring or similar are slim to none.
A good point.
You could just use IndexOutOfBoundsException for the case of beginIndex < 0 and beginIndex >= endIndex and endIndex > s.length(), as is similarly the case for String.substring. Again, like previously, switching between parseInt(String, int, int) parseInt(String, int, int, int) requires no additional catching. You might want to add a comment in the code that some IndexOutOfBoundsException exceptions are implicit via calls to s.charAt (i did a double take before realizing :-) ). Fair points. I could argue String.substring(int, int), StringBuilder.append(CharSequence, int, int)etc are wrong, but I guess that might be a losing battle. :-)
Right, if we were starting again it might be different but i think consistency is important.
Integer.requireNonEmpty could be a static package private method on String (or perhaps even static public, it seems useful), plus i would not bother with the static import in this case. The requireNonEmpty throws NumberFormatException to keep compatibility with the old methods, so it wouldn't make much sense to add that to String. Doh! what was i thinking. If it stays perhaps place it as a package private method on Number.
If I split the parseX(String... and parseX(CharSequence... implementations as I intend to, this method will be redundant though.
Ok.
Paul.
Some comments: - The NumberFormatException.forInputString for some CharSequence is probably misleading since it doesn't consider the begin-end range which was in effect for the parsing. Rather than extracting the substring just for the error message perhaps include the index at which the error occurred. ie. add a NumberFormatException.forCharSequence(s, idx) static method - Long.java: Why not throw new NumberFormatException("Empty input") or call throw NumberFormatException.forInputString(""); - declaration of and assignment to multmin could be combined. declaration of "result" could also be moved later. - Since it's not part of the public API I recommend moving the System/JavaLangAccess changes to a separate RFE. (and it needs a test). On Jun 27 2014, at 12:02 , Claes Redestad <claes.redestad@oracle.com> wrote:
Hi,
updated webrev:http://cr.openjdk.java.net/~redestad/8041972/webrev.11
Changes: - Remove use of IllegalArgumentException in favor of IndexOutOfBoundsException/NumberFormatException, making the new methods behave in line with how String.substring wouldat some edge cases: "100".substring(3)equals "", thus Integer.parseInt("100", 10, 3) now throw NumberFormatException, while Integer.parseInt("100", 10, 4)/"100".substring(4) will throw IOOB. - For performance reasons the old and new methodshave been split apart. This introduces some code duplication, but removes the need to add special checks in some places. - Added more tests
/Claes
On 06/27/2014 10:54 AM, Paul Sandoz wrote:
On Jun 26, 2014, at 6:53 PM, Claes Redestad <claes.redestad@oracle.com> wrote:
On 06/25/2014 06:43 PM, Paul Sandoz wrote:
On Jun 19, 2014, at 7:39 PM, Claes Redestad <claes.redestad@oracle.com> wrote:
Hi,
an updated webrev with reworked, public methods is available here: http://cr.openjdk.java.net/~redestad/8041972/webrev.8/
Reviews are yet again appreciated!
I think "if (s == null)" or "Objects.requireNonNull(s)" is preferable to s.getClass(). (I am informed by those more knowledgeable than i that the latter also has poorer performance in the client compiler.) Agreed. Using s.getClass() was necessitated to retain performance using default compiler (-XX:+TieredCompilation) in the microbenchmarks I've been using, and going back to testing with C1 (via means of -XX:TieredStartAtLevel=1-3), it's apparent that the patch can cause a regression with the client compiler that I hadn't checked.It even looks like C2 alone (-XX:-TieredCompilation) suffers slightly.
Changing to Objects.requireNonNull doesn't seem to make things better, though. Rather the regression seem to be due to C1 (and in some ways even C2) not dealing very well with the increased degrees of freedoms in the new methods, so I'm currently considering splitting apart the implementations to keep the old implementations of parseX(String[, int]) untouched while duplicating some code to build the new methods. Ugly, but I guess it's anecessary evil here.
Ok. Perhaps it might be possible to place the specifics of constraint checking in public methods, but they defer to a general private method that can assume it's arguments are safe (or such arguments are checked by other method calls e.g. CS.charAt)
Related to the above, it might be preferable to retain the existing semantics of throwing NumberFormatException on a null string value, since someone might switch between parseInt(String ) and parseInt(String, int, int, int) and the null will then slip through the catch of NumberFormatException. Consider Integer.parseInt(s.substring(1)) and Integer.parseInt(s, 10, 1): the first would throw NullPointerException currently if s == null, while the latter instead would start throwing NumberFormatException. I think we should favor throwing a NPE here. I'd argue that the risk that someone changes to any of the range-based alternatives when they aren't replacing a call to substring or similar are slim to none.
A good point.
You could just use IndexOutOfBoundsException for the case of beginIndex < 0 and beginIndex >= endIndex and endIndex > s.length(), as is similarly the case for String.substring. Again, like previously, switching between parseInt(String, int, int) parseInt(String, int, int, int) requires no additional catching. You might want to add a comment in the code that some IndexOutOfBoundsException exceptions are implicit via calls to s.charAt (i did a double take before realizing :-) ). Fair points. I could argue String.substring(int, int), StringBuilder.append(CharSequence, int, int)etc are wrong, but I guess that might be a losing battle. :-)
Right, if we were starting again it might be different but i think consistency is important.
Integer.requireNonEmpty could be a static package private method on String (or perhaps even static public, it seems useful), plus i would not bother with the static import in this case. The requireNonEmpty throws NumberFormatException to keep compatibility with the old methods, so it wouldn't make much sense to add that to String. Doh! what was i thinking. If it stays perhaps place it as a package private method on Number.
If I split the parseX(String... and parseX(CharSequence... implementations as I intend to, this method will be redundant though.
Ok.
Paul.
Thank you for looking at this, Mike! New webrev: http://cr.openjdk.java.net/~redestad/8041972/webrev.12 On 2014-07-12 00:55, Mike Duigou wrote:
Some comments:
- The NumberFormatException.forInputString for some CharSequence is probably misleading since it doesn't consider the begin-end range which was in effect for the parsing. Rather than extracting the substring just for the error message perhaps include the index at which the error occurred. ie. add a NumberFormatException.forCharSequence(s, idx) static method
I think not extracting the actual substring for error messages will quickly become awkward for cases where the sequence is representing something really large and/or noisy, so I think it's safer to only output the subsequence being parsed. In line with your other suggestions, I've added a forCharSequence which takes the sequence, beginIndex, endIndex and the index at which the error was generated to produce NFEs with messages in the style of 'Error at index 5 in: "-1234S"'
- Long.java: Why not throw new NumberFormatException("Empty input") or call throw NumberFormatException.forInputString("");
Fixed!
- declaration of and assignment to multmin could be combined. declaration of "result" could also be moved later.
Fixed. I did the same for digit and narrowed the scoping of these vars.
- Since it's not part of the public API I recommend moving the System/JavaLangAccess changes to a separate RFE. (and it needs a test).
I've broken out those changes and will file a separate RFE for the formatUnsigned additions to JLS. Do you think we need to drop the "/format" from the name of this RFE? Thanks! /Claes
On Jun 27 2014, at 12:02 , Claes Redestad <claes.redestad@oracle.com> wrote:
Hi,
updated webrev:http://cr.openjdk.java.net/~redestad/8041972/webrev.11
Changes: - Remove use of IllegalArgumentException in favor of IndexOutOfBoundsException/NumberFormatException, making the new methods behave in line with how String.substring wouldat some edge cases: "100".substring(3)equals "", thus Integer.parseInt("100", 10, 3) now throw NumberFormatException, while Integer.parseInt("100", 10, 4)/"100".substring(4) will throw IOOB. - For performance reasons the old and new methodshave been split apart. This introduces some code duplication, but removes the need to add special checks in some places. - Added more tests
/Claes
On 06/27/2014 10:54 AM, Paul Sandoz wrote:
On Jun 26, 2014, at 6:53 PM, Claes Redestad <claes.redestad@oracle.com> wrote:
On 06/25/2014 06:43 PM, Paul Sandoz wrote:
On Jun 19, 2014, at 7:39 PM, Claes Redestad <claes.redestad@oracle.com> wrote:
Hi,
an updated webrev with reworked, public methods is available here: http://cr.openjdk.java.net/~redestad/8041972/webrev.8/
Reviews are yet again appreciated!
I think "if (s == null)" or "Objects.requireNonNull(s)" is preferable to s.getClass(). (I am informed by those more knowledgeable than i that the latter also has poorer performance in the client compiler.) Agreed. Using s.getClass() was necessitated to retain performance using default compiler (-XX:+TieredCompilation) in the microbenchmarks I've been using, and going back to testing with C1 (via means of -XX:TieredStartAtLevel=1-3), it's apparent that the patch can cause a regression with the client compiler that I hadn't checked.It even looks like C2 alone (-XX:-TieredCompilation) suffers slightly.
Changing to Objects.requireNonNull doesn't seem to make things better, though. Rather the regression seem to be due to C1 (and in some ways even C2) not dealing very well with the increased degrees of freedoms in the new methods, so I'm currently considering splitting apart the implementations to keep the old implementations of parseX(String[, int]) untouched while duplicating some code to build the new methods. Ugly, but I guess it's anecessary evil here.
Ok. Perhaps it might be possible to place the specifics of constraint checking in public methods, but they defer to a general private method that can assume it's arguments are safe (or such arguments are checked by other method calls e.g. CS.charAt)
Related to the above, it might be preferable to retain the existing semantics of throwing NumberFormatException on a null string value, since someone might switch between parseInt(String ) and parseInt(String, int, int, int) and the null will then slip through the catch of NumberFormatException. Consider Integer.parseInt(s.substring(1)) and Integer.parseInt(s, 10, 1): the first would throw NullPointerException currently if s == null, while the latter instead would start throwing NumberFormatException. I think we should favor throwing a NPE here. I'd argue that the risk that someone changes to any of the range-based alternatives when they aren't replacing a call to substring or similar are slim to none.
A good point.
You could just use IndexOutOfBoundsException for the case of beginIndex < 0 and beginIndex >= endIndex and endIndex > s.length(), as is similarly the case for String.substring. Again, like previously, switching between parseInt(String, int, int) parseInt(String, int, int, int) requires no additional catching. You might want to add a comment in the code that some IndexOutOfBoundsException exceptions are implicit via calls to s.charAt (i did a double take before realizing :-) ). Fair points. I could argue String.substring(int, int), StringBuilder.append(CharSequence, int, int)etc are wrong, but I guess that might be a losing battle. :-)
Right, if we were starting again it might be different but i think consistency is important.
Integer.requireNonEmpty could be a static package private method on String (or perhaps even static public, it seems useful), plus i would not bother with the static import in this case. The requireNonEmpty throws NumberFormatException to keep compatibility with the old methods, so it wouldn't make much sense to add that to String. Doh! what was i thinking. If it stays perhaps place it as a package private method on Number.
If I split the parseX(String... and parseX(CharSequence... implementations as I intend to, this method will be redundant though.
Ok.
Paul.
Hello Claes, Instead of saying * Extend upon parseFoo(String, int) in the javadoc of the new methods paired with an * @see parseFoo(String, int) please use an {@link parseFoo} instead. Thanks, -Joe On 06/15/2014 03:22 PM, Claes Redestad wrote:
I've updated the patch to use CharSequence in favor of String for all new methods, as well as ensuring all new methods are package private (for now):
http://cr.openjdk.java.net/~redestad/8041972/webrev.2/
Reviews appreciated!
/Claes
On 2014-06-15 14:29, 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.
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. 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.
/Claes
[1] https://www.mail-archive.com/core-libs-dev@openjdk.java.net/msg09694.html
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
participants (6)
-
Claes Redestad
-
Joe Darcy
-
Mike Duigou
-
Paul Sandoz
-
Remi Forax
-
roger riggs