8358880: Performance of parsing with DecimalFormat can be improved #25644

j3graham · 2025-06-04T18:18:39Z

This PR replaces construction of intermediate strings to be parsed with more direct manipulation of numbers. It also has a more streamlined mechanism of handling Long.MIN_VALUE when parsing longs by using Long.parseUnsignedLong

As a small side-effect it also eliminates the use of a cached StringBuilder in DigitList.

Testing:

GHA
Local run of tier 2 and jtreg:jdk/java/text
New benchmark: DecimalFormatParseBench

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-8358880: Performance of parsing with DecimalFormat can be improved (Enhancement - P4)

Reviewers

Justin Lu (@justin-curtis-lu - Committer)
Chen Liang (@liach - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25644

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-06-04T18:19:17Z

👋 Welcome back j3graham! 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-06-04T18:19:56Z

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

openjdk · 2025-06-04T18:20:25Z

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

core-libs
i18n

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.

j3graham · 2025-06-04T18:21:48Z

Rough performance results on AArch64 M4:

Original
Benchmark                                   Mode  Cnt      Score     Error   Units
-DecimalFormatParseBench.testParseDoubles  thrpt   15  15200.984 ± 409.547  ops/ms
-DecimalFormatParseBench.testParseLongs    thrpt   15  25777.899 ± 559.096  ops/ms

This PR
Benchmark                                   Mode  Cnt      Score     Error   Units
+DecimalFormatParseBench.testParseDoubles  thrpt   15  28041.325 ± 472.657  ops/ms
+DecimalFormatParseBench.testParseLongs    thrpt   15  34181.146 ± 655.719  ops/ms

src/java.base/share/classes/java/text/DigitList.java

liach · 2025-06-04T23:15:37Z

src/java.base/share/classes/java/text/DigitList.java

-        temp.append("0".repeat(Math.max(0, decimalAt - count)));
-        return Long.parseLong(temp.toString());
+        long pow10 = Math.powExact(10L, Math.max(0, decimalAt - count));
+        return Math.multiplyExact(v, pow10);


These two methods throw ArithmeticException. This needs to be rethrown as NumberFormatException.

This one is a little odd. The parse methods that call getLong are not supposed to throw NumberFormatException either. So wherever getLong is called, it must be preceded by a check to fitsIntoLong, which should avoid any exceptions here. That said, rethrowing as NFE would avoid new surprises. What do you think?

I will leave this question to I18N reviewers, who are ultimately in charge of DigitList.

The existing implementation does not throw NumberFormatException/ArithmeticException, but ParseException if parsing is failing. I would expect the same here.

Sorry, I'm not seeing where the original could throw ParseException.

Sorry if I was unclear. I mean the parse() in the NumberFormat do not throw NumberFormatException/ArithmeticException, but ParseException, so if this piece of code need to throw something, it should be ParseException

The parse() methods where this code gets used in DecimalFormat unfortunately don't throw ParseException. The current calls to getLong always are guarded with a call to fitsIntoLong, which should avoid any exceptions actually being thrown here. So there is no parse failure as such - instead it tries to parse it as a double or a BigDecimal. If getLong were to be called without the guard, the exception would have come from Long.parseLong, which throws a NumberFormatException.

I've added a commit to follow @liach's suggestion to at least handle the ArithmeticException so as to not introduce new exceptions into the mix.

OK, sounds reasonable.

ALUMINIS650 · 2025-06-09T13:01:48Z

Hi @ALUMINIS650, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user ALUMINIS650" for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

I agree to the OpenJDK Terms of Use for all comments I make in a project in the OpenJDK GitHub organization.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

mlbridge · 2025-06-09T17:16:00Z

Webrevs

justin-curtis-lu

Thanks for the improvements. I think we need to prioritize behavioral compatibility with this change, so we will want to run the JCK tests as well for the extra safety.

test/jdk/java/text/Format/DecimalFormat/CloneTest.java

test/micro/org/openjdk/bench/java/text/DecimalFormatParseBench.java

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

justin-curtis-lu

On our CI, I did a java_text JCK run as well as tiers 1-3 on all platforms which both came back good. I just have a final comment.

justin-curtis-lu · 2025-06-11T23:47:01Z

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

@@ -1824,6 +1837,17 @@ private static BinaryToASCIIConverter getBinaryToASCIIConverter(float f) {
        return buf;
    }

+    static ASCIIToBinaryConverter readDoubleSignlessDigits(int decExp, char[] digits, int length) {
+        if (decExp < MIN_DECIMAL_EXPONENT) {


Is this check needed? I think ASCIIToBinaryConverter will return the proper zero value when doubleValue() is invoked.

if (decExponent < MIN_DECIMAL_EXPONENT - 1) { return (isNegative) ? -0.0 : 0.0;

And if this explicit check is a shortcut, I don't think we would need one for an edge case.

Unfortunately some check is required (a test fails), but I now realize what I had was wrong. The issue is that on line 1084 (https://github.com/openjdk/jdk/pull/25644/files#diff-79e6fd549b5ec5e7f49658581beddcb07fcbb0c09ae8e1117c385b66514da6d2R1084)) exp can overflow and become positive again. I've updated the check to avoid the overflow.

Ah got it, I see your point. We would have goten underflow in ASCIIToBinaryConverter.doubleValue() for some extreme cases without a check.

Is there a specific example you have that requires the switch to the newer check? Adding a comment along those lines might be helpful. Actually, I thought DigitList caps decimalAt to Integer.MIN/MAX, so then the first check you had would have been fine. (Maybe I am missing something?)

I don't have a specific example, so I've reverted to my original check. I'm a bit unsettled by the check for an extreme value later in doubleValue() comparing against MIN_DECIMAL_EXPONENT - 1

IMO, the original check you had is easier to understand what is happening without further context, so I prefer your switch back.

I think we are fine from (negative) "extreme values" in doubleValue() because of the check you have implemented in the first place. i.e. we avoid any potential underflow from int exp = decExponent - kDigits;. I think we do need a comment to accompany the check. (Why do we check? why not check the max exponent value?)

Also, should the check be against MIN_DECIMAL_EXPONENT - 1 for consistency with doubleValue()? (Functionally, I don't think it matters.)

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

This reverts commit 6a07287.

jddarcy · 2025-06-17T03:53:03Z

/reviewers 2 reviewer

openjdk · 2025-06-17T03:53:43Z

@jddarcy
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).

bridgekeeper · 2025-07-15T07:50:33Z

@j3graham 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 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!

liach

Looks reasonable from a code cleanup point of view.

justin-curtis-lu

The current form looks good to me. The long parsing did not change much on my machine performance wise, but I think it is a good simplification to include.

improve getDouble, getLong

8bf1f61

openjdk bot added core-libs core-libs-dev@openjdk.org i18n i18n-dev@openjdk.org labels Jun 4, 2025

j3graham added 2 commits June 4, 2025 15:07

copyright dates

9644568

update comment

bcac896

liach reviewed Jun 4, 2025

View reviewed changes

j3graham added 2 commits June 4, 2025 19:54

simplify comparison

e13dea8

simplify getBigDecimal

a6978bd

j3graham changed the title ~~Improve performance of parsing with DecimalFormat~~ 8358880: Performance of parsing with DecimalFormat can be improved Jun 9, 2025

Merge branch 'openjdk:master' into digitlist-getdouble-get-long

a85ddd8

j3graham marked this pull request as ready for review June 9, 2025 17:10

openjdk bot added the rfr Pull request is ready for review label Jun 9, 2025

catch ArithmeticException

da9e4ae

justin-curtis-lu reviewed Jun 9, 2025

View reviewed changes

Address review comments

6953dcf

justin-curtis-lu reviewed Jun 11, 2025

View reviewed changes

j3graham added 3 commits June 12, 2025 11:21

fix overflow check

6a07287

Revert "fix overflow check"

c87a3de

This reverts commit 6a07287.

add comments

b7faa3b

liach approved these changes Jul 15, 2025

View reviewed changes

justin-curtis-lu approved these changes Jul 21, 2025

View reviewed changes

8358880: Performance of parsing with DecimalFormat can be improved #25644

Are you sure you want to change the base?

8358880: Performance of parsing with DecimalFormat can be improved #25644

Conversation

j3graham commented Jun 4, 2025 • 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 Jun 4, 2025

Uh oh!

openjdk bot commented Jun 4, 2025

Uh oh!

openjdk bot commented Jun 4, 2025

Uh oh!

j3graham commented Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

j3graham Jun 4, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

j3graham Jun 9, 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!

naotoj Jun 9, 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!

j3graham Jun 9, 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!

Choose a reason for hiding this comment

Uh oh!

ALUMINIS650 commented Jun 9, 2025 • edited by bridgekeeper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

justin-curtis-lu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

justin-curtis-lu left a comment

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!

Uh oh!

jddarcy commented Jun 17, 2025

Uh oh!

openjdk bot commented Jun 17, 2025

Uh oh!

bridgekeeper bot commented Jul 15, 2025

Uh oh!

liach left a comment

j3graham commented Jun 4, 2025 •

edited by openjdk bot

Loading

j3graham commented Jun 4, 2025 •

edited

Loading

j3graham Jun 4, 2025 •

edited

Loading

j3graham Jun 9, 2025 •

edited

Loading

naotoj Jun 9, 2025 •

edited

Loading

j3graham Jun 9, 2025 •

edited

Loading

ALUMINIS650 commented Jun 9, 2025 •

edited by bridgekeeper bot

Loading

mlbridge bot commented Jun 9, 2025 •

edited

Loading