8343829: Unify decimal and hexadecimal parsing in FloatingDecimal #22737

rgiulietti · 2024-12-13T15:04:12Z

See the JBS bug for some details.

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-8343829: Unify decimal and hexadecimal parsing in FloatingDecimal (Enhancement - P4)

Reviewers

Joe Darcy (@jddarcy - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22737

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-12-13T15:05:10Z

👋 Welcome back rgiulietti! 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-12-13T15:05:48Z

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

8343829: Unify decimal and hexadecimal parsing in FloatingDecimal

Reviewed-by: darcy

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

4fc10a1: 8356087: Problematic KeyInfo check using key algorithm in P11SecretKeyFactory class
adebfa7: 8356700: RISC-V: Declare incompressible scope in fill_words / zero_memory assembler routines
303f410: 8355708: Two Float16 IR tests fail after JDK-8345125
... and 26 more: https://git.openjdk.org/jdk/compare/0f2a6c266b2e3aab59a5cd183e10dfc4820ca92d...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-12-13T15:06:18Z

@rgiulietti 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.

mlbridge · 2024-12-13T15:09:09Z

Webrevs

j3graham · 2024-12-15T21:57:12Z

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

+//    private static final int BINARY_256_IX = 4;
+
+    /* The precision of the format. */
+    private static final int[] P = {


What about organizing these constants as as a record - something like this:

record FloatFmt(int ix, int p, int epMin, int epMax, int w, int eMin, int eMax, int qMin, int hexCount) { public static final FloatFmt BINARY_16 = build(0, 11, -8, 6); public static final FloatFmt BINARY_32 = build(1, 24, -46, 40); public static final FloatFmt BINARY_64 = build(2, 53, -324, 310); public static final FloatFmt BINARY_128 = build(3, 113, -4_966, 4934); public static final FloatFmt BINARY_256 = build(4, 237, -78_985, 78_915); private static FloatFmt build(int ix, int p, int epMin, int epMax) { int w = (1 << 4 + ix) - p; int eMin = (-1 << w - 1) + 2; int eMax = (1 << w - 1) - 1; int qMin = eMin - p + 1; int hexCount = p / 4 + 2; return new FloatFmt(ix, p, epMin, epMax, w, eMin, eMax, qMin, hexCount); } }

Methods could then be parameterized with the static instances rather than integers. I believe these values would inline well as constants.

Thanks for the suggestion @j3graham.
I'll consider it for the next commits.

bridgekeeper · 2025-01-13T13:32:08Z

@rgiulietti 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-02-10T15:54:05Z

@rgiulietti 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.

rgiulietti · 2025-02-10T15:55:42Z

/open Still in review

openjdk · 2025-02-10T15:56:49Z

@rgiulietti This pull request is now open

bridgekeeper · 2025-03-10T17:59:37Z

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

rgiulietti · 2025-03-10T18:08:52Z

Ping

bridgekeeper · 2025-04-07T23:43:47Z

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

rgiulietti · 2025-04-29T17:37:10Z

Comment added...

wenshao · 2025-05-02T09:17:11Z

static final int BIG_DECIMAL_EXPONENT = 324;

The BIG_DECIMAL_EXPONENT in line 50 is no longer used and can be removed.

wenshao · 2025-05-02T12:08:32Z

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

+//    private static final int BINARY_256_IX = 4;
+
+    /* The precision of the format. */
+    private static final int[] P = {


private static final int[] P private static final int[] EP_MIN private static final int[] EP_MAX private static final int[] W private static final int[] E_MAX private static final int[] E_MIN private static final int[] Q_MIN private static final int[] HEX_COUNT

These should all be added with @stable

wenshao · 2025-05-02T12:24:23Z

We can do some cleanup before this PR, such as #24999

rgiulietti · 2025-05-02T12:38:06Z

This is just the first step of a planned overhaul of conversions from decimal strings to floating-point values. There are more steps to come.
So I'm not sure that #24999 turns out to be useful at this stage.

wenshao · 2025-05-02T13:40:14Z

Q_MIN -> E_MIN -> E_MAX -> W -> P Five layers of Stable, I don't know if C2 can work correctly, maybe need to confirm

rgiulietti · 2025-05-02T13:46:23Z

@wenshao Do you mean that C2 might generate erroneous code (bad situation!), or that it might not take advantage of @Stable in optimized code?

wenshao · 2025-05-02T14:05:16Z

@wenshao Do you mean that C2 might generate erroneous code (bad situation!), or that it might not take advantage of @Stable in optimized code?

I am worried that too many layers will cause C2 to not be optimized, but I am not sure, and I also hope to learn how to know that @Stable works

rgiulietti · 2025-05-02T14:10:38Z

I'm not worried if C2 cannot optimize too many levels of @Stable here. The values are used in an unusually long method, so I don't think there are measurable benefits anyway.
In contrast, I would be much concerned if there are real risks of erroneous code being generated.

jaikiran · 2025-05-02T14:21:54Z

If I understand correctly, we are talking about several (primitive) array type fields having a @Stable annotation, in a single class, right? So we aren't really talking about levels?
If so, given what @Stable is documented to do, I don't think the number of fields annotated as @Stable plays any role in the C2 compiler generating the resultant code.

rgiulietti · 2025-05-02T15:28:39Z

@jaikiran "Levels" in the sense that Q_MIN depends on E_MIN, which depends on E_MAX, which depends on W, which depends on P.
If @Stable helps, which I think it does, great. If it doesn't, it shouldn't matter in practice.

jddarcy · 2025-05-07T22:39:35Z

Generally good cleanup @rgiulietti.

I'd like to see some kind of representation of the grammar of strings being recognized included in this file, for example a cut-and-paste of the grammar from Double.valueOf(String) (with leading and trailing spaces):

FloatValue:
    Sign_opt NaN 
    Sign_opt Infinity 
    Sign_opt FloatingPointLiteral 
    Sign_opt HexFloatingPointLiteral 
    SignedInteger 

HexFloatingPointLiteral:
    HexSignificand BinaryExponent FloatTypeSuffixopt 

HexSignificand:
    HexNumeral 
    HexNumeral . 
    0x HexDigitsopt . HexDigits 
    0X HexDigitsopt . HexDigits 

BinaryExponent:
    BinaryExponentIndicator SignedInteger 

BinaryExponentIndicator:
    p 
    P

liach

Took a look over the string parsing part. Not professional enough about floating numbers.

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

jddarcy · 2025-05-13T03:58:44Z

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

+        }
+
+        /*
+         * In some places the idiom


Might be easier to read the code if this logic were refactored into a method.

Right.
I'll take this note into account in the next phase of the cleanup.

rgiulietti · 2025-05-14T07:58:50Z

/integrate

openjdk · 2025-05-14T07:59:24Z

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

3b27198: 8325647: [IR framework] Only prints stdout if exitCode is 134
a3e094e: 8354968: Replace unicode sequences in comment text with UTF-8 characters
dd25159: 8356644: Update encoding declaration to UTF-8
... and 64 more: https://git.openjdk.org/jdk/compare/0f2a6c266b2e3aab59a5cd183e10dfc4820ca92d...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-05-14T07:59:32Z

@rgiulietti Pushed as commit d1032d7.

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

rgiulietti added 2 commits December 12, 2024 16:27

8343829: Unify decimal and hexadecimal parsing in FloatingDecimal

1537e4a

Refactoring some code and comments.

e993925

openjdk bot added the rfr Pull request is ready for review label Dec 13, 2024

openjdk bot added the core-libs core-libs-dev@openjdk.org label Dec 13, 2024

rgiulietti added 3 commits December 13, 2024 16:21

Removed repeated comment.

85709dd

Minors in comments.

d2777da

Merge branch 'master' into 8343829

d055d0e

j3graham reviewed Dec 15, 2024

View reviewed changes

rgiulietti added 2 commits December 16, 2024 10:47

Redacted comments.

cb7bb25

Merge branch 'master' into 8343829

092755b

bridgekeeper bot closed this Feb 10, 2025

openjdk bot reopened this Feb 10, 2025

rgiulietti added 2 commits May 2, 2025 13:54

Merge branch 'master' into 8343829

dfd112c

Remove unused BIG_DECIMAL_EXPONENT

49ed9aa

wenshao reviewed May 2, 2025

View reviewed changes

Make some static arrays @stable.

41d070e

rgiulietti added 4 commits May 9, 2025 19:52

Merge branch 'master' into 8343829

d86f0cd

Added javadoc to refer to the grammar in j.l.Double.

9d14562

Merge branch 'master' into 8343829

217f07b

Removed useless comment.

88fe2c0

liach reviewed May 9, 2025

View reviewed changes

Addressed reviewer comments.

dbb5a27

jddarcy reviewed May 13, 2025

View reviewed changes

jddarcy approved these changes May 13, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 13, 2025

openjdk bot added the integrated Pull request has been integrated label May 14, 2025

openjdk bot closed this May 14, 2025

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

rgiulietti deleted the 8343829 branch May 14, 2025 08:01

8343829: Unify decimal and hexadecimal parsing in FloatingDecimal #22737

8343829: Unify decimal and hexadecimal parsing in FloatingDecimal #22737

Uh oh!

Conversation

rgiulietti commented Dec 13, 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 Dec 13, 2024

Uh oh!

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

Uh oh!

Uh oh!

openjdk bot commented Dec 13, 2024

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

j3graham Dec 15, 2024

Choose a reason for hiding this comment

Uh oh!

rgiulietti Dec 16, 2024

Choose a reason for hiding this comment

Uh oh!

bridgekeeper bot commented Jan 13, 2025

Uh oh!

bridgekeeper bot commented Feb 10, 2025

Uh oh!

rgiulietti commented Feb 10, 2025

Uh oh!

openjdk bot commented Feb 10, 2025

Uh oh!

bridgekeeper bot commented Mar 10, 2025

Uh oh!

rgiulietti commented Mar 10, 2025

Uh oh!

bridgekeeper bot commented Apr 7, 2025

Uh oh!

rgiulietti commented Apr 29, 2025

Uh oh!

wenshao commented May 2, 2025

Uh oh!

wenshao May 2, 2025

Choose a reason for hiding this comment

Uh oh!

wenshao commented May 2, 2025

Uh oh!

rgiulietti commented May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wenshao commented May 2, 2025

Uh oh!

rgiulietti commented May 2, 2025

Uh oh!

wenshao commented May 2, 2025

Uh oh!

rgiulietti commented May 2, 2025

Uh oh!

jaikiran commented May 2, 2025

Uh oh!

rgiulietti commented May 2, 2025

Uh oh!

jddarcy commented May 7, 2025

Uh oh!

liach left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jddarcy May 13, 2025

Choose a reason for hiding this comment

Uh oh!

rgiulietti May 13, 2025

Choose a reason for hiding this comment

Uh oh!

rgiulietti commented Dec 13, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Dec 13, 2024 •

edited

Loading

mlbridge bot commented Dec 13, 2024 •

edited

Loading

rgiulietti commented May 2, 2025 •

edited

Loading