On Wed, 5 Jun 2024 03:59:24 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
After an initial pass, at first glance I don't see compatibility concerns since
there no behavioral changes to the public/protected members of the
Format/Format.FieldDelegate classes/subclasses and we have left the abstract
methods untouched. This change allows internally for StringBuilder to be
utilized via the proxy and the newly added package-private methods added to
NumberFormat.
Since we are already in the scope of Format, I think something to consider is
whether we might want to include the other possible classes, such as
DateFormat.format(Date) in this change as well. Although it might get quite
messy if corresponding changes are made to all Format subclasses where
possible; just like this change, I'm sure it would kick off a chain of buffer
to proxy changes everywhere.
I'm sure there is more discussion to be had in general.
src/java.base/share/classes/java/text/CompactNumberFormat.java line 885:
> 883: }
> 884:
> 885: private StringBuilderBufferProxy format(BigInteger number,
> StringBuilderBufferProxy result,
While the DecimalFormat and CompactNumberFormat BigInteger and BigDecimal
`format` methods will always return a StringBuffer, we must change the method
signatures to use the proxy, otherwise we would need to define alternate
methods all the way at the `Format.FieldDelegate` level. So I think this is a
lesser of two evils.
src/java.base/share/classes/java/text/StringBuilderBufferProxy.java line 30:
> 28: * Provide the least interfaces that support both:
> 29: * NumberFormat#format((double|long), StringBuilder, FieldPosition)
> 30: * and NumberFormat#format(double, StringBuffer, FieldPosition)
Shouldn't this be `NumberFormat#format((double|long), StringBuffer,
FieldPosition)`?
src/java.base/share/classes/java/text/StringBuilderBufferProxy.java line 150:
> 148: class StringBufferImpl implements StringBuilderBufferProxy {
> 149:
> 150: private StringBuffer sb;
Should this be `final`?
-------------
PR Review: https://git.openjdk.org/jdk/pull/19513#pullrequestreview-2116341710
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1638667165
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1638686606
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1638540586
