Re: RFR: 8333396: Performance regression of DecimalFormat.format [v12]
On Thu, 27 Jun 2024 11:19:44 GMT, lingjun-cg wrote:
>> 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`
>
> I know little about `record class`, it seems `record class` is help to model
> data aggregation, but here it act as a proxy class.
Yes, but since these implementations are just proxy classes, I think it is even
more the reason to make them as simple/concise as possible since we don't care
too much about what they do. (Since for most part it's the same as normal
StringBuf/Bldr)
Either is fine here, we can stick with as is if you prefer.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1657646453
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v12]
On Thu, 27 Jun 2024 05:08:06 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/StringBufFactory.java line 45:
>
>> 43: }
>> 44:
>> 45: private static class StringBufferImpl implements Format.StringBuf {
>
> The implementations may be more concise as a `record class`
I know little about `record class`, it seems `record class` is help to model
data aggregation, but here it act as a proxy class.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1656957129
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v12]
On Wed, 26 Jun 2024 09:43:32 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
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:
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v12]
On Wed, 26 Jun 2024 09:43:32 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
src/java.base/share/classes/java/text/Format.java line 193:
> 191:
> 192: StringBuilder format(Object obj,
> 193: StringBuilder toAppendTo,
Can we change this type to `StringBuf`, like `StringBuf format(Object obj,
StringBuf toAppendTo, FieldPosition pos)`? Then we can remove a few
`StringBuilder` overloads by just using the `StringBuf` version.
src/java.base/share/classes/java/text/NumberFormat.java line 424:
> 422:
> 423: StringBuilder format(double number,
> 424: StringBuilder toAppendTo,
Same remark, `StringBuf format(double number, StringBuf toAppendTo,
FieldPosition pos)`
src/java.base/share/classes/java/text/SimpleDateFormat.java line 1034:
> 1032: @Override
> 1033: public AttributedCharacterIterator formatToCharacterIterator(Object
> obj) {
> 1034: StringBuilder sb = new StringBuilder();
On second thought, we can just make this `StringBuf sb =
StringBufFactory.of(new StringBuilder())` - or just add `StringBufFactory.of()`
which defaults to create a StringBuilder-backed buf.
src/java.base/share/classes/java/text/SimpleDateFormat.java line 1448:
> 1446: numberFormat.setMaximumIntegerDigits(maxDigits);
> 1447: if (buffer.isProxyStringBuilder()) {
> 1448: numberFormat.format((long)value, buffer.asStringBuilder(),
> DontCareFieldPosition.INSTANCE);
This `numberFormat` might be a user class; if that's the case, we might do
something like:
if (buffer.isProxyStringBuilder()) {
var nf = numberFormat; // field is protected, users can change it even with
races!
if (nf.isInternalSubclass()) {
nf.format((long)value, buffer.asStringBuilder(),
DontCareFieldPosition.INSTANCE);
} else {
// format to stringbuffer, and add that buffer to the builder
buffer.append(nf.format((long)value, new StringBuffer(),
DontCareFieldPosition.INSTANCE));
}
} else { // existing code
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1655595282
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1655596623
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1655605616
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1655603917
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v12]
On Wed, 26 Jun 2024 09:43:32 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
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_r169020
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1655566915
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1655571771
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v12]
> ### 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/b6646c5d..7eb9b523
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=19513=11
- incr: https://webrevs.openjdk.org/?repo=jdk=19513=10-11
Stats: 242 lines in 9 files changed: 220 ins; 0 del; 22 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
