On Wed, 26 Jun 2024 09:43:32 GMT, lingjun-cg <d...@openjdk.org> wrote:
>> ### Performance regression of DecimalFormat.format
>> From the output of perf, we can see the hottest regions contain atomic
>> instructions. But when run with JDK 11, there is no such problem. The
>> reason is the removed biased locking.
>> The DecimalFormat uses StringBuffer everywhere, and StringBuffer itself
>> contains many synchronized methods.
>> So I added support for some new methods that accept StringBuilder which is
>> lock-free.
>>
>> ### Benchmark testcase
>>
>> @BenchmarkMode(Mode.AverageTime)
>> @Warmup(iterations = 5, time = 500, timeUnit = TimeUnit.MILLISECONDS)
>> @Measurement(iterations = 10, time = 500, timeUnit = TimeUnit.MILLISECONDS)
>> @State(Scope.Thread)
>> @OutputTimeUnit(TimeUnit.NANOSECONDS)
>> public class JmhDecimalFormat {
>>
>> private DecimalFormat format;
>>
>> @Setup(Level.Trial)
>> public void setup() {
>> format = new DecimalFormat("#0.00000");
>> }
>>
>> @Benchmark
>> public void testNewAndFormat() throws InterruptedException {
>> new DecimalFormat("#0.00000").format(9524234.1236457);
>> }
>>
>> @Benchmark
>> public void testNewOnly() throws InterruptedException {
>> new DecimalFormat("#0.00000");
>> }
>>
>> @Benchmark
>> public void testFormatOnly() throws InterruptedException {
>> format.format(9524234.1236457);
>> }
>> }
>>
>>
>> ### Test result
>> #### Current JDK before optimize
>>
>> Benchmark Mode Cnt Score Error Units
>> JmhDecimalFormat.testFormatOnly avgt 50 642.099 ? 1.253 ns/op
>> JmhDecimalFormat.testNewAndFormat avgt 50 989.307 ? 3.676 ns/op
>> JmhDecimalFormat.testNewOnly avgt 50 303.381 ? 5.252 ns/op
>>
>>
>>
>> #### Current JDK after optimize
>>
>> Benchmark Mode Cnt Score Error Units
>> JmhDecimalFormat.testFormatOnly avgt 50 351.499 ? 0.761 ns/op
>> JmhDecimalFormat.testNewAndFormat avgt 50 615.145 ? 2.478 ns/op
>> JmhDecimalFormat.testNewOnly avgt 50 209.874 ? 9.951 ns/op
>>
>>
>> ### JDK 11
>>
>> Benchmark Mode Cnt Score Error Units
>> JmhDecimalFormat.testFormatOnly avgt 50 364.214 ? 1.191 ns/op
>> JmhDecimalFormat.testNewAndFormat avgt 50 658.699 ? 2.311 ns/op
>> JmhDecimalFormat.testNewOnly avgt 50 248.300 ? 5.158 ns/op
>
> lingjun-cg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8333396: Performance regression of DecimalFormat.format
Thanks for all the updates. Looks good to me pending the other's comments.
(BTW, the benchmark can go to -> _test/micro/org/openjdk/bench/java/text/_ as
opposed to _test/jdk/java/text_)
src/java.base/share/classes/java/text/CompactNumberFormat.java line 569:
> 567: @Override
> 568: StringBuilder format(Object number,
> 569: StringBuilder toAppendTo,
nit: indentation seems odd here/not consistent with the other corresponding
method.
src/java.base/share/classes/java/text/SimpleDateFormat.java line 1330:
> 1328:
> 1329: int num = (value / 60) * 100 + (value % 60);
> 1330: if (buffer.isProxyStringBuilder()) {
If we made the implementations of `StringBuf` records, we could use record
patterns and do away with the `isProxyStringBuilder()` method. Although we
would have to change the visibility of the implementations, so maybe not...
src/java.base/share/classes/java/text/StringBufFactory.java line 29:
> 27:
> 28: /**
> 29: * StringBufFactory create {code Format.StringBuf}'s implements that
Nit: Just to improve some grammar, how about something like,
* {@code StringBufFactory} creates implementations of {@code Format.StringBuf},
* which is an interface with the minimum overlap required to support {@code
StringBuffer}
* and {@code StringBuilder} in {@code Format}. This allows for {@code
StringBuilder} to be used
* in place of {@code StringBuffer} to provide performance benefits for JDK
internal
* {@code Format} subclasses.
src/java.base/share/classes/java/text/StringBufFactory.java line 37:
> 35: final class StringBufFactory {
> 36:
> 37: static Format.StringBuf of(StringBuffer sb) {
At least for the factory class, let's just `import
java.text.Format.StringBuf;`, so we don't have to use the full name everywhere.
src/java.base/share/classes/java/text/StringBufFactory.java line 45:
> 43: }
> 44:
> 45: private static class StringBufferImpl implements Format.StringBuf {
The implementations may be more concise as a `record class`
-------------
PR Review: https://git.openjdk.org/jdk/pull/19513#pullrequestreview-2144190566
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1656424171
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1656446980
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1656412306
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1656413186
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1656420349
