Re: RFR: 8333396: Performance regression of DecimalFormat.format [v3]
On Tue, 4 Jun 2024 08:58:38 GMT, lingjun-cg wrote: > > > > Hi, I think you can add the jmh test case. > > > > > > > > > [#19513 > > > (comment)](https://github.com/openjdk/jdk/pull/19513#issue-2330131051) > > > already contains the test case. > > > > > > I mean it can be added as a test case in test/micro and be evaluated in > > future build. > > Thanks for your remainder. There exist a JMH test case which named > 'test/micro/org/openjdk/bench/java/text/DefFormatterBench.java'. > > This JMH test case run in throughput mode. If run without the patch, the > output is: > > ``` > Benchmark Mode Cnt ScoreError > Units > DefFormatterBench.testDefNumberFormatter thrpt5 1842.666 ? 83.694 > ops/ms > ``` > > If run with after apply the patch, the output is: > > ``` > Benchmark Mode Cnt Score Error > Units > DefFormatterBench.testDefNumberFormatter thrpt5 3988.274 ? 513.434 > ops/ms > ``` > > The score is increased by (3988.274-1842.666)/1842.666 = 116%. Yes, it's what I expected. - PR Comment: https://git.openjdk.org/jdk/pull/19513#issuecomment-2146999574
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v3]
On Tue, 4 Jun 2024 07:54:56 GMT, kuaiwei wrote: > > > Hi, I think you can add the jmh test case. > > > > > > [#19513 > > (comment)](https://github.com/openjdk/jdk/pull/19513#issue-2330131051) > > already contains the test case. > > I mean it can be added as a test case in test/micro and be evaluated in > future build. Thanks for your remainder. There exist a JMH test case which named 'test/micro/org/openjdk/bench/java/text/DefFormatterBench.java'. This JMH test case run in throughput mode. If run without the patch, the output is: Benchmark Mode Cnt ScoreError Units DefFormatterBench.testDefNumberFormatter thrpt5 1842.666 ? 83.694 ops/ms If run with after apply the patch, the output is: Benchmark Mode Cnt Score Error Units DefFormatterBench.testDefNumberFormatter thrpt5 3988.274 ? 513.434 ops/ms The score is increased by (3988.274-1842.666)/1842.666 = 116%. - PR Comment: https://git.openjdk.org/jdk/pull/19513#issuecomment-2146983441
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v3]
On Tue, 4 Jun 2024 02:18:49 GMT, lingjun-cg 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.0");
>> }
>>
>> @Benchmark
>> public void testNewAndFormat() throws InterruptedException {
>> new DecimalFormat("#0.0").format(9524234.1236457);
>> }
>>
>> @Benchmark
>> public void testNewOnly() throws InterruptedException {
>> new DecimalFormat("#0.0");
>> }
>>
>> @Benchmark
>> public void testFormatOnly() throws InterruptedException {
>> format.format(9524234.1236457);
>> }
>> }
>>
>>
>> ### Test result
>> Current JDK before optimize
>>
>> Benchmark Mode CntScore 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 CntScore Error Units
>> JmhDecimalFormat.testFormatOnlyavgt 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 CntScore Error Units
>> JmhDecimalFormat.testFormatOnlyavgt 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:
>
> 896: Performance regression of DecimalFormat.format
> > Hi, I think you can add the jmh test case.
>
> [#19513
> (comment)](https://github.com/openjdk/jdk/pull/19513#issue-2330131051)
> already contains the test case.
I mean it can be added as a test case in test/micro and be evaluated in future
build.
-
PR Comment: https://git.openjdk.org/jdk/pull/19513#issuecomment-2146853395
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v3]
On Tue, 4 Jun 2024 07:21:15 GMT, kuaiwei wrote: > Hi, I think you can add the jmh test case. https://github.com/openjdk/jdk/pull/19513#issue-2330131051 already contains the test case. - PR Comment: https://git.openjdk.org/jdk/pull/19513#issuecomment-2146844849
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v3]
On Tue, 4 Jun 2024 02:18:49 GMT, lingjun-cg 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.
>>
>> ### Test result
>>
>> @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.0");
>> }
>>
>> @Benchmark
>> public void testNewAndFormat() throws InterruptedException {
>> new DecimalFormat("#0.0").format(9524234.1236457);
>> }
>>
>> @Benchmark
>> public void testNewOnly() throws InterruptedException {
>> new DecimalFormat("#0.0");
>> }
>>
>> @Benchmark
>> public void testFormatOnly() throws InterruptedException {
>> format.format(9524234.1236457);
>> }
>> }
>>
>> Current JDK before optimize
>>
>> Benchmark Mode CntScore 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 CntScore Error Units
>> JmhDecimalFormat.testFormatOnlyavgt 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 CntScore Error Units
>> JmhDecimalFormat.testFormatOnlyavgt 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:
>
> 896: Performance regression of DecimalFormat.format
Hi, I think you can add the jmh test case.
-
PR Comment: https://git.openjdk.org/jdk/pull/19513#issuecomment-2146791250
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v3]
> ### 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.
>
> ### Test result
>
> @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.0");
> }
>
> @Benchmark
> public void testNewAndFormat() throws InterruptedException {
> new DecimalFormat("#0.0").format(9524234.1236457);
> }
>
> @Benchmark
> public void testNewOnly() throws InterruptedException {
> new DecimalFormat("#0.0");
> }
>
> @Benchmark
> public void testFormatOnly() throws InterruptedException {
> format.format(9524234.1236457);
> }
> }
>
> Current JDK before optimize
>
> Benchmark Mode CntScore 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 CntScore Error Units
> JmhDecimalFormat.testFormatOnlyavgt 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 CntScore Error Units
> JmhDecimalFormat.testFormatOnlyavgt 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:
896: Performance regression of DecimalFormat.format
-
Changes:
- all: https://git.openjdk.org/jdk/pull/19513/files
- new: https://git.openjdk.org/jdk/pull/19513/files/b48962b5..0a581428
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=19513&range=02
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=19513&range=01-02
Stats: 6 lines in 1 file changed: 0 ins; 2 del; 4 mod
Patch: https://git.openjdk.org/jdk/pull/19513.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19513/head:pull/19513
PR: https://git.openjdk.org/jdk/pull/19513
