8343962: [REDO] Move getChars to DecimalDigits #22023

wenshao · 2024-11-12T01:25:16Z

This PR is a resubmission after PR #21593 was rolled back, and the unsafe offset overflow issue has been fixed.

Move getChars methods of StringLatin1 and StringUTF16 to DecimalDigits to reduce duplication.
HexDigits and OctalDigits also include getCharsLatin1 and getCharsUTF16
Putting these two methods into DecimalDigits can avoid the need to expose them in JavaLangAccess
Eliminate duplicate code in BigDecimal
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.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)

Issue

JDK-8343962: [REDO] Move getChars to DecimalDigits (Enhancement - P4)

Reviewers

Claes Redestad (@cl4es - Reviewer)
Raffaello Giulietti (@rgiulietti - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/22023/head:pull/22023
$ git checkout pull/22023

Update a local copy of the PR:
$ git checkout pull/22023
$ git pull https://git.openjdk.org/jdk.git pull/22023/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22023

View PR using the GUI difftool:
$ git pr show -t 22023

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/22023.diff

Using Webrev

Link to Webrev Comment

…up_202410

bridgekeeper · 2024-11-12T01:26:10Z

👋 Welcome back swen! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-11-12T01:26:30Z

@wenshao This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8343962: [REDO] Move getChars to DecimalDigits

Reviewed-by: redestad, rgiulietti

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 83 new commits pushed to the master branch:

bef5f56: 8291027: Some of TimeZone methods marked 'synchronized' unnecessarily
8e8f800: 8344943: Mark not subclassable classes final in java.base exported classes
76f792b: 8348203: [JVMCI] Make eager JVMCI initialization observable in the debugger
909cef5: 8339891: Several sun/security/ssl/SSLSessionImpl/* tests override test.java.opts
56a1877: 8348170: Unnecessary Hashtable usage in CSS.styleConstantToCssMap
9768f60: 8345249: Apply some conservative cleanups in FileURLConnection
c5a69b6: 8331723: Serial: Remove the unused parameter of the method SerialHeap::gc_prologue
9c55e25: 8347981: RISC-V: Add Zfa zli imm loads
e20bd01: 8344361: Restore null return for invalid services from legacy providers
5a0bdd0: 8346890: AArch64: Type profile counters generate suboptimal code
... and 73 more: https://git.openjdk.org/jdk/compare/0fbf10a9cf51d01d82cd43cf0edfaeee83313a9c...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-11-12T01:27:11Z

@wenshao The following labels will be automatically applied to this pull request:

core-libs
hotspot-compiler
security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-11-12T01:30:28Z

Webrevs

dholmes-ora · 2024-11-12T01:46:58Z

@wenshao you need a new JBS issue to complete this work under.

wenshao · 2024-11-12T02:20:59Z

/label remove security hotspot-compiler

openjdk · 2024-11-12T02:21:16Z

@wenshao
The security label was successfully removed.

The hotspot-compiler label was successfully removed.

…up_202411

rgiulietti · 2025-01-17T15:46:25Z

src/java.base/share/classes/jdk/internal/util/DecimalDigits.java

+    }
+
+    private static void putCharUTF16(byte[] buf, int charPos, int c) {
+        UNSAFE.putChar(buf, ARRAY_BYTE_BASE_OFFSET + ((long) charPos << 1), (char) c);


I'm not sure we can put a char into a byte[].
@cl4es is this safe on all platforms?

The doc of Unsafe::putChar() delegates to the doc of Unsafe::putInt() which clearly states that the Object and offset arguments must locate a variable of the same type as the one of argument x, which is not the case here.

This is safe. See

jdk/src/java.base/share/classes/java/lang/invoke/X-VarHandleByteArrayView.java.template

Lines 129 to 132 in 3804082

return UNSAFE.get$Type$Unaligned(

ba,

((long) index(ba, index)) + Unsafe.ARRAY_BYTE_BASE_OFFSET,

handle.be);

, where MethodHandles.byteArrayViewVarHandle implementation does getChar/Short/Int/Long on a byte array.

The difference there is the use of Unsafe.get/putCharUnaligned. I think putChar might work for all current platforms and modes since we're always storing at an index that is divisible by 2 in the byte array, thus aligned up to char. That said this might be an abuse of an unspecified behavior and adding -Unaligned might be a bit safer.

…up_202411

wenshao · 2025-01-20T16:09:05Z

The performance numbers below show a significant improvement for appendWithIntLatin1 and appendWithLongLatin1. However, for appendWithIntUtf16 and appendWithLongUtf16, the performance is variable, sometimes faster and sometimes slower. This inconsistency might be due to measurement errors or other factors.

1. Script

git remote add wenshao git@github.com:wenshao/jdk.git
git fetch wenshao

# baseline d9e4383cca7
git checkout d9e4383cca77ed9667ea6916624f6416f976e3bf
make test TEST="micro:java.lang.StringBuilders.appendWithInt"
make test TEST="micro:java.lang.StringBuilders.appendWithLong"

 # current be1f88abfa5
git checkout be1f88abfa505b08bda761317961fef9b79986fe
make test TEST="micro:java.lang.StringBuilders.appendWithInt"
make test TEST="micro:java.lang.StringBuilders.appendWithLong"

2. aliyun_ecs_c8a_x64 (CPU AMD EPYC™ Genoa)

-Benchmark                            Mode  Cnt    Score   Error  Units (d9e4383cca7)
-StringBuilders.appendWithIntLatin1   avgt   15  171.264 ± 2.500  ns/op
-StringBuilders.appendWithIntUtf16    avgt   15  162.430 ± 1.829  ns/op
-StringBuilders.appendWithLongLatin1  avgt   15  227.369 ± 1.452  ns/op
-StringBuilders.appendWithLongUtf16   avgt   15  214.854 ± 0.495  ns/op

+Benchmark                            Mode  Cnt    Score   Error  Units (be1f88abfa5)
+StringBuilders.appendWithIntLatin1   avgt   15  136.149 ± 0.213  ns/op +25.79%
+StringBuilders.appendWithIntUtf16    avgt   15  165.650 ± 0.762  ns/op  -1.94%
+StringBuilders.appendWithLongLatin1  avgt   15  190.156 ± 2.260  ns/op +19.56%
+StringBuilders.appendWithLongUtf16   avgt   15  224.880 ± 0.389  ns/op  -4.45%

3. aliyun_ecs_c8i_x64 (CPU Intel®Xeon®Emerald Rapids)

-Benchmark                            Mode  Cnt    Score   Error  Units (d9e4383cca7)
-StringBuilders.appendWithIntLatin1   avgt   15  176.098 ± 3.122  ns/op
-StringBuilders.appendWithIntUtf16    avgt   15  162.548 ± 0.403  ns/op
-StringBuilders.appendWithLongLatin1  avgt   15  225.868 ± 1.912  ns/op
-StringBuilders.appendWithLongUtf16   avgt   15  201.952 ± 6.429  ns/op


+Benchmark                            Mode  Cnt    Score   Error  Units (be1f88abfa5)
+StringBuilders.appendWithIntLatin1   avgt   15  145.052 ± 0.633  ns/op +21.40%
+StringBuilders.appendWithIntUtf16    avgt   15  166.678 ± 0.392  ns/op  -2.47%
+StringBuilders.appendWithLongLatin1  avgt   15  187.617 ± 1.750  ns/op +20.38%
+StringBuilders.appendWithLongUtf16   avgt   15  214.047 ± 1.687  ns/op  -5.65%

4. aliyun_ecs_c8y_aarch64 (CPU Aliyun Yitian 710 ARM v9)

-Benchmark                            Mode  Cnt    Score   Error  Units (d9e4383cca7)
-StringBuilders.appendWithIntLatin1   avgt   15  267.675 ± 0.180  ns/op
-StringBuilders.appendWithIntUtf16    avgt   15  246.470 ± 0.896  ns/op
-StringBuilders.appendWithLongLatin1  avgt   15  342.262 ± 0.292  ns/op
-StringBuilders.appendWithLongUtf16   avgt   15  327.840 ± 0.565  ns/op

+Benchmark                            Mode  Cnt    Score   Error  Units (be1f88abfa5)
+StringBuilders.appendWithIntLatin1   avgt   15  239.923 ± 1.331  ns/op +11.56%
+StringBuilders.appendWithIntUtf16    avgt   15  252.083 ± 2.270  ns/op  -2.22%
+StringBuilders.appendWithLongLatin1  avgt   15  294.602 ± 2.770  ns/op +16.17%
+StringBuilders.appendWithLongUtf16   avgt   15  324.465 ± 0.498  ns/op  +1.04%

5. MacBook M1 Pro (aarch64)

-Benchmark                            Mode  Cnt    Score   Error  Units (d9e4383cca7)
-StringBuilders.appendWithIntLatin1   avgt   15  183.633 ? 5.675  ns/op
-StringBuilders.appendWithIntUtf16    avgt   15  156.111 ? 1.045  ns/op
-StringBuilders.appendWithLongLatin1  avgt   15  246.320 ? 0.934  ns/op
-StringBuilders.appendWithLongUtf16   avgt   15  222.053 ? 1.177  ns/op


+Benchmark                            Mode  Cnt    Score   Error  Units (be1f88abfa5)
+StringBuilders.appendWithIntLatin1   avgt   15  139.442 ? 0.488  ns/op +31.69%
+StringBuilders.appendWithIntUtf16    avgt   15  155.319 ? 1.039  ns/op  +0.50%
+StringBuilders.appendWithLongLatin1  avgt   15  202.335 ? 2.133  ns/op +21.73%
+StringBuilders.appendWithLongUtf16   avgt   15  223.250 ? 0.774  ns/op  -0.53%

6. orange_pi5_aarch64 (CPU RK3588S ARMv8.4)

-Benchmark                            Mode  Cnt    Score    Error  Units (d9e4383cca7)
-StringBuilders.appendWithIntLatin1   avgt   15  460.837 ±  6.925  ns/op
-StringBuilders.appendWithIntUtf16    avgt   15  482.623 ± 16.269  ns/op
-StringBuilders.appendWithLongLatin1  avgt   15  653.808 ± 15.051  ns/op
-StringBuilders.appendWithLongUtf16   avgt   15  619.189 ± 15.581  ns/op

+Benchmark                            Mode  Cnt    Score   Error   Units
+StringBuilders.appendWithIntLatin1   avgt   15  385.403 ± 5.387   ns/op +19.57%
+StringBuilders.appendWithIntUtf16    avgt   15  439.273 ± 3.404   ns/op  +9.86%
+StringBuilders.appendWithLongLatin1  avgt   15  573.817 ± 20.499  ns/op +13.94%
+StringBuilders.appendWithLongUtf16   avgt   15  661.942 ±  0.282  ns/op  -6.45%

rgiulietti · 2025-01-20T16:19:12Z

Yes, that's what I observe as well. Your M1 results are consistent with the one I published just a few minutes earlier, so that's good.

Do you think it is worth reconsidering the UTF16 case, which is mostly slightly slower? If not, I will approve the PR.

wenshao · 2025-01-20T17:21:59Z

The UTF16 case is slower on x64 machines, and I think it's worth analyzing why.

rgiulietti · 2025-01-20T17:28:40Z

Do you prefer to postpone that analysis to a followup PR, so that we can integrate this one and move on?
Up to you ;-)

wenshao · 2025-01-21T03:05:40Z

Thanks @rgiulietti

I found out that the reason for the slowdown on x64 may not be related to the changes in this PR, because I reverted the code to (d22a286) in the current PR branch, and the performance is the same. I suspect that the master version C2 has new optimizations.

I think it can be integrated, and further optimization can be put in the next step.

My next optimization plan is to change the length of DIGITS to 128 and use DIGITS[v & 0x7f] to eliminate array bounds checks.

rgiulietti

Thanks @wenshao for your contribution.

rgiulietti · 2025-01-21T08:31:33Z

@cl4es I think you need to re-review for this PR to move to 100% approved

rgiulietti · 2025-01-22T10:33:38Z

@cl4es FYI, the changes introduced since the commit you approved are just cosmetic and stylistic. The only notable modification is the use of Unsafe::putCharUnaligned.

cl4es

Thanks, looks good.

(I'm on temporary leave, sorry for the late response)

wenshao · 2025-01-25T03:11:43Z

/integrate

openjdk · 2025-01-25T03:13:02Z

Going to push as commit f446cef.
Since your change was applied there have been 87 commits pushed to the master branch:

7c0985f: 8348420: Shenandoah: Check is_reserved before using ReservedSpace instances
f8e81d2: 8347506: Compatible OCSP readtimeout property with OCSP timeout
907350e: 8346587: Distrust TLS server certificates anchored by Camerfirma Root CAs
dec9367: 8347498: JDK 24 RDP2 L10n resource files update
bef5f56: 8291027: Some of TimeZone methods marked 'synchronized' unnecessarily
8e8f800: 8344943: Mark not subclassable classes final in java.base exported classes
76f792b: 8348203: [JVMCI] Make eager JVMCI initialization observable in the debugger
909cef5: 8339891: Several sun/security/ssl/SSLSessionImpl/* tests override test.java.opts
56a1877: 8348170: Unnecessary Hashtable usage in CSS.styleConstantToCssMap
9768f60: 8345249: Apply some conservative cleanups in FileURLConnection
... and 77 more: https://git.openjdk.org/jdk/compare/0fbf10a9cf51d01d82cd43cf0edfaeee83313a9c...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-01-25T03:13:18Z

@wenshao Pushed as commit f446cef.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

wenshao added 14 commits October 20, 2024 07:03

move getChars to DecimalDigits & deduplicate

e7aca75

add comments

2b7baad

remove unused code

7fbb399

remove JLA

fb6c741

fix import

fffa528

remove digitPair

060893f

unsafe putByte

4aa84d5

fix Helper

0484684

fix Helper

6324cf4

fix Helper

65dbe67

Merge remote-tracking branch 'upstream/master' into int_get_chars_ded…

73b3200

…up_202410

remove comments, from @liach

e97138d

add benchmark

cf947f6

fix unsafe address overflow

cd9ba30

openjdk bot added the rfr Pull request is ready for review label Nov 12, 2024

openjdk bot added security security-dev@openjdk.org hotspot-compiler hotspot-compiler-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Nov 12, 2024

wenshao changed the title ~~8342650: Move getChars to DecimalDigits~~ 8343962: Move getChars to DecimalDigits Nov 12, 2024

wenshao changed the title ~~8343962: Move getChars to DecimalDigits~~ 8343962: [REDO] Move getChars to DecimalDigits Nov 12, 2024

openjdk bot removed security security-dev@openjdk.org hotspot-compiler hotspot-compiler-dev@openjdk.org labels Nov 12, 2024

Merge remote-tracking branch 'upstream/master' into int_get_chars_ded…

ee05a6f

…up_202411

rgiulietti reviewed Jan 17, 2025

View reviewed changes

wenshao added 9 commits January 18, 2025 08:53

fix comment

3e23ab6

Coding style consistency, from rgiulietti

7df0c8b

fix benchmark, from rgiulietti

803a88a

Coding style consistency, from rgiulietti

d3d9631

from rgiulietti

c8e4fe1

bug fix

34bd9d4

use putCharUnaligned

7eb89c9

fix comments

960e279

Merge remote-tracking branch 'upstream/master' into int_get_chars_ded…

be1f88a

…up_202411

rgiulietti approved these changes Jan 21, 2025

View reviewed changes

wenshao requested a review from cl4es January 22, 2025 09:27

cl4es approved these changes Jan 24, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 24, 2025

openjdk bot added the integrated Pull request has been integrated label Jan 25, 2025

openjdk bot closed this Jan 25, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 25, 2025

liach mentioned this pull request Feb 4, 2025

8349241: Fix the concurrent execution JVM crash of StringBuilder::append(int/long) #23427

Closed

3 tasks

wenshao mentioned this pull request Mar 5, 2025

Backport toString相关优化和线程安全问题 dragonwell-project/dragonwell21#182

Open

	return UNSAFE.get$Type$Unaligned(
	ba,
	((long) index(ba, index)) + Unsafe.ARRAY_BYTE_BASE_OFFSET,
	handle.be);

8343962: [REDO] Move getChars to DecimalDigits #22023

8343962: [REDO] Move getChars to DecimalDigits #22023

Uh oh!

Conversation

wenshao commented Nov 12, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Nov 12, 2024

Uh oh!

openjdk bot commented Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Nov 12, 2024

Uh oh!

mlbridge bot commented Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

dholmes-ora commented Nov 12, 2024

Uh oh!

wenshao commented Nov 12, 2024

Uh oh!

openjdk bot commented Nov 12, 2024

Uh oh!

rgiulietti Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

rgiulietti Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

liach Jan 18, 2025

Choose a reason for hiding this comment

Uh oh!

cl4es Jan 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wenshao commented Jan 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1. Script

2. aliyun_ecs_c8a_x64 (CPU AMD EPYC™ Genoa)

3. aliyun_ecs_c8i_x64 (CPU Intel®Xeon®Emerald Rapids)

4. aliyun_ecs_c8y_aarch64 (CPU Aliyun Yitian 710 ARM v9)

5. MacBook M1 Pro (aarch64)

6. orange_pi5_aarch64 (CPU RK3588S ARMv8.4)

Uh oh!

rgiulietti commented Jan 20, 2025

Uh oh!

wenshao commented Jan 20, 2025

Uh oh!

rgiulietti commented Jan 20, 2025

Uh oh!

wenshao commented Jan 21, 2025

Uh oh!

rgiulietti left a comment

Choose a reason for hiding this comment

Uh oh!

rgiulietti commented Jan 21, 2025

Uh oh!

rgiulietti commented Jan 22, 2025

Uh oh!

cl4es left a comment

Choose a reason for hiding this comment

Uh oh!

wenshao commented Jan 25, 2025

Uh oh!

openjdk bot commented Jan 25, 2025

Uh oh!

openjdk bot commented Jan 25, 2025

Uh oh!

Uh oh!

wenshao commented Nov 12, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Nov 12, 2024 •

edited

Loading

mlbridge bot commented Nov 12, 2024 •

edited

Loading

cl4es Jan 19, 2025 •

edited

Loading

wenshao commented Jan 20, 2025 •

edited

Loading