Re: RFR: 8333396: Performance regression of DecimalFormat.format [v8]
On Tue, 25 Jun 2024 19:33:02 GMT, Naoto Sato wrote: >> I did not mean to introduce a public API for this change (if we do, the fix >> cannot be backported). I thought we could define a package private one, but >> based on your observation, it may get messier... So I agree that falling >> back to `StringBuf` is the way to go, IMO. > >> So, considering all the information given, is it enough to start our new >> review process? @naotoj @liach @justin-curtis-lu > > Well, I was suggesting the same buffer proxying for other Format classes than > NumberFormat subclasses, such as DateFormat so that they would have the same > performance benefit. Would you be willing to do that too? Thanks all for your valuable suggestions. All suggestions were accepted. Is it enough to start our new review process? @naotoj @justin-curtis-lu @liach @irisclark - PR Comment: https://git.openjdk.org/jdk/pull/19513#issuecomment-2207831524
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v8]
On Tue, 25 Jun 2024 19:33:02 GMT, Naoto Sato wrote: > > So, considering all the information given, is it enough to start our new > > review process? @naotoj @liach @justin-curtis-lu > > Well, I was suggesting the same buffer proxying for other Format classes than > NumberFormat subclasses, such as DateFormat so that they would have the same > performance benefit. Would you be willing to do that too? Sure. All `Format`'s subclasses has been replaced with buffer proxying. After that I run the benchmark test with averageTime mode. The result show the StringBuilder has take effect. Please review again. @naotoj @liach @justin-curtis-lu | Testcase | JDK 11 | JDK 22 | Current JDK | | - | - |- | - | | java.text.NumberFormat#format(double)| 362.221 | 636.049 | 351.913| | java.text.DateFormat#format(java.util.Date)| 362.273|944.733|317.436| |java.text.MessageFormat#format| 599.146| 937.717|499.584| |java.text.ListFormat#format| N/A | 464.123|318.978| - PR Comment: https://git.openjdk.org/jdk/pull/19513#issuecomment-2191307113
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v8]
On Thu, 20 Jun 2024 18:58:27 GMT, Naoto Sato wrote: >> lingjun-cg has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 896: Performance regression of DecimalFormat.format > > I did not mean to introduce a public API for this change (if we do, the fix > cannot be backported). I thought we could define a package private one, but > based on your observation, it may get messier... So I agree that falling back > to `StringBuf` is the way to go, IMO. > So, considering all the information given, is it enough to start our new > review process? @naotoj @liach @justin-curtis-lu Well, I was suggesting the same buffer proxying for other Format classes than NumberFormat subclasses, such as DateFormat so that they would have the same performance benefit. Would you be willing to do that too? - PR Comment: https://git.openjdk.org/jdk/pull/19513#issuecomment-2189821715
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v8]
On Thu, 20 Jun 2024 18:58:27 GMT, Naoto Sato wrote: >> lingjun-cg has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 896: Performance regression of DecimalFormat.format > > I did not mean to introduce a public API for this change (if we do, the fix > cannot be backported). I thought we could define a package private one, but > based on your observation, it may get messier... So I agree that falling back > to `StringBuf` is the way to go, IMO. So, considering all the information given, is it enough to start our new review process? @naotoj @liach @justin-curtis-lu - PR Comment: https://git.openjdk.org/jdk/pull/19513#issuecomment-2187842086
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v8]
On Fri, 21 Jun 2024 02:10:59 GMT, lingjun-cg wrote: > If do that ,there is some performance degradation. Thanks for taking the time to do benchmarks. If that is the case, then lets stick with the proxy solution then. - PR Comment: https://git.openjdk.org/jdk/pull/19513#issuecomment-2183118752
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v8]
On Thu, 20 Jun 2024 21:53:25 GMT, Justin Lu wrote:
> > It requires append(int), but the Appendable has no such method.
>
> If via `Appendable` we don't have access to `append(int)`, can we simply
> `append(String.valueOf(int))`. And similarly for `append(char[], int, int)`,
> can we `append(String.valueOf(char[], int, int))`.
>
> According to the method descriptions in `AbstractStringBuilder`, this should
> behaviorally be the same. That way we don't have to add any new classes, and
> can continue with the generic implementation.
If do that ,there is some performance degradation.
Benchmark Mode Cnt
Score Error Units
AppendableTest.tesAppendableAppendCharAsciiavgt 50
33.628 ± 0.263 ns/op
AppendableTest.tesAppendableAppendCharMixNoAsciiChars avgt 50
33.522 ± 0.245 ns/op
AppendableTest.testAppendableAppendInt avgt 50
32.602 ± 0.186 ns/op
AppendableTest.testStringBufferAppendCharArrayAsciiavgt 50
31.028 ± 0.134 ns/op
AppendableTest.testStringBufferAppendCharArrayMixNoAsciiChars avgt 50
31.075 ± 0.158 ns/op
AppendableTest.testStringBufferAppendInt avgt 50
25.706 ± 0.268 ns/op
@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 AppendableTest {
private StringBuffer sb;
private Appendable appendable;
private char[] asciiChars;
private char[] mixNoAsciiChars;
@Setup(Level.Iteration)
public void setup() {
sb = new StringBuffer();
appendable = sb;
asciiChars = "Criticism appears to Anatole France the most recent and
possibly the ultimate evolution".toCharArray();
mixNoAsciiChars = "The 测试数据 above mentioned two volumes of poetry were
followed by many works in prose, which we shall notice. France’s critical
writings".toCharArray();
}
@Benchmark
public void testStringBufferAppendInt() throws InterruptedException {
sb.append(12345890);
}
@Benchmark
public void testAppendableAppendInt() throws InterruptedException,
IOException {
appendable.append(String.valueOf(12345890));
}
@Benchmark
public void testStringBufferAppendCharArrayAscii() throws
InterruptedException {
sb.append(asciiChars, 40, 18);
}
@Benchmark
public void tesAppendableAppendCharAscii() throws InterruptedException,
IOException {
appendable.append(new String(asciiChars, 40, 18));
}
@Benchmark
public void testStringBufferAppendCharArrayMixNoAsciiChars() throws
InterruptedException {
sb.append(mixNoAsciiChars, 40, 18);
}
@Benchmark
public void tesAppendableAppendCharMixNoAsciiChars() throws
InterruptedException, IOException {
appendable.append(new String(mixNoAsciiChars, 40, 18));
}
}
-
PR Comment: https://git.openjdk.org/jdk/pull/19513#issuecomment-2181851223
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v8]
On Wed, 19 Jun 2024 02:05:28 GMT, lingjun-cg wrote: > It requires append(int), but the Appendable has no such method. If via `Appendable` we don't have access to `append(int)`, can we simply `append(String.valueOf(int))`. And similarly for `append(char[], int, int)`, can we `append(String.valueOf(char[], int, int))`. According to the method descriptions in `AbstractStringBuilder`, this should behaviorally be the same. That way we don't have to add any new classes, and can continue with the generic implementation. - PR Comment: https://git.openjdk.org/jdk/pull/19513#issuecomment-2181620101
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v8]
On Tue, 18 Jun 2024 10:00:33 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
A `jdk.internal.access.StringBuf` that `AbstractStringBuilder` implements may
work, right?
-
PR Comment: https://git.openjdk.org/jdk/pull/19513#issuecomment-2181402131
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v8]
On Tue, 18 Jun 2024 10:00:33 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
I did not mean to introduce a public API for this change (if we do, the fix
cannot be backported). I thought we could define a package private one, but
based on your observation, it may get messier... So I agree that falling back
to `StringBuf` is the way to go, IMO.
-
PR Comment: https://git.openjdk.org/jdk/pull/19513#issuecomment-2181339942
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v8]
On Tue, 18 Jun 2024 10:00:33 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
If change the visibility of `java.lang.AbstractStringBuilder` from
package-private to public, it seems more simple.
The `AbstractStringBuilder` is a sealed class, user code cannot extend it, and
it also used by other class like `String`.
-
PR Comment: https://git.openjdk.org/jdk/pull/19513#issuecomment-2179655141
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v8]
On Wed, 19 Jun 2024 02:12:01 GMT, lingjun-cg wrote:
>> src/java.base/share/classes/java/text/Format.java line 278:
>>
>>> 276: * {@code false} otherwise
>>> 277: */
>>> 278: boolean isInternalSubclass() {
>>
>> Since this is defined in Format, can we apply similar changes of
>> StringBuilder formatting to the other Format subclasses beyond just
>> NumberFormat.
>>
>> For example, in DateFormat, something such as,
>>
>>
>> T formatWithGeneric(Date date,
>>T toAppendTo,
>>FieldPosition pos) {
>> throw new UnsupportedOperationException("Subclasses should override
>> this method");
>> }
>
> ok. I will update it if we have a conclusion about using `StringBuf` or
> using ``.
This comment inspire me that use '' in
`SimpleDateFormat` is impossible, because the 'Appendable' lack of necessary
`append` method which SimpleDateFormat requires.
So we need discuss whether `StringBuf` is a better solution?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1645309140
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v8]
On Tue, 18 Jun 2024 20:39:30 GMT, Justin Lu wrote:
>> lingjun-cg has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 896: Performance regression of DecimalFormat.format
>
> src/java.base/share/classes/java/text/ChoiceFormat.java line 561:
>
>> 559: toAppendTo.append(choiceFormats[i]);
>> 560: } catch (IOException ioe) {
>> 561: throw new UncheckedIOException(ioe.getMessage(), ioe);
>
> Perhaps `AssertionError` instead of `UncheckedIOException` is better suited
> here and in the other ocurrences.
This can be removed if using `StringBuf`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1645304136
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v8]
On Tue, 18 Jun 2024 20:36:52 GMT, Justin Lu wrote:
>> lingjun-cg has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 896: Performance regression of DecimalFormat.format
>
> src/java.base/share/classes/java/text/Format.java line 278:
>
>> 276: * {@code false} otherwise
>> 277: */
>> 278: boolean isInternalSubclass() {
>
> Since this is defined in Format, can we apply similar changes of
> StringBuilder formatting to the other Format subclasses beyond just
> NumberFormat.
>
> For example, in DateFormat, something such as,
>
>
> T formatWithGeneric(Date date,
>T toAppendTo,
>FieldPosition pos) {
> throw new UnsupportedOperationException("Subclasses should override
> this method");
> }
ok. I will update it if we have a conclusion about using `StringBuf` or using
``.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1645300958
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v8]
On Tue, 18 Jun 2024 10:00:33 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
Using ` ` instead of `StringBuf` seems a
good idea, but it has a disadvantage. The `Appendable` defines only 3 `append`
methods which cannot cover some cases. For example, this method
`SimpleDateFormat#format(Date, StringBuffer, FieldDelegate)` contains the
following code:
case TAG_QUOTE_CHARS:
toAppendTo.append(compiledPattern, i, count);
i += count;
break;
It requires `append(char[], int, int)`, but the `Appendable` has no such method.
So it's difficult to use ` ` in methods
`String DateFormat.format(Date date), String ListFormat.format(List
input) and, String MessageFormat.format(Object obj)`.
The method in `java.text.MessageFormat#subformat` contains the following code:
int argumentNumber = argumentNumbers[i];
if (arguments == null || argumentNumber >= arguments.length) {
result.append('{').append(argumentNumber).append('}');
continue;
}
It requires `append(int)`, but the `Appendable` has no such method.
To sum up
1. ` ` is clear, but lack of extensibility
2. StringBuf is not so clear, but it's more extensibility. We can add
`append(char[], int, int)` to `StringBuf` to support `SimpleDateFormat`.
3. New interface StringBuf no need to handle IOException, but use ` ` need to add a lot of try...catch statements around
the append method.
4.
So it seems `StringBuf` is better if we want to fix the problem in
`DateFormat,ListFormat,MessageFormat` in unique way.
I look forward to your reply. @naotoj @liach @justin-curtis-lu
Using ` ` instead of `StringBuf` seems a
good idea, but it has a disadvantage. The `Appendable` defines only 3 `append`
methods which cannot cover some cases. For example, this method
`SimpleDateFormat#format(Date, StringBuffer, FieldDelegate)` contains the
following code:
case TAG_QUOTE_CHARS:
toAppendTo.append(compiledPattern, i, count);
i += count;
break;
It requires `append(char[], int, int)`, but the `Appendable` has no such method.
So it's difficult to use ` ` in methods
`String DateFormat.format(Date date), String ListFormat.format(List
input) and, String MessageFormat.format(Object obj)`.
The method in `java.text.MessageFormat#subformat` contains the following code:
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v8]
On Tue, 18 Jun 2024 10:00:33 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
> If not that, throw a StackOverflowException when run with
> DecimalFormat.format(double)
I'm not sure I understand this point.
I think this is on the right track and looks much better with the generic
implementation.
src/java.base/share/classes/java/text/ChoiceFormat.java line 561:
> 559: toAppendTo.append(choiceFormats[i]);
> 560: } catch (IOException ioe) {
> 561: throw new UncheckedIOException(ioe.getMessage(), ioe);
Perhaps `AssertionError` instead of `UncheckedIOException` is better suited
here and in the other ocurrences.
src/java.base/share/classes/java/text/Format.java line 278:
> 276: * {@code false} otherwise
> 277: */
> 278: boolean isInternalSubclass() {
Since this is defined in Format, can we apply similar changes of StringBuilder
formatting to the other Format subclasses beyond just NumberFormat.
For example, in DateFormat, something such as,
T formatWithGeneric(Date date,
T toAppendTo,
FieldPosition pos) {
throw new UnsupportedOperationException("Subclasses should override
this method");
}
-
PR Review: https://git.openjdk.org/jdk/pull/19513#pullrequestreview-2126447188
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1645029030
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1645026453
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v8]
> ### 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
-
Changes:
- all: https://git.openjdk.org/jdk/pull/19513/files
- new: https://git.openjdk.org/jdk/pull/19513/files/e95f0928..1fde6a60
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=19513&range=07
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=19513&range=06-07
Stats: 186 lines in 8 files changed: 130 ins; 0 del; 56 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
