8263038: Optimize String.format for simple specifiers #2830
Conversation
…on-trivial specifiers
|
👋 Welcome back redestad! A progress list of the required criteria for merging this PR into |
|
Microbench results - baseline: Patched: -Xint similarly sees no change on complexString, but a 3-3.5x speed-up on stringFormat |
|
/label remove i18n |
|
@cl4es |
Webrevs
|
| @@ -3012,19 +3014,19 @@ private void printCharacter(Object arg, Locale l) throws IOException { | |||
| if (arg instanceof Character) { | |||
| s = ((Character)arg).toString(); | |||
| } else if (arg instanceof Byte) { | |||
| byte i = ((Byte)arg).byteValue(); | |||
| byte i = (Byte) arg; | |||
There was a problem hiding this comment.
Can the pattern matching for instanceof be used here to remove explicit casts. (Supported in JDK 16)
if (arg instanceof Character c) {
s = c.toString();
} else if (arg instanceof Byte i) {
if (Character.isValidCodePoint(i))
s = new String(Character.toChars(i));
else
throw new IllegalFormatCodePointException(i);
} else if (arg instanceof Short i) {
if (Character.isValidCodePoint(i))
s = new String(Character.toChars(i));
else
throw new IllegalFormatCodePointException(i);
} ```
etc..
There was a problem hiding this comment.
I did think about it, but it seemed to stray a bit too far from the intent of this enhancement.
There was a problem hiding this comment.
I only looked at it because of the updates to use switch expressions...
ok, either way.
There was a problem hiding this comment.
I had reason to muck around with the switch expressions, since Conversion.isValid was inefficient (for startup) and subtly wrong (accepted 't').
Getting rid of explicit unboxing - .byteValue() - is just a syntactic improvement, so I indulged in a few places.
Using instanceof pattern matching OTOH changes bytecode shape a fair bit by introducing locals and changing the execution order. The suggestions you made above also mean we'd unbox twice. Probably inconsequential, but I'm wary of such changes when I don't have benchmarks to assert they are performance neutral.
There was a problem hiding this comment.
Makes sense, good to know... slick language features may come at a cost.
|
@cl4es 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 48 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
Piling on another optimization: The Since the zero value is only used when printing floating point number I refactored so that the localized zero is evaluated lazily, which gets numbers on the micros in line with the numbers for a US locale: |
|
Thank you for reviews, Roger and Naoto! /integrate |
openjdk
bot
added
integrated
|
@cl4es Since your change was applied there have been 49 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit f71b21b. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
This patch optimizes String.format expressions that uses trivial specifiers. In the JDK, the most common variation of String.format is a variation of format("foo: %s", s), which gets a significant speed-up from this.
Various other cleanups and minor improvements reduce overhead further and ensure we get a small gain also for more complex format strings.
Progress
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/2830/head:pull/2830$ git checkout pull/2830