RFR: 8343962: [REDO] Move getChars to DecimalDigits [v6]

[Claes Redestad]

On Wed, 11 Dec 2024 22:30:32 GMT, Shaojin Wen <swen at openjdk.org> wrote:

>> This PR is a resubmission after PR #21593 was rolled back, and the unsafe offset overflow issue has been fixed.
>> 
>> 1) Move getChars methods of StringLatin1 and StringUTF16 to DecimalDigits to reduce duplication.
>> 
>> 2) HexDigits and OctalDigits also include getCharsLatin1 and getCharsUTF16
>> 
>> 3) Putting these two methods into DecimalDigits can avoid the need to expose them in JavaLangAccess
>> Eliminate duplicate code in BigDecimal
>> 
>> 4) This PR will improve the performance of Integer/Long.toString and StringBuilder.append(int/long) scenarios. This is because Unsafe.putByte is used to eliminate array bounds checks, and of course this elimination is safe. In previous versions, in Integer/Long.toString and StringBuilder.append(int/long) scenarios, -COMPACT_STRING performed better than +COMPACT_STRING. This is because StringUTF16.getChars uses StringUTF16.putChar, which is similar to Unsafe.putChar, and there is no bounds check.
>
> Shaojin Wen has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 19 additional commits since the last revision:
> 
>  - Merge remote-tracking branch 'upstream/master' into int_get_chars_dedup_202411
>  - form @cl4es
>  - Merge remote-tracking branch 'upstream/master' into int_get_chars_dedup_202411
>  - Merge remote-tracking branch 'upstream/master' into int_get_chars_dedup_202411
>  - Merge remote-tracking branch 'upstream/master' into int_get_chars_dedup_202411
>  - fix unsafe address overflow
>  - add benchmark
>  - remove comments, from @liach
>  - Merge remote-tracking branch 'upstream/master' into int_get_chars_dedup_202410
>  - fix Helper
>  - ... and 9 more: https://git.openjdk.org/jdk/compare/32ef3930...8122c1bf

Always good to see some methods removed from `JavaLangAccess` - but it appears these are already unused?

I think the performance gain from this is somewhat underwhelming. `Unsafe` is a big hammer to eek out ~10% performance in an isolated code path.

And perhaps it would be better to just push the microbenchmark, any code cleanups (like the removal of unused methods from JavaLangAccess) and leave code unchanged and leave it as a challenge for JIT engineers to get the pre-existing code to parity with this `Unsafe` routine.

That said I'm ok with this as is but will request a second opinion. Maybe @rgiulietti can weigh in?

-------------

Marked as reviewed by redestad (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/22023#pullrequestreview-2548739634