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 the changes. Since this is a performance improvement, I also would
like to have the microbenchmark that you used as the test case.
src/java.base/share/classes/java/text/Format.java line 427:
> 425: /**
> 426: * Used to distinguish JDK internal subclass and user-defined
> subclass
> 427: * of {code Format}.
There are a lot of places which is missing `@` for `code` tag in the comments.
src/java.base/share/classes/java/text/Format.java line 434:
> 432: boolean isInternalSubclass() {
> 433: return false;
> 434: }
Do we need this? Should comparing the package name with 'java.text' be
sufficient?
src/java.base/share/classes/java/text/Format.java line 438:
> 436: /**
> 437: * StringBuf is the minimal common interface of {code StringBuffer}
> and {code StringBuilder}.
> 438: * It used by the various {code Format} implementations as the
> internal string buffer.
typo: "is" is missing
src/java.base/share/classes/java/text/Format.java line 440:
> 438: * It used by the various {code Format} implementations as the
> internal string buffer.
> 439: */
> 440: interface StringBuf {
Can we make it `sealed`?
src/java.base/share/classes/java/text/ListFormat.java line 365:
> 363: return format(input, new StringBuffer(),
> 364: DontCareFieldPosition.INSTANCE).toString();
> 365: }
Since `ListFormat` is a final class, no need to have a condition, always use
StringBuilder version.
src/java.base/share/classes/java/text/StringBufFactory.java line 35:
> 33: * a better performance.
> 34: */
> 35: final class StringBufFactory {
Add a private no-arg constructor so that no instance can be created for
`StringBufFactory` itself.
-------------
PR Review: https://git.openjdk.org/jdk/pull/19513#pullrequestreview-2143027077
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1655560390
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1655563089
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1655565976
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1655559020
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1655566915
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1655571771
