8205592: BigDecimal.doubleValue() is depressingly slow #9410

rgiulietti · 2022-07-07T15:20:32Z

A reimplementation of BigDecimal.[double|float]Value() to enhance performance, avoiding an intermediate string and its subsequent parsing on the slow path.

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-8205592: BigDecimal.doubleValue() is depressingly slow

Reviewers

Joe Darcy (@jddarcy - Reviewer) ⚠️ Review applies to 75fde901

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9410

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2022-07-07T15:21:40Z

👋 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 · 2022-07-07T15:24:41Z

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

rgiulietti · 2022-07-07T15:27:35Z

These are the improvements over the current implementation:

Much more cases are processed by the fast path.
Most values that will either produce 0 or infinity are detected early in a fast way to avoid expensive computations.
If neither of the above applies, the conversion to String and subsequent parsing, as currently done, is replaced by BigInteger arithmetic. There's at most one division between BigIntegers. Of course, no need for toString() nor parsing.
Extensive comments explain all the details.

JMH benchmarks show that, on the fast path, the new implementation is on par or way better (>200x for the cases not currently covered).
Cases where 0 or infinity are detected early contribute with speedup factors of >200x.
BigInteger arithmetic contributes with speed factors of 2x-8x on "typical" BigDecimals of precision 18 to 24 and scale 2 to 6.

mlbridge · 2022-07-07T15:30:28Z

Webrevs

01: Full - Incremental (f679fb2b)
00: Full (75fde901)

TheShermanTanker · 2022-07-21T02:10:27Z

Impressive performance improvements, I also like the PR title :P

rgiulietti · 2022-07-22T14:57:16Z

@TheShermanTanker
The title is the one from the JBS issue.
As for the performance improvements, note that microbenchmarks are what they are. The overall effect of these improvements might be barely noticeable in some programs using BigDecimal and might be positively impact others.

bridgekeeper · 2022-08-19T16:52:21Z

@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 · 2022-08-19T16:54:07Z

keep open

bridgekeeper · 2022-09-17T00:49:59Z

@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 · 2022-09-19T10:15:04Z

keep open, waiting for review

bridgekeeper · 2022-10-17T12:14:53Z

@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 · 2022-10-17T13:31:34Z

waiting for review

bridgekeeper · 2022-11-14T17:29:09Z

@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 · 2022-12-12T11:17:20Z

waiting for review

bridgekeeper · 2023-01-09T22:30:40Z

@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 · 2023-01-31T13:39:46Z

This PR is waiting for a review

plokhotnyuk · 2023-02-13T17:53:46Z

@rgiulietti Thanks for keeping making JDK faster!

I have a couple of review comments:

1) For the line https://github.com/openjdk/jdk/pull/9410/files#diff-94d400b99466045dd76001c37eada6b24d086d8d115b49c439752bbceb233772L3746

It seems that removing this check for 22-bit number introduces an extra rounding error.

Please consider just increasing the constant up to 1L << 32 and using DOUBLE_10_POW instead of FLOAT_10_POW.

Bellow are REPL expressions that illustrate my thoughts (sorry for using Scala):

$ scala
Welcome to Scala 3.2.0 (17.0.5, Java OpenJDK 64-Bit Server VM).
Type in expressions for evaluation. Or try :help.
                                                                                                                                                                                                                   
scala> new java.math.BigDecimal("167772170E-1").floatValue
val res0: Float = 1.6777216E7
                                                                                                                                                                                                                   
scala> 167772170E-1f
val res1: Float = 1.6777216E7
                                                                                                                                                                                                                   
scala> 167772170 / 10.0f
val res2: Float = 1.6777218E7

scala> (167772170 / 10.0).toFloat
val res3: Float = 1.6777216E7

2) For the line: https://github.com/openjdk/jdk/pull/9410/files#diff-94d400b99466045dd76001c37eada6b24d086d8d115b49c439752bbceb233772L3791

It seems that removing this check for 52-bit number introduces an extra rounding error:

$ scala
Welcome to Scala 3.2.0 (17.0.6, Java OpenJDK 64-Bit Server VM).
Type in expressions for evaluation. Or try :help.

scala> new java.math.BigDecimal("90071992547409930E-1").doubleValue
val res0: Double = 9.007199254740992E15
                                                                                                                                                                                                                   
scala> 90071992547409930E-1
val res1: Double = 9.007199254740992E15
                                                                                                                                                                                                                   
scala> 90071992547409930L / 10.0
val res2: Double = 9.007199254740994E15

3) For the line: https://github.com/openjdk/jdk/pull/9410/files#diff-94d400b99466045dd76001c37eada6b24d086d8d115b49c439752bbceb233772L3800

Here before falling back to the costly fallback with big number operations we can get a number of digits in intCompact and use a trick with two multiplications

rgiulietti · 2023-02-13T20:05:58Z

@plokhotnyuk

The expression new java.math.BigDecimal("90071992547409930E-1").doubleValue() would be processed by the proposed code as follows.

We have:
intCompact = 90071992547409930L
scale = 1

L.3838 sets v == 9.007199254740994E16
The test (long) v == intCompact on L. 3848 fails, so the fast path is not executed.
The slow path then ensures that the conversion is affected by at most one rounding error.

On the other hand, the expression
scala> 90071992547409930L / 10.0
is subject to two rounding errors, but it i executed neither by the current nor by the proposed code.

A similar analysis holds for the float example.

As for the last paragraph, I'll have to have a deeper look. Stay tuned ;-)

plokhotnyuk · 2023-02-14T08:02:08Z

Sorry, I overlooked those checks two times :)

How about adding a moderate path like this?

I think it worth do be reused for regular parsing of double and float values from String.

rgiulietti · 2023-02-14T14:07:22Z

@plokhotnyuk The main goal of this PR is to avoid generating a string and parse it, as it happens in the current implementation. The fact that it results in being faster is only a welcome byproduct.

The proposed patch for doubleValue() is only about 40 lines of code, not counting }-only lines and the extensive comments explaining the details for the benefit of both reviewers and maintainers. Shorter, documented code has a higher chance to be correct and understood. It also contributes to simpler and quicker reviewing.

Adding the "moderate path" to this patch would increase the code size considerably. Moreover, I would have to invest time to understand the dense, uncommented code and convince myself and the reviewers that it is correct. I would also have to setup benchmarks to measure the overall benefits of adding it to the proposed patch. And add specific tests to cover the path.

Before that, I would prefer for this patch to be first reviewed as it is (with possible corrections). I hope to have time to invest into your proposals once this PR is integrated into mainline. Thanks for your patience.

plokhotnyuk · 2023-02-14T14:36:42Z

@rgiulietti Thanks for the explanation!

I wish faster reviews of all your PRs!

I bet that investigation of the moderate path will pay itself when that path will be reused for improving of java.lang.Double.parseDouble and java.lang.Float.parseFloat methods.

You can roughly estimate the moderate path speed up comparing the throughput of borer and jsoniterScala in the following chart. Both of them use the same fast path and fallback to java.lang.Double.parseDouble, the difference is that jsoniterScala (and smithy4sJson that is based on it) adds using of the moderate path:

BTW, the jacksonScala uses a Java port of Daniel Lemire's fast_float project.

P.S. Here is the setup method of the corresponding benchmark.

bridgekeeper · 2023-03-14T18:38:34Z

@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 · 2023-04-10T21:13:10Z

Keep it open

jddarcy · 2023-04-26T01:41:09Z

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

 public class BigDecimal extends Number implements Comparable<BigDecimal> {
+    /*
+     * Let l = log_2(10).
+     * Then, L < l < L + ulp(L) / 2, that is, L = roundTiesToEven(l).


It doesn't matter in terms of the code, but shouldn't this be something like:

L - (ulp(L)) < l < L ulp(L)

In other words, without further checking, it isn't clear that L is the lower-bound of the two double value bracketing l.

(If the ulp function being discussed were the real-valued version than L +/- ulp(l) would also be a reasonable formulation.)

While it isn't obvious from the definition, high precision computations reveal that the inequalities L < l < L + ulp(L) / 2 do indeed hold.
But as noted, this is not really relevant for the rest of the analysis.

jddarcy · 2023-04-26T01:44:54Z

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

+        if (intCompact == 0) {
+            return 0.0f;
+        }
+        BigInteger d = unscaledValue().abs();


I'd prefer a name other than "d" be used for the BigInteger significand's magnitude.

I have no objections as long as it is a 1 letter name that does not conflict with others.
Have you a preferred one?

I have no objections as long as it is a 1 letter name that does not conflict with others. Have you a preferred one?

I usually use "bd" or "bi" for such BigDecima/BigInteger variables myself and use "d" for double; "v" or "w" have integer-ish feel to me so perhaps one of those?

jddarcy · 2023-04-26T01:47:07Z

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

    @Override
-    public float floatValue(){
-        if(intCompact != INFLATED) {
+    public float floatValue() {


If the total size of code were a concern, I wouldn't be adverse to floatValue() being implemented as

return (float)doubleValue();

Given the 2p+2 property between the float and double formats, BigDecimal -> double -> float and BigDecimal -> float round the same way.

No, that would not be correct. It would be subject to double rounding, against the spec.

For example, BigDecimal 1.000000059604644775390626 should round to float 1.0000001f.
When going to the closest double and then to the closest float, however, it first rounds to double 1.0000000596046448 and the to float 1.0f.

No, that would not be correct. It would be subject to double rounding, against the spec.

For example, BigDecimal 1.000000059604644775390626 should round to float 1.0000001f. When going to the closest double and then to the closest float, however, it first rounds to double 1.0000000596046448 and the to float 1.0f.

Ah right; thanks for the correction -- since the set of possible inputs in this case isn't constrained by operations on float values, the 2p+2 property that hold for {+, -, *, /, sqrt} doesn't hold here.

openjdk · 2023-04-26T01:59:19Z

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

8205592: BigDecimal.doubleValue() is depressingly slow

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

41ba05e: 8306850: Open source AWT Model related tests
fed262a: 8306949: Resolve miscellaneous multiple symbol definition issues when statically linking JDK/VM natives with standard launcher
96cdf93: 8306833: Change CardTable::_covered to static array
1be80a4: 8287087: C2: perform SLP reduction analysis on-demand
ba43649: 8306976: UTIL_REQUIRE_SPECIAL warning on grep
cbccc4c: 8304265: Implementation of Foreign Function and Memory API (Third Preview)
41d5853: 8306940: test/jdk/java/net/httpclient/XxxxInURI.java should call HttpClient::close
d94ce65: 8306858: Remove some remnants of CMS from SA agent
a83c02f: 8306654: Disable NMT location_printing_cheap_dead_xx tests again
de0c05d: 6995195: Static initialization deadlock in sun.java2d.loops.Blit and GraphicsPrimitiveMgr
... and 3877 more: https://git.openjdk.org/jdk/compare/cce77a700141a854bafaa5ccb33db026affcf322...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.

rgiulietti · 2023-04-27T17:00:49Z

/integrate

openjdk · 2023-04-27T17:02:10Z

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

41ba05e: 8306850: Open source AWT Model related tests
fed262a: 8306949: Resolve miscellaneous multiple symbol definition issues when statically linking JDK/VM natives with standard launcher
96cdf93: 8306833: Change CardTable::_covered to static array
1be80a4: 8287087: C2: perform SLP reduction analysis on-demand
ba43649: 8306976: UTIL_REQUIRE_SPECIAL warning on grep
cbccc4c: 8304265: Implementation of Foreign Function and Memory API (Third Preview)
41d5853: 8306940: test/jdk/java/net/httpclient/XxxxInURI.java should call HttpClient::close
d94ce65: 8306858: Remove some remnants of CMS from SA agent
a83c02f: 8306654: Disable NMT location_printing_cheap_dead_xx tests again
de0c05d: 6995195: Static initialization deadlock in sun.java2d.loops.Blit and GraphicsPrimitiveMgr
... and 3877 more: https://git.openjdk.org/jdk/compare/cce77a700141a854bafaa5ccb33db026affcf322...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-04-27T17:02:26Z

@rgiulietti Pushed as commit eb35861.

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

8205592: BigDecimal.doubleValue() is depressingly slow

75fde90

openjdk bot added the rfr Pull request is ready for review label Jul 7, 2022

openjdk bot added the core-libs [email protected] label Jul 7, 2022

wrandelshofer mentioned this pull request Dec 19, 2022

possible performance issue with very big doubles wrandelshofer/FastDoubleParser#32

Closed

pjfanning mentioned this pull request Dec 19, 2022

consider inlining BigDecimal toDouble and toFloat improvements FasterXML/jackson-core#870

Closed

jddarcy reviewed Apr 26, 2023

View reviewed changes

jddarcy approved these changes Apr 26, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 26, 2023

Renamed local variable b to w in full[Float|Double]Value().

f679fb2

openjdk bot added the integrated Pull request has been integrated label Apr 27, 2023

openjdk bot closed this Apr 27, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 27, 2023

rgiulietti deleted the JDK-8205592 branch April 27, 2023 17:17

wrandelshofer mentioned this pull request Apr 29, 2023

Implement faster slow path for double parser (JDK 21) wrandelshofer/FastDoubleParser#39

Closed

cl4es mentioned this pull request Sep 5, 2023

8315585: Optimization for decimal to string #15555

Closed

3 tasks

8205592: BigDecimal.doubleValue() is depressingly slow #9410

8205592: BigDecimal.doubleValue() is depressingly slow #9410

Uh oh!

Conversation

rgiulietti commented Jul 7, 2022 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Jul 7, 2022

Uh oh!

openjdk bot commented Jul 7, 2022

Uh oh!

rgiulietti commented Jul 7, 2022

Uh oh!

mlbridge bot commented Jul 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

TheShermanTanker commented Jul 21, 2022

Uh oh!

rgiulietti commented Jul 22, 2022

Uh oh!

bridgekeeper bot commented Aug 19, 2022

Uh oh!

rgiulietti commented Aug 19, 2022

Uh oh!

bridgekeeper bot commented Sep 17, 2022

Uh oh!

rgiulietti commented Sep 19, 2022

Uh oh!

bridgekeeper bot commented Oct 17, 2022

Uh oh!

rgiulietti commented Oct 17, 2022

Uh oh!

bridgekeeper bot commented Nov 14, 2022

Uh oh!

rgiulietti commented Dec 12, 2022

Uh oh!

bridgekeeper bot commented Jan 9, 2023

Uh oh!

rgiulietti commented Jan 31, 2023

Uh oh!

plokhotnyuk commented Feb 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1) For the line https://github.com/openjdk/jdk/pull/9410/files#diff-94d400b99466045dd76001c37eada6b24d086d8d115b49c439752bbceb233772L3746

2) For the line: https://github.com/openjdk/jdk/pull/9410/files#diff-94d400b99466045dd76001c37eada6b24d086d8d115b49c439752bbceb233772L3791

3) For the line: https://github.com/openjdk/jdk/pull/9410/files#diff-94d400b99466045dd76001c37eada6b24d086d8d115b49c439752bbceb233772L3800

Uh oh!

rgiulietti commented Feb 13, 2023

Uh oh!

plokhotnyuk commented Feb 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rgiulietti commented Feb 14, 2023

Uh oh!

plokhotnyuk commented Feb 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bridgekeeper bot commented Mar 14, 2023

Uh oh!

rgiulietti commented Apr 10, 2023

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!

rgiulietti commented Jul 7, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Jul 7, 2022 •

edited

Loading

plokhotnyuk commented Feb 13, 2023 •

edited

Loading

plokhotnyuk commented Feb 14, 2023 •

edited

Loading

plokhotnyuk commented Feb 14, 2023 •

edited

Loading

openjdk bot commented Apr 26, 2023 •

edited

Loading