8342650: Move getChars to DecimalDigits #21593

wenshao · 2024-10-20T00:19:25Z

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

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8342650: Move getChars to DecimalDigits (Enhancement - P4)

Reviewers

Chen Liang (@liach - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21593

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-10-20T00:19:59Z

👋 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-10-20T00:20:27Z

@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:

8342650: Move getChars to DecimalDigits

Reviewed-by: liach

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 72 new commits pushed to the master branch:

423e8e0: 8339162: [REDO] JDK-8338440 Parallel: Improve fragmentation mitigation in Full GC
f400896: 8342442: Static ACVP sample tests
325a2c3: 8343848: Fix typo of property name in TestOAEPPadding after 8341927
2614c99: 8343793: Test java/foreign/TestMemorySession.java is timing out
8a69893: 8343819: Link Float.NaN and Double.NaN to equivalence discussion in Double
52c0b09: 8277240: java/awt/Graphics2D/ScaledTransform/ScaledTransform.java dialog does not get disposed
0329855: 8343804: Show the default time zone with -XshowSettings option
44ec501: 8305895: Implement JEP 450: Compact Object Headers (Experimental)
6053962: 8343684: Test KDFDelayedProviderSyncTest.java intermittent fails didn't finish within the time-out 150
2fb0c1d: 8343844: Add benchmarks for superword/autovectorization in FFM BulkOperations
... and 62 more: https://git.openjdk.org/jdk/compare/5890d9438bbde88b89070052926a2eafe13d7b42...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-10-20T00:20:51Z

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

core-libs
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-10-20T07:57:19Z

Webrevs

j3graham · 2024-10-20T17:33:09Z

src/java.base/share/classes/java/math/BigDecimal.java

        char[] coeff;
        int offset;  // offset is the starting index for coeff array
        // Get the significand as an absolute value
        if (intCompact != INFLATED) {
-            offset = sbHelper.putIntCompact(Math.abs(intCompact));
-            coeff  = sbHelper.getCompactCharArray();
+            coeff = new char[19];


A possibility here would be to change coeff to be a String. The “else” branch already creates a string and has to additionally create a char array from it. If this is the only place where the DecimalDigits.getChars(… char[]) is used, some extra code duplication could be removed.

If this PR can be merged, we can continue to complete PR #16006, which can also remove DecimalDigits.getChars(… char[])

@j3graham I have submitted PR #22009

wenshao · 2024-10-22T23:18:15Z

/label remove security

openjdk · 2024-10-22T23:19:17Z

@wenshao
The security label was successfully removed.

…up_202410

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

liach · 2024-11-06T01:44:38Z

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

+    public static void putPairLatin1(byte[] buf, int charPos, int v) {
+        int packed = DIGITS[v];
+        putCharLatin1(buf, charPos, packed & 0xFF);
+        putCharLatin1(buf, charPos + 1, packed >> 8);


Does merge store work here? Original code in StringLatin1 uses direct array writes, not sure about this version.

Tests show that the current version of MergeStore also works

[TraceMergeStores] found: 0--> 114 StoreB === 80 91 113 95 [[ 18 ]] @byte[int:>=0] (java/lang/Cloneable,java/io/Serializable):exact+any *, idx=5; unsafe Memory: @byte[int:>=0] (java/lang/Cloneable,java/io/Serializable):NotNull:exact+any *, idx=5; !jvms: DecimalDigits::putCharLatin1 @ bci:12 (line 430) DecimalDigits::putPairLatin1 @ bci:24 (line 413) 1--> 91 StoreB === 80 7 90 51 [[ 114 ]] @byte[int:>=0] (java/lang/Cloneable,java/io/Serializable):exact+any *, idx=5; unsafe Memory: @byte[int:>=0] (java/lang/Cloneable,java/io/Serializable):NotNull:exact+any *, idx=5; !jvms: DecimalDigits::putCharLatin1 @ bci:12 (line 430) DecimalDigits::putPairLatin1 @ bci:13 (line 412) [TraceMergeStores] truncated: 0--> 114 StoreB === 80 91 113 95 [[ 18 ]] @byte[int:>=0] (java/lang/Cloneable,java/io/Serializable):exact+any *, idx=5; unsafe Memory: @byte[int:>=0] (java/lang/Cloneable,java/io/Serializable):NotNull:exact+any *, idx=5; !jvms: DecimalDigits::putCharLatin1 @ bci:12 (line 430) DecimalDigits::putPairLatin1 @ bci:24 (line 413) 1--> 91 StoreB === 80 7 90 51 [[ 114 ]] @byte[int:>=0] (java/lang/Cloneable,java/io/Serializable):exact+any *, idx=5; unsafe Memory: @byte[int:>=0] (java/lang/Cloneable,java/io/Serializable):NotNull:exact+any *, idx=5; !jvms: DecimalDigits::putCharLatin1 @ bci:12 (line 430) DecimalDigits::putPairLatin1 @ bci:13 (line 412) [TraceMergeStores]: Replace 91 StoreB === 80 7 90 51 [[ 114 ]] @byte[int:>=0] (java/lang/Cloneable,java/io/Serializable):exact+any *, idx=5; unsafe Memory: @byte[int:>=0] (java/lang/Cloneable,java/io/Serializable):NotNull:exact+any *, idx=5; !jvms: DecimalDigits::putCharLatin1 @ bci:12 (line 430) DecimalDigits::putPairLatin1 @ bci:13 (line 412) 114 StoreB === 80 91 113 95 [[ 18 ]] @byte[int:>=0] (java/lang/Cloneable,java/io/Serializable):exact+any *, idx=5; unsafe Memory: @byte[int:>=0] (java/lang/Cloneable,java/io/Serializable):NotNull:exact+any *, idx=5; !jvms: DecimalDigits::putCharLatin1 @ bci:12 (line 430) DecimalDigits::putPairLatin1 @ bci:24 (line 413) [TraceMergeStores]: with 51 LoadS === 33 7 49 [[ 91 85 95 70 118 ]] @short[int:>=0] (java/lang/Cloneable,java/io/Serializable):exact+any *, idx=4; #short !jvms: DecimalDigits::putPairLatin1 @ bci:4 (line 411) 118 StoreC === 80 7 90 51 [[ ]] @byte[int:>=0] (java/lang/Cloneable,java/io/Serializable):NotNull:exact+any *, idx=5; mismatched Memory: @byte[int:>=0] (java/lang/Cloneable,java/io/Serializable):NotNull:exact+any *, idx=5; [TraceMergeStores] MergePrimitiveStores::run: 118 StoreC === 80 7 90 51 [[ 18 ]] @byte[int:>=0] (java/lang/Cloneable,java/io/Serializable):NotNull:exact+any *, idx=5; mismatched Memory: @byte[int:>=0] (java/lang/Cloneable,java/io/Serializable):NotNull:exact+any *, idx=5; !orig=[114] !jvms: DecimalDigits::putCharLatin1 @ bci:12 (line 430) DecimalDigits::putPairLatin1 @ bci:24 (line 413)

liach

Looks good. Tier 1-3 tests pass.

wenshao · 2024-11-11T01:39:58Z

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.

wenshao · 2024-11-11T05:06:04Z

/integrate

openjdk · 2024-11-11T05:06:59Z

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

ca69a53: 8211033: Clean up the processing -classpath argument not to set LM_CLASS
7b0f273: 8343894: ProblemList javax/management/remote/mandatory/notif/EmptyDomainNotificationTest.java
74ae3c6: 8343650: Reuse StringLatin1::putCharsAt and StringUTF16::putCharsAt
423e8e0: 8339162: [REDO] JDK-8338440 Parallel: Improve fragmentation mitigation in Full GC
f400896: 8342442: Static ACVP sample tests
325a2c3: 8343848: Fix typo of property name in TestOAEPPadding after 8341927
2614c99: 8343793: Test java/foreign/TestMemorySession.java is timing out
8a69893: 8343819: Link Float.NaN and Double.NaN to equivalence discussion in Double
52c0b09: 8277240: java/awt/Graphics2D/ScaledTransform/ScaledTransform.java dialog does not get disposed
0329855: 8343804: Show the default time zone with -XshowSettings option
... and 65 more: https://git.openjdk.org/jdk/compare/5890d9438bbde88b89070052926a2eafe13d7b42...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-11-11T05:07:06Z

@wenshao Pushed as commit e1d684c.

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

TobiHartmann · 2024-11-11T12:05:36Z

This caused a regression, see JDK-8343925. You might want to consider a quick backout.

AlanBateman · 2024-11-11T12:12:54Z

I see Shaojin has created an issue to exclude the test but I think better to back this out quickly.

liach · 2024-11-11T12:30:19Z

I think the fix is easy: Unsafe calls are using a bad offset which should be cast to long. Will submit a PR, but since I cannot reproduce the original issue locally or on internal CI, need others review first.

wenshao · 2024-11-11T12:42:18Z

I submitted a rollback PR #22012. I can't reproduce the problem on a MacBook M1 Max, but I agree that more testing is needed, so let's roll it back first.

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

/open

openjdk · 2024-11-12T01:17:10Z

@wenshao The command open can only be used in open pull requests.

move getChars to DecimalDigits & deduplicate

e7aca75

openjdk bot added security [email protected] core-libs [email protected] labels Oct 20, 2024

add comments

2b7baad

wenshao changed the title ~~Move getChars methods of StringLatin1 and StringUTF16 to DecimalDigits to reduce duplication~~ Move getChars to DecimalDigits to reduce duplication Oct 20, 2024

wenshao changed the title ~~Move getChars to DecimalDigits to reduce duplication~~ Move getChars to DecimalDigits Oct 20, 2024

wenshao added 8 commits October 20, 2024 08:58

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

wenshao changed the title ~~Move getChars to DecimalDigits~~ 8342650: Move getChars to DecimalDigits Oct 20, 2024

openjdk bot added the rfr Pull request is ready for review label Oct 20, 2024

j3graham reviewed Oct 20, 2024

View reviewed changes

openjdk bot removed the security [email protected] label Oct 22, 2024

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

73b3200

…up_202410

liach reviewed Nov 6, 2024

View reviewed changes

wenshao added 2 commits November 6, 2024 13:13

remove comments, from @liach

e97138d

add benchmark

cf947f6

liach approved these changes Nov 10, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 10, 2024

openjdk bot added the integrated Pull request has been integrated label Nov 11, 2024

openjdk bot closed this Nov 11, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 11, 2024

This was referenced Nov 11, 2024

Optimize int and long to string #22000

Closed

8315585: Optimization for decimal to string #22009

Closed

wenshao mentioned this pull request Nov 11, 2024

8343925: [BACKOUT] JDK-8342650 Move getChars to DecimalDigits #22012

Closed

3 tasks

wenshao mentioned this pull request Nov 11, 2024

Fix DecimalDigits unsafe offset overflow #22014

Closed

3 tasks

wenshao mentioned this pull request Nov 12, 2024

8343962: [REDO] Move getChars to DecimalDigits #22023

Closed

3 tasks

graalvmbot mentioned this pull request Nov 19, 2024

[GR-59836] Update labsjdk to 24+24-jvmci-b01 oracle/graal#10108

Merged

wenshao mentioned this pull request Jan 25, 2025

8315585: Optimization for decimal to string #23310

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8342650: Move getChars to DecimalDigits #21593

8342650: Move getChars to DecimalDigits #21593

wenshao commented Oct 20, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Oct 20, 2024

openjdk bot commented Oct 20, 2024 •

edited

Loading

openjdk bot commented Oct 20, 2024

mlbridge bot commented Oct 20, 2024 •

edited

Loading

j3graham Oct 20, 2024 •

edited

Loading

wenshao Oct 20, 2024

wenshao Nov 11, 2024

wenshao commented Oct 22, 2024

openjdk bot commented Oct 22, 2024

liach Nov 6, 2024

wenshao Nov 6, 2024

liach left a comment

wenshao commented Nov 11, 2024

wenshao commented Nov 11, 2024

openjdk bot commented Nov 11, 2024

openjdk bot commented Nov 11, 2024

TobiHartmann commented Nov 11, 2024

AlanBateman commented Nov 11, 2024

liach commented Nov 11, 2024

wenshao commented Nov 11, 2024

wenshao commented Nov 12, 2024

openjdk bot commented Nov 12, 2024

8342650: Move getChars to DecimalDigits #21593

8342650: Move getChars to DecimalDigits #21593

Conversation

wenshao commented Oct 20, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Oct 20, 2024

openjdk bot commented Oct 20, 2024 • edited Loading

openjdk bot commented Oct 20, 2024

mlbridge bot commented Oct 20, 2024 • edited Loading

Webrevs

j3graham Oct 20, 2024 • edited Loading

Choose a reason for hiding this comment

wenshao Oct 20, 2024

Choose a reason for hiding this comment

wenshao Nov 11, 2024

Choose a reason for hiding this comment

wenshao commented Oct 22, 2024

openjdk bot commented Oct 22, 2024

liach Nov 6, 2024

Choose a reason for hiding this comment

wenshao Nov 6, 2024

Choose a reason for hiding this comment

liach left a comment

Choose a reason for hiding this comment

wenshao commented Nov 11, 2024

wenshao commented Nov 11, 2024

openjdk bot commented Nov 11, 2024

openjdk bot commented Nov 11, 2024

TobiHartmann commented Nov 11, 2024

AlanBateman commented Nov 11, 2024

liach commented Nov 11, 2024

wenshao commented Nov 11, 2024

wenshao commented Nov 12, 2024

openjdk bot commented Nov 12, 2024

wenshao commented Oct 20, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Oct 20, 2024 •

edited

Loading

mlbridge bot commented Oct 20, 2024 •

edited

Loading

j3graham Oct 20, 2024 •

edited

Loading