Re: RFR: 8335252: Reduce size of j.u.Formatter.Conversion#isValid [v4]
On Fri, 28 Jun 2024 12:46:49 GMT, Shaojin Wen wrote: >> 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:+PrintInlining` to see the master >> branch: >> >> @ 109 java.util.Formatter$Conversion::isValid (358 bytes) failed to >> inline: hot method too big >> >> >> current version >> >> @ 109 java.util.Formatter$Conversion::isValid (10 bytes) inline (hot) >> @ 4 >> jdk.internal.util.ImmutableBitSetPredicate$SmallImmutableBitSetPredicate::test >> (50 bytes) inline (hot) > > Shaojin Wen has updated the pull request incrementally with one additional > commit since the last revision: > > fix comment I agree with improving the legacy `String.format` code, within reason due it's wide use and appeal. Doing so does not detract from the goal of providing smarter and faster formatting via `StringTemplates`. - PR Comment: https://git.openjdk.org/jdk/pull/19926#issuecomment-2198045830
Re: RFR: 8335252: Reduce size of j.u.Formatter.Conversion#isValid [v4]
On Fri, 28 Jun 2024 12:46:49 GMT, Shaojin Wen wrote: >> 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:+PrintInlining` to see the master >> branch: >> >> @ 109 java.util.Formatter$Conversion::isValid (358 bytes) failed to >> inline: hot method too big >> >> >> current version >> >> @ 109 java.util.Formatter$Conversion::isValid (10 bytes) inline (hot) >> @ 4 >> jdk.internal.util.ImmutableBitSetPredicate$SmallImmutableBitSetPredicate::test >> (50 bytes) inline (hot) > > Shaojin Wen has updated the pull request incrementally with one additional > commit since the last revision: > > fix comment Thanks Wen Shaojin. Though in the long term we would avoid repeated parsing, this little optimization to speed up parsing is still very useful. - PR Comment: https://git.openjdk.org/jdk/pull/19926#issuecomment-2197731180
Re: RFR: 8335252: Reduce size of j.u.Formatter.Conversion#isValid [v4]
On Fri, 28 Jun 2024 16:29:02 GMT, Paul Sandoz wrote:
>> Shaojin Wen has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fix comment
>
> 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.format is widely used. In some performance-critical scenarios, when
doing performance profiling, it is often found that String.format calls consume
a lot of time. My suggestion is to replace `String.format` with `String concat`
or StringBuilder.
String info = String.format(" %s", str);
replace to
String info = " %s" + str;
or
String info = new StringBuilder(" ").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();
}
}
BenchmarkMode Cnt Score Error Units
StringFormat.stringIntFormat avgt 15 87.451 ? 2.890 ns/op
StringFormat.stringIntIndy avgt 15 6.471 ? 0.089 ns/op
StringFormat.stringIntStringBuilder avgt 15 7.949 ? 0.067 ns/op
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](https://github.com/openjdk/jdk/pull/15776)
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.
-
PR Comment: https://git.openjdk.org/jdk/pull/19926#issuecomment-2197391335
Re: RFR: 8335252: Reduce size of j.u.Formatter.Conversion#isValid [v4]
On Fri, 28 Jun 2024 12:46:49 GMT, Shaojin Wen wrote: >> 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:+PrintInlining` to see the master >> branch: >> >> @ 109 java.util.Formatter$Conversion::isValid (358 bytes) failed to >> inline: hot method too big >> >> >> current version >> >> @ 109 java.util.Formatter$Conversion::isValid (10 bytes) inline (hot) >> @ 4 >> jdk.internal.util.ImmutableBitSetPredicate$SmallImmutableBitSetPredicate::test >> (50 bytes) inline (hot) > > Shaojin Wen has updated the pull request incrementally with one additional > commit since the last revision: > > fix comment 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? - PR Comment: https://git.openjdk.org/jdk/pull/19926#issuecomment-2197275066
Re: RFR: 8335252: Reduce size of j.u.Formatter.Conversion#isValid [v4]
On Fri, 28 Jun 2024 12:46:49 GMT, Shaojin Wen wrote: >> 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:+PrintInlining` to see the master >> branch: >> >> @ 109 java.util.Formatter$Conversion::isValid (358 bytes) failed to >> inline: hot method too big >> >> >> current version >> >> @ 109 java.util.Formatter$Conversion::isValid (10 bytes) inline (hot) >> @ 4 >> jdk.internal.util.ImmutableBitSetPredicate$SmallImmutableBitSetPredicate::test >> (50 bytes) inline (hot) > > Shaojin Wen has updated the pull request incrementally with one additional > commit since the last revision: > > fix comment @wenshao Your change (at version 5707dc675b5b7d4960d3e3121a5c8fe341862d7b) is now ready to be sponsored by a Committer. - PR Comment: https://git.openjdk.org/jdk/pull/19926#issuecomment-2197261960
Re: RFR: 8335252: Reduce size of j.u.Formatter.Conversion#isValid [v4]
On Fri, 28 Jun 2024 12:46:49 GMT, Shaojin Wen wrote: >> 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:+PrintInlining` to see the master >> branch: >> >> @ 109 java.util.Formatter$Conversion::isValid (358 bytes) failed to >> inline: hot method too big >> >> >> current version >> >> @ 109 java.util.Formatter$Conversion::isValid (10 bytes) inline (hot) >> @ 4 >> jdk.internal.util.ImmutableBitSetPredicate$SmallImmutableBitSetPredicate::test >> (50 bytes) inline (hot) > > Shaojin Wen has updated the pull request incrementally with one additional > commit since the last revision: > > fix comment Thanks for the updates. I think this is now a harmless and straightforward improvement. - Marked as reviewed by redestad (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19926#pullrequestreview-2147942903
Re: RFR: 8335252: Reduce size of j.u.Formatter.Conversion#isValid [v4]
> 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:+PrintInlining` to see the master > branch: > > @ 109 java.util.Formatter$Conversion::isValid (358 bytes) failed to > inline: hot method too big > > > current version > > @ 109 java.util.Formatter$Conversion::isValid (10 bytes) inline (hot) > @ 4 > jdk.internal.util.ImmutableBitSetPredicate$SmallImmutableBitSetPredicate::test > (50 bytes) inline (hot) Shaojin Wen has updated the pull request incrementally with one additional commit since the last revision: fix comment - Changes: - all: https://git.openjdk.org/jdk/pull/19926/files - new: https://git.openjdk.org/jdk/pull/19926/files/192aa465..5707dc67 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19926=03 - incr: https://webrevs.openjdk.org/?repo=jdk=19926=02-03 Stats: 2 lines in 1 file changed: 1 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19926.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19926/head:pull/19926 PR: https://git.openjdk.org/jdk/pull/19926
