-
Notifications
You must be signed in to change notification settings - Fork 6k
8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where applicable #5068
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
👋 Welcome back stsypanov! A progress list of the required criteria for merging this PR into |
@stsypanov The following labels will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me.
Could you consider adding microbenchmarks for Integer/Long.decode?
@@ -1447,15 +1447,15 @@ else if (nm.startsWith("0", index) && nm.length() > 1 + index) { | |||
throw new NumberFormatException("Sign character in wrong position"); | |||
|
|||
try { | |||
result = Integer.valueOf(nm.substring(index), radix); | |||
result = negative ? Integer.valueOf(-result.intValue()) : result; | |||
result = parseInt(nm.substring(index), radix); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly a follow-up, but I think using parseInt/-Long(nm, index, nm.length(), radix)
could give an additional speed-up in these cases (by avoiding a substring allocation).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! Let me check this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, looks like getting rid of substring()
call makes things faster:
before
Benchmark (size) Mode Cnt Score Error Units
Integers.decode 500 avgt 15 11.444 ? 0.949 us/op
Integers.parseInt 500 avgt 15 8.669 ? 0.152 us/op
Integers.toStringBig 500 avgt 15 10.295 ? 0.612 us/op
Integers.toStringSmall 500 avgt 15 7.020 ? 0.581 us/op
Benchmark (size) Mode Cnt Score Error Units
Longs.decode 500 avgt 15 29.568 ? 9.785 us/op
Longs.repetitiveSubtraction 500 avgt 15 0.820 ? 0.153 us/op
Longs.toStringBig 500 avgt 15 13.418 ? 0.744 us/op
Longs.toStringSmall 500 avgt 15 8.180 ? 0.780 us/op
after
Benchmark (size) Mode Cnt Score Error Units
Integers.decode 500 avgt 15 7.377 ? 0.040 us/op
Integers.parseInt 500 avgt 15 8.720 ? 0.230 us/op
Integers.toStringBig 500 avgt 15 10.328 ? 0.266 us/op
Integers.toStringSmall 500 avgt 15 6.913 ? 0.178 us/op
Benchmark (size) Mode Cnt Score Error Units
Longs.decode 500 avgt 15 8.373 ? 0.708 us/op
Longs.repetitiveSubtraction 500 avgt 15 0.771 ? 0.003 us/op
Longs.toStringBig 500 avgt 15 13.126 ? 0.079 us/op
Longs.toStringSmall 500 avgt 15 6.915 ? 0.259 us/op
|
@stsypanov This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 4 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@cl4es) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
Mailing list message from Thomas Lu=c3=9fnig on security-dev: Hi, i wonder why all usages of decode should be replaced. The double allocation with result = Integer.valueOf(nm.substring(index), radix); could be replace with either int result = Integer.parseInt(nm.substring(index),radix) or result = Integer.valueOf(negative this way only one place is changed and the performance of parse is Gru? Thomas Lu?nig On 10.08.2021 19:43:15, ?????? ??????? wrote: |
1 similar comment
Mailing list message from Thomas Lu=c3=9fnig on security-dev: Hi, i wonder why all usages of decode should be replaced. The double allocation with result = Integer.valueOf(nm.substring(index), radix); could be replace with either int result = Integer.parseInt(nm.substring(index),radix) or result = Integer.valueOf(negative this way only one place is changed and the performance of parse is Gru? Thomas Lu?nig On 10.08.2021 19:43:15, ?????? ??????? wrote: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
/integrate |
@stsypanov |
/sponsor |
Going to push as commit b29fbad.
Your commit was automatically rebased without conflicts. |
@cl4es @stsypanov Pushed as commit b29fbad. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Hi,
|
The pre-existing logic here is iffy, but I think it is correct. For Integer: If negative is true, then parsing "2147483648" (Integer.MAX_VALUE + 1) would throw, be reparsed as "-2147483648" and then be accepted as Integer.MIN_VALUE. This is the only case that should be non-exceptional, but it should also be exceedingly rare in practice. For negative values it improves the error messages a bit to add the "-" and reparse. Improving the catch clauses with |
OK even if we keep out the edge case in the try block the "parseLong(nm.substring(index), radix)" could be replaced as already mentioned with parseLong(nm. index, nm.length(), radix) |
I think it already was: https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/Long.java#L1287
Yes, it's slightly misleading that the radix specifier gets cropped out, but the error message does include the radix information so it's not a bug technically:
It might be a bit faster for that one non-exceptional accepted input, sure. It could also incur a cost on the fast path due increasing the weight of the code. You'd need to carefully measure that the added logic and checks doesn't cause any trouble elsewhere. |
The code in
Integer.decode()
andLong.decode()
might allocate two instances of Integer/Long for the negative values less than -127:To avoid this we can declare 'result' as
int
and useInteger.parseInt()
method. Same applicable forLong
and some other classes.Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5068/head:pull/5068
$ git checkout pull/5068
Update a local copy of the PR:
$ git checkout pull/5068
$ git pull https://git.openjdk.java.net/jdk pull/5068/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 5068
View PR using the GUI difftool:
$ git pr show -t 5068
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5068.diff