Re: RFR: 8333396: Performance regression of DecimalFormat.format [v5]
On Mon, 17 Jun 2024 18:17:06 GMT, Justin Lu wrote: >>> I second Justin's suggestion here. The change should benefit every >>> implementation in the JDK, not only NumberFormat. Also, I might suggest >>> renaming the new class, as it is kind of long/redundant. How about using >>> something as simple as "StringBuf"? >> >> Thanks for your comment. The long name bother me for a while. I have changed >> it to ""StringBuf". > > Hi @lingjun-cg > > Thanks for your work here. > > While this would benefit the performance of NumberFormat subclasses, we are > hoping that if we are going to make the internal switch to StringBuilder, we > can also make similar changes to other Format subclasses as well where > applicable. So, we would want `isInternalSubclass()` to be defined at the > Format level. Some methods that I can immediately think of would be `String > DateFormat.format(Date date)`, `String ListFormat.format(List input)` > and, `String MessageFormat.format(Object obj)`. > > I believe @naotoj shares the same sentiment. @justin-curtis-lu The method `isInternalSubclass` in `DecimalFormat` already pulls up to `Format`. @liach @naotoj Using `Appendable & CharSequence` instead of an interface with a limited method is a great idea. Following this idea, I have updated the PR. But there are some notes when implementing the idea. 1. Rename `format` to `formatWithGeneric` in DecimalFormat that accepts StringBuilder. If not that, throw a StackOverflowException when run with `DecimalFormat.format(double)`. If not rename the format, there are 2 methods in DecimalFormat: ``` @Override public StringBuffer format(double number, StringBuffer result, FieldPosition fieldPosition) { return format(number, result, fieldPosition); } @Override T format(double number, T result, FieldPosition fieldPosition) { } Because the second parameter type `StringBuffer` of the 1st method is more concrete than type `T` of the 2nd method, so the 1st method will call itself recursively. 2. The `Appendable#append(java.lang.CharSequence)` method can throws IOException, so there is some messy code handling IOException. - PR Comment: https://git.openjdk.org/jdk/pull/19513#issuecomment-2175778275
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v5]
On Fri, 14 Jun 2024 03:28:58 GMT, lingjun-cg wrote: >> I second Justin's suggestion here. The change should benefit every >> implementation in the JDK, not only NumberFormat. Also, I might suggest >> renaming the new class, as it is kind of long/redundant. How about using >> something as simple as "StringBuf"? > >> I second Justin's suggestion here. The change should benefit every >> implementation in the JDK, not only NumberFormat. Also, I might suggest >> renaming the new class, as it is kind of long/redundant. How about using >> something as simple as "StringBuf"? > > Thanks for your comment. The long name bother me for a while. I have changed > it to ""StringBuf". > Hi @lingjun-cg > > Thanks for your work here. > > While this would benefit the performance of NumberFormat subclasses, we are > hoping that if we are going to make the internal switch to StringBuilder, we > can also make similar changes to other Format subclasses as well where > applicable. So, we would want `isInternalSubclass()` to be defined at the > Format level. Some methods that I can immediately think of would be `String > DateFormat.format(Date date)`, `String ListFormat.format(List input)` > and, `String MessageFormat.format(Object obj)`. > > I believe @naotoj shares the same sentiment. Yes, that is what I would expect in this change. - PR Comment: https://git.openjdk.org/jdk/pull/19513#issuecomment-2174184666
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v5]
On Fri, 14 Jun 2024 03:28:58 GMT, lingjun-cg wrote: >> I second Justin's suggestion here. The change should benefit every >> implementation in the JDK, not only NumberFormat. Also, I might suggest >> renaming the new class, as it is kind of long/redundant. How about using >> something as simple as "StringBuf"? > >> I second Justin's suggestion here. The change should benefit every >> implementation in the JDK, not only NumberFormat. Also, I might suggest >> renaming the new class, as it is kind of long/redundant. How about using >> something as simple as "StringBuf"? > > Thanks for your comment. The long name bother me for a while. I have changed > it to ""StringBuf". Hi @lingjun-cg Thanks for your work here. While this would benefit the performance of NumberFormat subclasses, we are hoping that if we are going to make the internal switch to StringBuilder, we can also make similar changes to other Format subclasses as well where applicable. So, we would want `isInternalSubclass()` to be defined at the Format level. Some methods that I can immediately think of would be `String DateFormat.format(Date date)`, `String ListFormat.format(List input)` and, `String MessageFormat.format(Object obj)`. I believe @naotoj shares the same sentiment. - PR Comment: https://git.openjdk.org/jdk/pull/19513#issuecomment-2174040047
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v5]
On Fri, 14 Jun 2024 04:04:48 GMT, lingjun-cg wrote: >> 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. > > I'm not sure what you mean. Would you suggest add two new methods that accept > StringBuilderBufferProxy and keep the origin methods that accept > StringBuffer? Like this: > > interface FieldDelegate { > > public void formatted(Format.Field attr, Object value, int start, > int end, StringBuffer buffer); > > public void formatted(int fieldID, Format.Field attr, Object value, > int start, int end, StringBuffer buffer); > > public void formatted(Format.Field attr, Object value, int start, > int end, StringBuilderBufferProxy buffer); > > public void formatted(int fieldID, Format.Field attr, Object value, > int start, int end, StringBuilderBufferProxy > buffer); > } > > But DecimalFormat#subformatNumber is a common method that only accept > StringBuilderBufferProxy, and CompactNumberFormat#format(BigDecimal) call it, > so CompactNumberFormat#format(BigDecimal) need change signature. Ah, no, I was only making a observation. I agree that adding another set of methods to FieldDelegate to support both buffer and proxy is not ideal. And as you pointed out, `format(BigDecimal...` calls other methods that are reliant on proxy anyways. - PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1642223921
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v5]
On Thu, 13 Jun 2024 17:53:45 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/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. I'm not sure what you mean. Would you suggest add two new methods that accept StringBuilderBufferProxy and keep the origin methods that accept StringBuffer? Like this: interface FieldDelegate { public void formatted(Format.Field attr, Object value, int start, int end, StringBuffer buffer); public void formatted(int fieldID, Format.Field attr, Object value, int start, int end, StringBuffer buffer); public void formatted(Format.Field attr, Object value, int start, int end, StringBuilderBufferProxy buffer); public void formatted(int fieldID, Format.Field attr, Object value, int start, int end, StringBuilderBufferProxy buffer); } But DecimalFormat#subformatNumber is a common method that only accept StringBuilderBufferProxy, and CompactNumberFormat#format(BigDecimal) call it, so CompactNumberFormat#format(BigDecimal) need change signature. - PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1639197282
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v5]
On Thu, 13 Jun 2024 19:40:49 GMT, Chen Liang 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/StringBuilderBufferProxy.java line 108: > >> 106: } >> 107: >> 108: class StringBuilderImpl implements StringBuilderBufferProxy { > > Avoid adding nested classes to interfaces, which are public. These classes > are publicly accessible under reflection, and we want to avoid more of such > occurrences. See > https://bugs.openjdk.org/browse/JDK-8308040?focusedId=14581402&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14581402 Thanks for your suggestion. I split this class into 2 classes: one is the interface, the other is the factory class with package-private. - PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1639179219
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v5]
On Thu, 13 Jun 2024 19:40:48 GMT, Naoto Sato wrote: > I second Justin's suggestion here. The change should benefit every > implementation in the JDK, not only NumberFormat. Also, I might suggest > renaming the new class, as it is kind of long/redundant. How about using > something as simple as "StringBuf"? Thanks for your comment. The long name bother me for a while. I have changed it to ""StringBuf". - PR Comment: https://git.openjdk.org/jdk/pull/19513#issuecomment-2167144044
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v5]
On Wed, 5 Jun 2024 03:59:24 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 second Justin's suggestion here. The change should benefit every implementation in the JDK, not only NumberFormat. Also, I might suggest renaming the new class, as it is kind of long/redundant. How about using something as simple as "StringBuf"? - PR Comment: https://git.openjdk.org/jdk/pull/19513#issuecomment-2166636712
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v5]
On Wed, 5 Jun 2024 03:59:24 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/StringBuilderBufferProxy.java line 108: > 106: } > 107: > 108: class StringBuilderImpl implements StringBuilderBufferProxy { Avoid adding nested classes to interfaces, which are public. These classes are publicly accessible under reflection, and we want to avoid more of such occurrences. See https://bugs.openjdk.org/browse/JDK-8308040?focusedId=14581402&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14581402 - PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1638789017
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v5]
On Thu, 13 Jun 2024 16:51: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/StringBuilderBufferProxy.java line 150: > >> 148: class StringBufferImpl implements StringBuilderBufferProxy { >> 149: >> 150: private StringBuffer sb; > > Should this be `final`? In fact, we should just make them sealed interfaces implemented by 2 records. - PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1638781424
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v5]
On Wed, 5 Jun 2024 03:59:24 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 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
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v5]
> ### 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/6210c61e..1ab65e18 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19513&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19513&range=03-04 Stats: 45 lines in 8 files changed: 37 ins; 0 del; 8 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