<i18n dev> RFR: 8333396: Performance regression of DecimalFormat.format [v12]

[Naoto Sato]

On Wed, 26 Jun 2024 09:43:32 GMT, lingjun-cg <duke at 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