Re: RFR: 8327791: Optimization for new BigDecimal(String) [v10]
On Fri, 26 Apr 2024 02:04:38 GMT, Joe Darcy wrote: >>> @cl4es @jddarcy All feedback has been fixed, can it be integrated? >> >> Hello @wenshao , >> >> This change will need additional review from myself or others who maintain >> BigDecimal before it can be integrated. > >> @jddarcy Sorry for the pings, Can you review this PR for me? > > Hi @wenshao , > Can you provide some additional context about the benefits of this change > beyond the micro/nano bechmark results that have been discussed. For example, > is there interesting workload the change improves, etc.? Thanks. Thanks @jddarcy, I will provide new performance test data tomorrow (Saturday, April 26th) - PR Comment: https://git.openjdk.org/jdk/pull/18177#issuecomment-2078625327
Re: RFR: 8327791: Optimization for new BigDecimal(String) [v10]
On Tue, 19 Mar 2024 19:32:10 GMT, Joe Darcy wrote: >> I think splitting `CharArraySequence` into two versions is somewhat dubious >> as more observable types at call sites may mean the performance gain in >> targeted micros is lost. How much of an improvement did you observe from >> this? Again the `char[]` constructors is probably less performance sensitive >> than the others. > >> @cl4es @jddarcy All feedback has been fixed, can it be integrated? > > Hello @wenshao , > > This change will need additional review from myself or others who maintain > BigDecimal before it can be integrated. > @jddarcy Sorry for the pings, Can you review this PR for me? Hi @wenshao , Can you provide some additional context about the benefits of this change beyond the micro/nano bechmark results that have been discussed. For example, is there interesting workload the change improves, etc.? Thanks. - PR Comment: https://git.openjdk.org/jdk/pull/18177#issuecomment-2078497794
Re: RFR: 8327791: Optimization for new BigDecimal(String) [v10]
On Tue, 19 Mar 2024 19:32:10 GMT, Joe Darcy wrote: >> I think splitting `CharArraySequence` into two versions is somewhat dubious >> as more observable types at call sites may mean the performance gain in >> targeted micros is lost. How much of an improvement did you observe from >> this? Again the `char[]` constructors is probably less performance sensitive >> than the others. > >> @cl4es @jddarcy All feedback has been fixed, can it be integrated? > > Hello @wenshao , > > This change will need additional review from myself or others who maintain > BigDecimal before it can be integrated. @jddarcy Sorry for the pings, Can you review this PR for me? - PR Comment: https://git.openjdk.org/jdk/pull/18177#issuecomment-2076967334
Re: RFR: 8327791: Optimization for new BigDecimal(String) [v10]
On Tue, 19 Mar 2024 12:12:30 GMT, Shaojin Wen wrote: >> I think splitting `CharArraySequence` into two versions is somewhat dubious >> as more observable types at call sites may mean the performance gain in >> targeted micros is lost. How much of an improvement did you observe from >> this? Again the `char[]` constructors is probably less performance sensitive >> than the others. > > @cl4es @jddarcy All feedback has been fixed, can it be integrated? @wenshao I'll take a look sometime this week. - PR Comment: https://git.openjdk.org/jdk/pull/18177#issuecomment-200708
Re: RFR: 8327791: Optimization for new BigDecimal(String) [v10]
On Tue, 12 Mar 2024 14:03:01 GMT, Claes Redestad wrote: >> Shaojin Wen has updated the pull request incrementally with one additional >> commit since the last revision: >> >> restore comment > > I think splitting `CharArraySequence` into two versions is somewhat dubious > as more observable types at call sites may mean the performance gain in > targeted micros is lost. How much of an improvement did you observe from > this? Again the `char[]` constructors is probably less performance sensitive > than the others. > @cl4es @jddarcy All feedback has been fixed, can it be integrated? Hello @wenshao , This change will need additional review from myself or others who maintain BigDecimal before it can be integrated. - PR Comment: https://git.openjdk.org/jdk/pull/18177#issuecomment-2007967764
Re: RFR: 8327791: Optimization for new BigDecimal(String) [v10]
On Tue, 12 Mar 2024 14:03:01 GMT, Claes Redestad wrote: >> Shaojin Wen has updated the pull request incrementally with one additional >> commit since the last revision: >> >> restore comment > > I think splitting `CharArraySequence` into two versions is somewhat dubious > as more observable types at call sites may mean the performance gain in > targeted micros is lost. How much of an improvement did you observe from > this? Again the `char[]` constructors is probably less performance sensitive > than the others. @cl4es @jddarcy All feedback has been fixed, can it be integrated? - PR Comment: https://git.openjdk.org/jdk/pull/18177#issuecomment-2007020206
Re: RFR: 8327791: Optimization for new BigDecimal(String) [v10]
On Tue, 12 Mar 2024 13:07:26 GMT, Shaojin Wen wrote:
>> The current BigDecimal(String) constructor calls String#toCharArray, which
>> has a memory allocation.
>>
>>
>> public BigDecimal(String val) {
>> this(val.toCharArray(), 0, val.length()); // allocate char[]
>> }
>>
>>
>> When the length is greater than 18, create a char[]
>>
>>
>> boolean isCompact = (len <= MAX_COMPACT_DIGITS); // 18
>> if (!isCompact) {
>> // ...
>> } else {
>> char[] coeff = new char[len]; // allocate char[]
>> // ...
>> }
>>
>>
>> This PR eliminates the two memory allocations mentioned above, resulting in
>> an approximate 60% increase in performance..
>
> Shaojin Wen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> restore comment
I agree with you, the char[] constructors is probably less performance
sensitive than the others.
The performance numbers under MacBookPro M1 Max are as follows:
-Benchmark Mode Cnt Score Error
Units (one CharArraySequence)
-BigDecimals.testConstructorWithSmallCharArray avgt 15 19.157 ? 0.074 ns/op
+Benchmark Mode Cnt Score Error
Units (two CharArraySequence)
+BigDecimals.testConstructorWithSmallCharArray avgt 15 17.833 ? 0.124
ns/op +7.42
-
PR Comment: https://git.openjdk.org/jdk/pull/18177#issuecomment-1991781940
Re: RFR: 8327791: Optimization for new BigDecimal(String) [v10]
On Tue, 12 Mar 2024 13:07:26 GMT, Shaojin Wen wrote:
>> The current BigDecimal(String) constructor calls String#toCharArray, which
>> has a memory allocation.
>>
>>
>> public BigDecimal(String val) {
>> this(val.toCharArray(), 0, val.length()); // allocate char[]
>> }
>>
>>
>> When the length is greater than 18, create a char[]
>>
>>
>> boolean isCompact = (len <= MAX_COMPACT_DIGITS); // 18
>> if (!isCompact) {
>> // ...
>> } else {
>> char[] coeff = new char[len]; // allocate char[]
>> // ...
>> }
>>
>>
>> This PR eliminates the two memory allocations mentioned above, resulting in
>> an approximate 60% increase in performance..
>
> Shaojin Wen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> restore comment
I think splitting `CharArraySequence` into two versions is somewhat dubious as
more observable types at call sites may mean the performance gain in targeted
micros is lost. How much of an improvement did you observe from this? Again the
`char[]` constructors is probably less performance sensitive than the others.
-
PR Comment: https://git.openjdk.org/jdk/pull/18177#issuecomment-1991727125
Re: RFR: 8327791: Optimization for new BigDecimal(String) [v10]
> The current BigDecimal(String) constructor calls String#toCharArray, which
> has a memory allocation.
>
>
> public BigDecimal(String val) {
> this(val.toCharArray(), 0, val.length()); // allocate char[]
> }
>
>
> When the length is greater than 18, create a char[]
>
>
> boolean isCompact = (len <= MAX_COMPACT_DIGITS); // 18
> if (!isCompact) {
> // ...
> } else {
> char[] coeff = new char[len]; // allocate char[]
> // ...
> }
>
>
> This PR eliminates the two memory allocations mentioned above, resulting in
> an approximate 60% increase in performance..
Shaojin Wen has updated the pull request incrementally with one additional
commit since the last revision:
restore comment
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18177/files
- new: https://git.openjdk.org/jdk/pull/18177/files/8b2a762c..e118b939
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=18177=09
- incr: https://webrevs.openjdk.org/?repo=jdk=18177=08-09
Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod
Patch: https://git.openjdk.org/jdk/pull/18177.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18177/head:pull/18177
PR: https://git.openjdk.org/jdk/pull/18177
