8355719: Reduce memory consumption of BigInteger.pow() #24690

fabioromano1 · 2025-04-16T13:45:55Z

This PR optimizes BigInteger.pow(int) method. The primary enhancement in pow() is not concerned most on execution time, but rather in memory optimization, because the PR implementation does the "shift of the exponent" squaring the result rather than the base, so the base is not squared like in the current implementation, and this permits to save about half of the memory.

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-8355719: Reduce memory consumption of BigInteger.pow() (Enhancement - P4)

Reviewers

Raffaello Giulietti (@rgiulietti - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24690

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-04-16T13:46:40Z

👋 Welcome back fabioromano1! 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-04-16T13:47:30Z

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

8355719: Reduce memory consumption of BigInteger.pow()

Reviewed-by: rgiulietti

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

601f05e: 8352480: Don't follow symlinks in additional content for app images
2661f62: 8356036: (fs) FileKey.hashCode and UnixFileStore.hashCode implementations can use Long.hashCode
bed29a0: 8356555: Incorrect use of {@link} in BigDecimal
... and 226 more: https://git.openjdk.org/jdk/compare/21b0f5ea153c633de7f09bdb0399308c890f7e43...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@rgiulietti) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk · 2025-04-16T13:48:35Z

@fabioromano1 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 · 2025-04-16T15:05:11Z

Webrevs

fabioromano1 · 2025-04-16T22:09:08Z

@rgiulietti In this PR I've implemented nth root for BigIntegers using Newton's method and optimized BigInteger.pow(int) method. I've also uploaded a proof of convergence for the recurrence used to compute nth root.

…/jdk into BigInteger-nth-root

openjdk · 2025-04-17T02:31:32Z

⚠️ @fabioromano1 This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

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

rgiulietti · 2025-05-07T15:38:41Z

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

        } else {
-            if ((long)bitLength() * exponent / Integer.SIZE > MAX_MAG_LENGTH) {
+            if ((bitLength() - 1L) * exponent >= (long) MAX_MAG_LENGTH << 5) {


Suggested change

if ((bitLength() - 1L) * exponent >= (long) MAX_MAG_LENGTH << 5) {

if ((bitLength() - 1L) * exponent >= 32L * MAX_MAG_LENGTH) {

or

Suggested change

if ((bitLength() - 1L) * exponent >= (long) MAX_MAG_LENGTH << 5) {

if ((bitLength() - 1L) * exponent >= (long) Integer.SIZE * MAX_MAG_LENGTH) {

Both variant are easier to read, more honest, and exactly as efficient as with the shift. The right-hand sides are compile-time constants, so they have no impact on runtime performance.

More generally, the runtime compilers are perfectly capable to optimize multiplications by constant powers of 2 and replace them with shifts, even if the other operand is not a constant.

@rgiulietti What about (bitLength() - 1L) * exponent >= Integer.MAX_VALUE?

Ah right, but you probably want

Suggested change

if ((bitLength() - 1L) * exponent >= (long) MAX_MAG_LENGTH << 5) {

if ((bitLength() - 1L) * exponent > Integer.MAX_VALUE) {

I mean > rather than >=

Ah right, but you probably want

No, the sufficient condition to get the overflow is (bitLength() - 1L) * exponent + 1L > Integer.MAX_VALUE, which is equivalent to that one I wrote above.

At some point you proposed

(bitLength() - 1L) * exponent >= (long) MAX_MAG_LENGTH << 5

Given the value of MAX_MAG_LENGTH, which is 2^26, this is equivalent to

(bitLength() - 1L) * exponent >= 1L << 31

that is, to

(bitLength() - 1L) * exponent > Integer.MAX_VALUE

What am I missing?

The condition A := (bitLength() - 1L) * exponent + 1L > Integer.MAX_VALUE is more accurate, as it compares the bit length of the result, in fact B := (bitLength() - 1L) * exponent >= (long) MAX_MAG_LENGTH << 5 implies A, but A does not imply B. The BigIntegers can have a mag length up to MAX_MAG_LENGTH, but MAX_MAG_LENGTH * Integer.SIZE > Integer.MAX_VALUE.

OK.

But then your original expression

(((bitLength() - 1L) * exponent) >>> 5) + 1L > MAX_MAG_LENGTH

was a bit too restrictive as well, right?

OK.

But then your original expression

(((bitLength() - 1L) * exponent) >>> 5) + 1L > MAX_MAG_LENGTH

was a bit too restrictive as well, right?

On the contrary, it was too loose, as it admitted a bit length equal to MAX_MAG_LENGTH * Integer.SIZE == 2^31 > Integer.MAX_VALUE.

I mean, if B strictly implies A, then B is more restrictive (stronger).
Since B is equivalent to your original formulation, to me it means that it was more restrictive.

I mean, if B strictly implies A, then B is more restrictive (stronger). Since B is equivalent to your original formulation, to me it means that it was more restrictive.

Exactly.

test/micro/org/openjdk/bench/java/math/BigIntegerPow.java

rgiulietti · 2025-05-07T15:46:16Z

The large number algorithm loop in pow() might deserve an invariant in a comment.

Did you make sure that the existing tests also cover the unsignedLongPow() sufficiently well?

rgiulietti · 2025-05-07T15:48:31Z

JMH benchmarks look good for the random exponents as well.

fabioromano1 · 2025-05-07T16:00:44Z

Did you make sure that the existing tests also cover the unsignedLongPow() sufficiently well?

@rgiulietti There are already the tests for BigInteger's pow, since the code of unsignedLongPow() is basically equivalent to that one in the current implementation.

rgiulietti

Last nits.

I'll approve the PR in the next couple of days for last minute small changes from you.

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

test/micro/org/openjdk/bench/java/math/BigIntegerPow.java

rgiulietti · 2025-05-08T15:48:30Z

Please update the 2nd copyright year in BigInteger.

Otherwise looks good.
As soon as you feel comfortable with your changes, let me know so that I can approve.

Thanks for yet another nice contribution!

fabioromano1 · 2025-05-08T16:42:42Z

@rgiulietti Could be useful this optimization for int bases whose result does not fit in a long?

rgiulietti · 2025-05-08T18:21:59Z

@rgiulietti Could be useful this optimization for int bases whose result does not fit in a long?

There are endless optimizations and fast paths that one could conceive, but at some point we must decide to stop and turn our attention to other aspects of the platform. I think we have reached the turning point for this great PR.

Besides, the performance gains of yet another fast-path might be negligible in general usage, at the cost of another 50 lines or so of code that would probably only benefit very narrow use cases.

fabioromano1 · 2025-05-08T18:30:46Z

Otherwise looks good. As soon as you feel comfortable with your changes, let me know so that I can approve.

@rgiulietti For me, now the code is ready.

rgiulietti · 2025-05-08T18:36:42Z

Good.

Let's give another 24 hours for people around the globe to chime in for last minute comments.

fabioromano1 · 2025-05-09T17:00:24Z

/integrate

openjdk · 2025-05-09T17:00:54Z

@fabioromano1
Your change (at version 261dd31) is now ready to be sponsored by a Committer.

rgiulietti · 2025-05-09T17:06:08Z

/sponsor

openjdk · 2025-05-09T17:06:37Z

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

601f05e: 8352480: Don't follow symlinks in additional content for app images
2661f62: 8356036: (fs) FileKey.hashCode and UnixFileStore.hashCode implementations can use Long.hashCode
bed29a0: 8356555: Incorrect use of {@link} in BigDecimal
... and 226 more: https://git.openjdk.org/jdk/compare/21b0f5ea153c633de7f09bdb0399308c890f7e43...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-05-09T17:06:44Z

@rgiulietti @fabioromano1 Pushed as commit 1c5eb37.

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

fabioromano1 added 2 commits April 16, 2025 09:39

Add nthRoot(int) methods and optimize pow(int)

98a5b53

Merge branch 'openjdk:master' into BigInteger-nth-root

926970e

openjdk bot added the core-libs core-libs-dev@openjdk.org label Apr 16, 2025

Remove trailing whitespaces

a3f1489

openjdk bot added the rfr Pull request is ready for review label Apr 16, 2025

Correct loop recurrence according to proof of convergence

d280c37

Correct initial estimate of nth root for BigIntegers

91c3d1a

openjdk bot removed the rfr Pull request is ready for review label Apr 17, 2025

Removed trailing whitespace

1cfb775

openjdk bot added the rfr Pull request is ready for review label Apr 17, 2025

fabioromano1 added 2 commits April 16, 2025 22:30

Avoid an overflow in computing nth root estimate

c25bd32

Merge branch 'BigInteger-nth-root' of https://github.com/fabioromano1…

9c32ce4

…/jdk into BigInteger-nth-root

fabioromano1 added 12 commits April 17, 2025 07:22

optimize division in loop iteration of nth root

7ff919b

An optimization

1f5a9b4

Format code

b6c3320

Correct left shift if shift is zero

5d971fa

Memory usage optimization

e459c23

An optimization

48650de

BigIntegers nth root's initial estimate optimization

54ec8f8

Extend use cases of MutableBigInteger.valueOf(double)

3ea5190

An optimization

6c9b364

An optimization

3ca7d29

Format code

4516d88

Format code

b427091

rgiulietti reviewed May 7, 2025

View reviewed changes

fabioromano1 and others added 2 commits May 7, 2025 10:50

Suggested changes

4103e49

Removed needless brackets

5ebc16b

rgiulietti reviewed May 7, 2025

View reviewed changes

Code simplification

e0816d5

Suggested changes

009937b

rgiulietti reviewed May 8, 2025

View reviewed changes

src/java.base/share/classes/java/math/BigInteger.java Outdated Show resolved Hide resolved

test/micro/org/openjdk/bench/java/math/BigIntegerPow.java Outdated Show resolved Hide resolved

Suggested changes

2e08f77

Code simplification

261dd31

rgiulietti approved these changes May 9, 2025

View reviewed changes

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

openjdk bot added the sponsor Pull request is ready to be sponsored label May 9, 2025

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

openjdk bot closed this May 9, 2025

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

	if ((bitLength() - 1L) * exponent >= (long) MAX_MAG_LENGTH << 5) {
	if ((bitLength() - 1L) * exponent >= 32L * MAX_MAG_LENGTH) {

	if ((bitLength() - 1L) * exponent >= (long) MAX_MAG_LENGTH << 5) {
	if ((bitLength() - 1L) * exponent > Integer.MAX_VALUE) {

8355719: Reduce memory consumption of BigInteger.pow() #24690

8355719: Reduce memory consumption of BigInteger.pow() #24690

Uh oh!

Conversation

fabioromano1 commented Apr 16, 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 Apr 16, 2025

Uh oh!

openjdk bot commented Apr 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Apr 16, 2025

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

fabioromano1 commented Apr 16, 2025

Uh oh!

openjdk bot commented Apr 17, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rgiulietti May 7, 2025

Choose a reason for hiding this comment

Uh oh!

fabioromano1 May 7, 2025

Choose a reason for hiding this comment

Uh oh!

rgiulietti May 7, 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!

fabioromano1 May 7, 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!

rgiulietti May 7, 2025

Choose a reason for hiding this comment

Uh oh!

fabioromano1 May 7, 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!

rgiulietti May 7, 2025

Choose a reason for hiding this comment

Uh oh!

fabioromano1 May 7, 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!

rgiulietti May 7, 2025

Choose a reason for hiding this comment

Uh oh!

fabioromano1 May 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rgiulietti commented May 7, 2025

Uh oh!

rgiulietti commented May 7, 2025

Uh oh!

fabioromano1 commented May 7, 2025

Uh oh!

rgiulietti left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

fabioromano1 commented Apr 16, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Apr 16, 2025 •

edited

Loading

mlbridge bot commented Apr 16, 2025 •

edited

Loading

rgiulietti May 7, 2025 •

edited

Loading

fabioromano1 May 7, 2025 •

edited

Loading

fabioromano1 May 7, 2025 •

edited

Loading

fabioromano1 May 7, 2025 •

edited

Loading

rgiulietti commented May 8, 2025 •

edited

Loading

fabioromano1 commented May 8, 2025 •

edited

Loading