Re: RFR: 8333396: Performance regression of DecimalFormat.format [v6]
On Mon, 17 Jun 2024 20:10:18 GMT, Chen Liang wrote:
>> For StringBuf proxy, is it acceptible for us to introduce a new jdk.internal
>> public interface (accessible only within java.base module) to expose common
>> public methods in AbstractStringBuilder? We have public types extending or
>> implementing non-public-types in the JDK (AbstractStringBuilder,
>> NamedPackage) so I guess having a new module-specific superinterface would
>> be fine? Need verification from API experts.
>
>> Hi @liach Do you know any other places within java.base where we would need
>> the same proxy for StringBuffer?
>
> Good question! I looked at
> https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/class-use/StringBuffer.html.
> I think such a new interface indeed is of limited usefulness, as we don't
> really have that many non-java.lang APIs closely tied to StringBuffer.
> Matcher is like one, but it lives mostly fine without the shadowing because
> it is using `Appendable`. And this has enlightened me.
>
> In fact, we can use `Appendable` too, as we just need 2 `append` from
> `Appendable` and `subSequence` (replacing `substring`) and `length` from
> `CharSequence`. We can declare method like:
>
> T format(double number, T toAppendTo,
> FieldPosition status) {
>
> This signature accepts both `StringBuilder` and `StringBuffer`; all use sites
> can be according updated. The only thing need to change is that `substring`
> should now become `subSequence`, but it's just used in
> `CharacterIteratorFieldDelegate` so the impact is small.
> > Hi @liach Do you know any other places within java.base where we would need
> > the same proxy for StringBuffer?
>
> Good question! I looked at
> https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/class-use/StringBuffer.html.
> I think such a new interface indeed is of limited usefulness, as we don't
> really have that many non-java.lang APIs closely tied to StringBuffer.
> Matcher is like one, but it lives mostly fine without the shadowing because
> it is using `Appendable`. And this has enlightened me.
>
> In fact, we can use `Appendable` too, as we just need 2 `append` from
> `Appendable` and `subSequence` (replacing `substring`) and `length` from
> `CharSequence`. We can declare method like:
>
> ```java
> T format(double number, T toAppendTo,
> FieldPosition status) {
> ```
>
> This signature accepts both `StringBuilder` and `StringBuffer`; all use sites
> can be according updated. The only thing need to change is that `substring`
> should now become `subSequence`, but it's just used in
> `CharacterIteratorFieldDelegate` so the impact is small.
That looks promising! Much cleaner than having dual methods
-
PR Comment: https://git.openjdk.org/jdk/pull/19513#issuecomment-2174509435
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v6]
On Mon, 17 Jun 2024 18:47:11 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
>
> For StringBuf proxy, is it acceptible for us to introduce a new jdk.internal
> public interface (accessible only within java.base module) to expose common
> public methods in AbstractStringBuilder? We have public types extending or
> implementing non-public-types in the JDK (AbstractStringBuilder,
> NamedPackage) so I guess having a new module-specific superinterface would be
> fine? Need verification from API experts.
> Hi @liach Do you know any other places within java.base where we would need
> the same proxy for StringBuffer?
Good question! I looked at
https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/class-use/StringBuffer.html.
I think such a new interface indeed is of limited usefulness, as we don't
really have that many non-java.lang APIs closely tied to StringBuffer. Matcher
is like one, but it lives mostly fine without the shadowing because it is using
`Appendable`. And this has enlightened me.
In fact, we can use `Appendable` too, as we just need 2 `append` from
`Appendable` and `subSequence` (replacing `substring`) and `length` from
`CharSequence`. We can declare method like:
T format(double number, T toAppendTo,
FieldPosition status) {
This signature accepts both `StringBuilder` and `StringBuffer`; all use sites
can be according updated. The only thing need to change is that `substring`
should now become `subSequence`, but it's just used in
`CharacterIteratorFieldDelegate` so the impact is small.
-
PR Comment: https://git.openjdk.org/jdk/pull/19513#issuecomment-2174336411
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v6]
On Mon, 17 Jun 2024 18:47:11 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 > > For StringBuf proxy, is it acceptible for us to introduce a new jdk.internal > public interface (accessible only within java.base module) to expose common > public methods in AbstractStringBuilder? We have public types extending or > implementing non-public-types in the JDK (AbstractStringBuilder, > NamedPackage) so I guess having a new module-specific superinterface would be > fine? Need verification from API experts. Hi @liach Do you know any other places within java.base where we would need the same proxy for StringBuffer? - PR Comment: https://git.openjdk.org/jdk/pull/19513#issuecomment-2174283589
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v6]
On Fri, 14 Jun 2024 03:19:48 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
For StringBuf proxy, is it acceptible for us to introduce a new jdk.internal
public interface (accessible only within java.base module) to expose common
public methods in AbstractStringBuilder? We have public types extending or
implementing non-public-types in the JDK (AbstractStringBuilder, NamedPackage)
so I guess having a new module-specific superinterface would be fine? Need
verification from API experts.
-
PR Comment: https://git.openjdk.org/jdk/pull/19513#issuecomment-2174193760
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v6]
> ### 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/1ab65e18..d590f132
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=19513&range=05
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=19513&range=04-05
Stats: 467 lines in 11 files changed: 222 ins; 188 del; 57 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
