8315585: Optimization for decimal to string #23310

wenshao · 2025-01-25T07:25:40Z

Continue to complete PR #16006 and PR #21593 to improve BigDecimal::toString and BigDecimal::toPlainString performance and reduce duplicate code

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)

Warnings

⚠️ Patch contains a binary file (src/java.desktop/share/classes/javax/swing/beaninfo/images/JAppletColor16.gif)
⚠️ Patch contains a binary file (src/java.desktop/share/classes/javax/swing/beaninfo/images/JAppletColor32.gif)
⚠️ Patch contains a binary file (src/java.desktop/share/classes/javax/swing/beaninfo/images/JAppletMono16.gif)
⚠️ Patch contains a binary file (src/java.desktop/share/classes/javax/swing/beaninfo/images/JAppletMono32.gif)
⚠️ Patch contains a binary file (src/java.desktop/share/classes/javax/swing/doc-files/JRootPane-1.gif)
⚠️ Patch contains a binary file (test/jdk/javax/rmi/ssl/keystore)
⚠️ Patch contains a binary file (test/jdk/javax/rmi/ssl/truststore)
⚠️ Patch contains a binary file (test/jdk/jdk/internal/loader/URLClassPath/testclasses.jar)
⚠️ Patch contains a binary file (test/jdk/sun/net/www/protocol/https/HttpsClient/dnsstore)
⚠️ Patch contains a binary file (test/jdk/sun/net/www/protocol/https/HttpsClient/ipstore)

Issue

JDK-8315585: Optimization for decimal to string (Enhancement - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23310

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-01-25T07:26:28Z

👋 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 · 2025-01-25T07:26:37Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2025-01-25T07:27:07Z

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

core-libs

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

j3graham · 2025-01-25T07:43:29Z

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

-     * @param buf   target buffer, UTF16-coded.
-     * @return index of the most significant digit or minus sign, if present
-     */
-    public static int getChars(long i, int index, char[] buf) {


Before dropping this method, there is another candidate to use it here :

jdk/src/java.base/share/classes/jdk/internal/math/FloatingDecimal.java

Line 367 in 1d2eb2f

private void developLongDigits( int decExponent, long lvalue, int insignificantDigits ){

I took a look at the FloatingDecimal related code. The relevant code in FloatingDecimal can be refactored and simplified so that the removed code does not need to be used.

I submitted a PR to simplify FloatingDecimal #23311

liach · 2025-01-26T18:43:35Z

A great cleanup that consolidates scale2, unscaledAbsString, and unscaledString.

j3graham · 2025-01-26T19:31:08Z

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

        } else if (insertionPoint > 0) { /* Point goes inside intVal */
-            buf = new StringBuilder(intString);
-            buf.insert(insertionPoint, '.');
+            buf = new StringBuilder();


Could calculate the precise size for the StringBuilder

The performance will degrade if you precompute the length of the StringBuilder.

That’s interesting - should the precomputed lengths be removed in the other ones as well?

j3graham · 2025-01-26T19:32:29Z

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

-                buf.insert(0, '-');
+                buf.append('-');
+            buf.append(intString)
+                    .insert(insertionPoint + (signum < 0 ? 1 : 0), '.');


Instead of the insert, could do an append of the prefix, then dot, then the suffix.

buf.append(intString, 0, insertionPoint) .append('.') .append(intString, insertionPoint, intString.length());

This is another way to write it, but the performance will be reduced.

j3graham · 2025-01-26T19:52:08Z

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

+    }
+
+    private String layoutCharsE(boolean sci, String coeff, int coeffLen, long adjusted) {
+        StringBuilder buf = new StringBuilder(32);


The comment above about constructing a buffer probably belongs here. Also could calculate a better approximation of the size - possibly coeffLen+14 if that comment is accurate.

This reverts commit ddc2a86.

# Conflicts: # src/java.base/share/classes/jdk/internal/util/DecimalDigits.java

j3graham · 2025-02-01T21:04:29Z

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

        }
        return buf.toString();
    }

+    private static String scale2(int intCompact) {
+        int highInt = intCompact / 100;


Something to experiment with here:

int highInt = intCompact / 100; int lowInt = intCompact - highInt * 100; short packed=DecimalDigits.pair(lowInt); return new StringBuilder() .append(highInt) .append('.') .append((char) (packed & 0xFF)) .append((char) (packed >> 8)) .toString();

C2 seems to be able to optimize out the SB here, so it might do as well as newStringNoRepl.

Good suggestion, but DecimalDigits no longer provides a pair method

Right - you’d need to add it back to try this

I tested it on a MacBook M1 Max machine and the results were different from what you said. Using StringBuilder + digitPair would degrade performance.

git remote add wenshao git@github.com:wenshao/jdk.git git fetch wenshao # use JLA git checkout f9af0b0203145b95d82a2067a10bde61b0915dd7 make test TEST="micro:java.math.BigDecimals.smallScale2" # use digitPair git checkout 4124121933e08ab185f3f879856197ed15caafd7 make test TEST="micro:java.math.BigDecimals.smallScale2"

Benchmark Mode Cnt Score Error Units (f9af0b02031) BigDecimals.smallScale2EngineeringToString avgt 15 9.658 ? 0.228 ns/op BigDecimals.smallScale2LayoutCharsToString avgt 15 9.597 ? 0.047 ns/op BigDecimals.smallScale2PlainToString avgt 15 9.759 ? 0.054 ns/op Benchmark Mode Cnt Score Error Units (4124121933e) BigDecimals.smallScale2EngineeringToString avgt 15 18.763 ? 0.332 ns/op BigDecimals.smallScale2LayoutCharsToString avgt 15 18.738 ? 0.214 ns/op BigDecimals.smallScale2PlainToString avgt 15 18.992 ? 0.226 ns/op

jddarcy · 2025-02-04T20:36:08Z

Can we please have a pause on the sequence of "make XYZ toString faster" PRs until there is some wider discussion of goals, etc.? Thanks.

mlbridge · 2025-02-04T21:39:08Z

Mailing list message from Archie Cobbs on core-libs-dev:

On Tue, Feb 4, 2025 at 2:40?PM Joe Darcy <darcy at openjdk.org> wrote:

Can we please have a pause on the sequence of "make XYZ toString faster"
PRs until there is some wider discussion of goals, etc.? Thanks.

I agree with this sentiment... It was surprising to see how easily a VM
crash can sneak in.

There is always a trade-off between A and B, where:

A = Code clarity, robustness vs. future changes, friendliness to new
developers, minimizing obscure bugs (and security holes), etc...
B = Performance

Where should the line be drawn? Personally (as a Java user) I'd accept 1%
slower vs. 1% less likely to crash any day...

Performance is important but there should be some general guidelines and
maybe some specific policies. E.g. should there be a higher number of
reviews required whenever Unsafe is used purely for performance reasons?

It's also worth pondering what's implied by the Java team evangelizing to
the rest of the world to stop using Unsafe, while at the same time adding
it more and more ourselves (when not strictly required). In theory we
should instead be eating our own dog food (or better yet, improving its
quality).

Also: when does it become more appropriate to address a performance issue
in Hotspot instead of in Java source? If some optimization eliminates an
array range check that is clearly not needed, it might be feasible (and
much more widely beneficial) to teach Hotstpot how to figure that out
itself, etc.

Just some random thoughts...

-Archie

--
Archie L. Cobbs
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/core-libs-dev/attachments/20250204/116c8508/attachment.htm>

wenshao · 2025-02-04T23:22:38Z

I think you are talking about the problem of PR #23420, which is caused by the use of thread-unsafe StringBuilder in multi-threaded scenarios. This problem is very obscure and I didn't consider it before. I have started to solve this problem and have submitted PR #23427. After it is completed, I will continue to submit PR to redo PR #19626 in a thread-safe way.

The above problem does not affect toString, because it only occurs when StringBuilder is used in a multi-threaded scenario.

AlanBateman · 2025-02-05T12:01:03Z

/reviewers 2 reviewer

openjdk · 2025-02-05T12:02:09Z

@AlanBateman
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

mlbridge · 2025-02-07T15:45:04Z

Mailing list message from Archie Cobbs on core-libs-dev:

On Tue, Feb 4, 2025 at 5:26?PM Shaojin Wen <swen at openjdk.org> wrote:

I think you are talking about the problem of PR #23420, which is caused by
the use of thread-unsafe StringBuilder in multi-threaded scenarios. This
problem is very obscure and I didn't consider it before. I have started to
solve this problem and have submitted PR #23427. After it is completed, I
will continue to submit PR to redo PR #19626 in a thread-safe way.

Yes - apologies if it sounded like I was trying to single you out. The
optimizations you've been doing are looking great. It's just that this
example is a good data point in the larger discussion about what the
general policy should be, etc.

The above problem does not affect toString, because it only occurs when
StringBuilder is used in a multi-threaded scenario.

Good point, but frankly, an irrelevant one. The key issue here is that if
plain, ordinary, non-native-invoking Java bytecode can corrupt memory
and/or crash the JVM, then that's a Big Problem??. It doesn't matter how
contrived the code that makes it happen is.

-Archie

--
Archie L. Cobbs
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/core-libs-dev/attachments/20250207/67028683/attachment.htm>

mlbridge · 2025-02-08T00:15:04Z

Mailing list message from Paul Sandoz on core-libs-dev:

I would like to amplify this point ? undermining Java?s integrity is a big deal. Every time we use unsafe mechanisms within the JDK we risk doing that. The more complex such code is the harder it is reason about whether overall it is safe [*]. We need to balance reasoning about code, quality, and maintenance of against narrowly measured performance benefits that increase the risk of some integrity violation.

Paul.

[*] And even if it is not so complex, others may not be aware of the subtleties when refactoring. Unsafe allocation that does not zero memory is particular worrisome in this regard.

On Feb 7, 2025, at 7:42?AM, Archie Cobbs <archie.cobbs at gmail.com> wrote:
Good point, but frankly, an irrelevant one. The key issue here is that if plain, ordinary, non-native-invoking Java bytecode can corrupt memory and/or crash the JVM, then that's a Big Problem??. It doesn't matter how contrived the code that makes it happen is.

-Archie

--
Archie L. Cobbs

bridgekeeper · 2025-03-05T16:34:17Z

@wenshao This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

wenshao · 2025-03-09T15:41:14Z

Keep it alive.

bridgekeeper · 2025-04-06T17:58:47Z

@wenshao This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper · 2025-05-04T19:45:32Z

@wenshao This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

# Conflicts: # src/java.base/share/classes/java/math/BigDecimal.java # src/java.base/share/classes/jdk/internal/util/DecimalDigits.java # test/micro/org/openjdk/bench/java/math/BigDecimals.java

wenshao · 2025-05-25T09:46:42Z

/open

openjdk · 2025-05-25T09:47:29Z

@wenshao This pull request is now open

bridgekeeper · 2025-07-20T14:51:03Z

@wenshao This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

wenshao · 2025-07-20T23:27:05Z

/touch

openjdk · 2025-07-20T23:28:07Z

@wenshao The pull request is being re-evaluated and the inactivity timeout has been reset.

openjdk · 2025-08-20T22:14:32Z

@wenshao this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout dec_to_str_202501
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

wenshao added 2 commits January 25, 2025 11:26

add benchmark

4040d76

optimize toString/toPlainString

635882f

wenshao marked this pull request as draft January 25, 2025 07:26

openjdk bot added the core-libs core-libs-dev@openjdk.org label Jan 25, 2025

j3graham reviewed Jan 25, 2025

View reviewed changes

wenshao added 4 commits January 25, 2025 22:16

optimize scale2

7ed19c8

static scale2

7b82f78

optimize scale2 int

edffa0f

bug fix

72ed1b7

j3graham reviewed Jan 26, 2025

View reviewed changes

wenshao added 12 commits January 27, 2025 09:35

bug fix

cb6786b

code style

57cc7b2

use getBytes & JLA instead of StringBuilder

0d2b3aa

code style

4a857cf

code style

057d006

revert use StringBuilder

a41a34b

from @j3graham

a734f61

intCompact getValueString & int scale2

3ed3354

remove unnecessary abs

d99a963

simplify scale2

ddc2a86

Revert "simplify scale2"

d78b07c

This reverts commit ddc2a86.

refactor

4ee8837

wenshao changed the title ~~Optimization for decimal to string~~ 8315585 : Optimization for decimal to string Jan 27, 2025

wenshao changed the title ~~8315585 : Optimization for decimal to string~~ 8315585: Optimization for decimal to string Jan 27, 2025

wenshao added 4 commits January 30, 2025 08:13

Merge remote-tracking branch 'upstream/master' into dec_to_str_202501

cf43f34

# Conflicts: # src/java.base/share/classes/jdk/internal/util/DecimalDigits.java

copyright

0e7b30b

remove getChars(long, int, char[])

cf67d84

Merge remote-tracking branch 'upstream/master' into dec_to_str_202501

f9af0b0

# Conflicts: # src/java.base/share/classes/jdk/internal/util/DecimalDigits.java

j3graham reviewed Feb 1, 2025

View reviewed changes

Merge remote-tracking branch 'upstream/master' into dec_to_str_202501

a5a07ce

bridgekeeper bot closed this May 4, 2025

Merge remote-tracking branch 'upstream/master' into dec_to_str_202501

b4c142d

# Conflicts: # src/java.base/share/classes/java/math/BigDecimal.java # src/java.base/share/classes/jdk/internal/util/DecimalDigits.java # test/micro/org/openjdk/bench/java/math/BigDecimals.java

openjdk bot reopened this May 25, 2025

wenshao marked this pull request as draft May 25, 2025 09:47

openjdk bot removed the rfr Pull request is ready for review label May 25, 2025

wenshao mentioned this pull request Jun 14, 2025

8365832: Optimize FloatingDecimal and DigitList with byte[] and cleanup #23311

Open

3 tasks

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Aug 20, 2025

8315585: Optimization for decimal to string #23310

Are you sure you want to change the base?

8315585: Optimization for decimal to string #23310

Conversation

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

Uh oh!

Progress

Warnings

Issue

Reviewing

Uh oh!

bridgekeeper bot commented Jan 25, 2025

Uh oh!

openjdk bot commented Jan 25, 2025

Uh oh!

openjdk bot commented Jan 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liach commented Jan 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jddarcy commented Feb 4, 2025

Uh oh!

mlbridge bot commented Feb 4, 2025

Uh oh!

wenshao commented Feb 4, 2025

Uh oh!

AlanBateman commented Feb 5, 2025

Uh oh!

openjdk bot commented Feb 5, 2025

Uh oh!

mlbridge bot commented Feb 7, 2025

Uh oh!

mlbridge bot commented Feb 8, 2025

Uh oh!

bridgekeeper bot commented Mar 5, 2025

Uh oh!

wenshao commented Mar 9, 2025

Uh oh!

bridgekeeper bot commented Apr 6, 2025

Uh oh!

bridgekeeper bot commented May 4, 2025

Uh oh!

wenshao commented May 25, 2025

Uh oh!

openjdk bot commented May 25, 2025

Uh oh!

bridgekeeper bot commented Jul 20, 2025

Uh oh!

wenshao commented Jul 20, 2025

Uh oh!

openjdk bot commented Jul 20, 2025

Uh oh!

openjdk bot commented Aug 20, 2025

Uh oh!

Uh oh!

wenshao commented Jan 25, 2025 •

edited by openjdk bot

Loading