8335252: Reduce size of j.u.Formatter.Conversion#isValid #19926
Conversation
wenshao
changed the title
|
👋 Welcome back wenshao! A progress list of the required criteria for merging this PR into |
|
@wenshao 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 25 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. 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 |
|
@cl4es This is a scenario optimized using ImmutableBitSetPredicate |
|
Execute the following command, we can see that the javap -c java.util.Formatter.Conversion
|
wenshao
changed the title
Webrevs
|
It's surprising that javac generates synthetic cases identical to the default case for every char in the 37 to 120 range (the bounds seem to be the lowest and highest of the constants). Is there a benchmark where you see a benefit from this? EDIT: Not surprising once I was reminded that tableswitches need consecutive ranges. |
|
I found using @ForceInline gave me better performance with smaller code changes, Here are the performance numbers running on Mac M1 Max: |
wenshao
changed the title
|
Can you provide some additional context here? I think we need to be very careful about the general use @ForceInline in core libraries. While you show a modest performance benefit using a the micro benchmark, will it actually make any difference overall given formatting strings is not particular efficient? String templates, currently removed, provided good string formatting performance, and the redesign will i think also provide good performance. |
|
So the compound issue here is that there's a heuristic which prevents inlining of large methods, and that a tableswitch on inputs spanning from As an alternative to diff --git a/src/java.base/share/classes/java/util/Formatter.java b/src/java.base/share/classes/java/util/Formatter.java
index a7d95ee5780..50419f1ea23 100644
--- a/src/java.base/share/classes/java/util/Formatter.java
+++ b/src/java.base/share/classes/java/util/Formatter.java
@@ -4947,30 +4947,36 @@ static class Conversion {
static final char PERCENT_SIGN = '%';
static boolean isValid(char c) {
- return switch (c) {
- case BOOLEAN,
- BOOLEAN_UPPER,
- STRING,
- STRING_UPPER,
- HASHCODE,
- HASHCODE_UPPER,
- CHARACTER,
- CHARACTER_UPPER,
- DECIMAL_INTEGER,
- OCTAL_INTEGER,
- HEXADECIMAL_INTEGER,
- HEXADECIMAL_INTEGER_UPPER,
- SCIENTIFIC,
- SCIENTIFIC_UPPER,
- GENERAL,
- GENERAL_UPPER,
- DECIMAL_FLOAT,
- HEXADECIMAL_FLOAT,
- HEXADECIMAL_FLOAT_UPPER,
- LINE_SEPARATOR,
- PERCENT_SIGN -> true;
- default -> false;
- };
+ if (c >= 'a') {
+ return switch (c) {
+ case BOOLEAN,
+ STRING,
+ HASHCODE,
+ CHARACTER,
+ DECIMAL_INTEGER,
+ OCTAL_INTEGER,
+ HEXADECIMAL_INTEGER,
+ SCIENTIFIC,
+ GENERAL,
+ DECIMAL_FLOAT,
+ HEXADECIMAL_FLOAT,
+ LINE_SEPARATOR -> true;
+ default -> false;
+ };
+ } else {
+ return switch (c) {
+ case BOOLEAN_UPPER,
+ STRING_UPPER,
+ HASHCODE_UPPER,
+ CHARACTER_UPPER,
+ HEXADECIMAL_INTEGER_UPPER,
+ SCIENTIFIC_UPPER,
+ GENERAL_UPPER,
+ HEXADECIMAL_FLOAT_UPPER,
+ PERCENT_SIGN -> true;
+ default -> false;
+ };
+ }
}
// Returns true iff the Conversion is applicable to all objects.
And sees an improvement similar to Shrinking the size of the .class file by 121 bytes is a fun bonus. |
|
@cl4es provided a better idea, which is to change the branch of switch instead of using ForceInline. I have another approach: return switch (c) {
case BOOLEAN,
BOOLEAN_UPPER,
STRING,
STRING_UPPER,
HASHCODE,
HASHCODE_UPPER,
CHARACTER,
CHARACTER_UPPER,
DECIMAL_INTEGER,
OCTAL_INTEGER,
HEXADECIMAL_INTEGER,
HEXADECIMAL_INTEGER_UPPER,
SCIENTIFIC,
SCIENTIFIC_UPPER,
GENERAL,
GENERAL_UPPER,
DECIMAL_FLOAT,
HEXADECIMAL_FLOAT,
HEXADECIMAL_FLOAT_UPPER,
LINE_SEPARATOR -> true;
default -> c == PERCENT_SIGN;
}; |
|
This looks better. Avoids a branch up front, minimized code changes. The comment is good, but perhaps move it down into the default case? Update bug description to something like "Reduce size of j.u.Formatter.Conversion#isValid" ? |
wenshao
changed the title
cl4es
left a comment
cl4es
left a comment
There was a problem hiding this comment.
Thanks for the updates. I think this is now a harmless and straightforward improvement.
|
/integrate |
|
That's a clever change and more preferable (but still potentially fragile in the source to bytecode translation process). However I still don't understand the overall motivation, why does it matter? |
|
@PaulSandoz String info = String.format("xxxx %s", str);replace to String info = "xxxx %s" + str;or String info = new StringBuilder("xxxx ").append(str).toString();Let's do a Benchmark comparison: class StringFormat{
@Benchmark
public String stringIntFormat() {
return "%s %d".formatted(s, i);
}
@Benchmark
public String stringIntIndy() {
return s + " " + i;
}
@Benchmark
public String stringIntStringBuilder() {
return new StringBuilder(s).append(" ").append(i).toString();
}
}It can be seen that the performance of using StringConcat and StringBuilder is 10 times that of using String.format. StringTemplate (JEP 430/459/465) was an attempt to improve performance, but the API was not friendly. I was going to contribute to StringTemplate, (PR #16044 / PR #16021/PR #16017,) but StringTemplate has been removed. I'm working on improving the performance of String.format. The PR I submitted before: 8316704: Regex-free parsing of Formatter and FormatProcessor specifiers This PR is a very minor improvement, We may also need to do further work to provide a fastpath for common scenarios to improve performance. I also plan to do such work. |
|
Thanks Wen Shaojin. Though in the long term we would avoid repeated parsing, this little optimization to speed up parsing is still very useful. /sponsor |
|
Going to push as commit 5d866bf.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
I agree with improving the legacy |
4 participants
Currently, the java.util.Formatter$Conversion::isValid method is implemented based on switch, which cannot be inlined because codeSize > 325. This problem can be avoided by implementing it with ImmutableBitSetPredicate.
use
-XX:+UnlockDiagnosticVMOptions -XX:+PrintInliningto see the master branch:current version
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19926/head:pull/19926$ git checkout pull/19926Update a local copy of the PR:
$ git checkout pull/19926$ git pull https://git.openjdk.org/jdk.git pull/19926/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 19926View PR using the GUI difftool:
$ git pr show -t 19926Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19926.diff
Webrev
Link to Webrev Comment