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

[Justin Lu]

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 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