Skip to content

8077587: BigInteger Roots #24898

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 60 commits into
base: master
Choose a base branch
from

Conversation

fabioromano1
Copy link
Contributor

@fabioromano1 fabioromano1 commented Apr 26, 2025

This PR implements nth root computation for BigIntegers using Newton method.


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

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24898

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

Using diff file

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

Using Webrev

Link to Webrev Comment

fabioromano1 and others added 30 commits April 16, 2025 09:39
@rgiulietti
Copy link
Contributor

@fabioromano1 Is this proof still relevant?

@fabioromano1
Copy link
Contributor Author

fabioromano1 commented May 12, 2025

@rgiulietti No, the proof of the recurrence used is the one by Zimmermann linked in the code.

@rgiulietti
Copy link
Contributor

@fabioromano1 I guess this work will be the basis for a future work on BigDecimal n-th root?

Otherwise, there needs to be some evidence for useful use cases of general interest to justify adding n-th root to BigInteger.

@fabioromano1
Copy link
Contributor Author

fabioromano1 commented May 14, 2025

I guess this work will be the basis for a future work on BigDecimal n-th root?

@rgiulietti Yes, it is. Some use cases could be primality testing, like AKS, where part of the algorithm detect composite numbers by checking whether an integer is an exact power or not.

@rgiulietti
Copy link
Contributor

I doubt that the AKS primality test is of any practical relevance, although it's a great theoretical achievement.

So if you plan to add a n-th root to BigDecimal in a followup PR, then please keep going on this one.
Otherwise, I can't see a strong need for a BigInteger n-th root in the platform, although I might be wrong.

@fabioromano1
Copy link
Contributor Author

I doubt that the AKS primality test is of any practical relevance, although it's a great theoretical achievement.

Yes, indeed it was just an example, but the exact power detection can be used also as a test apart from the entire AKS algorithm.

@rgiulietti
Copy link
Contributor

The Rampdown Phase One for JDK 25 is about 3 weeks from now.

I think it's a bit too late for API additions to be approved in due time. And even if we could rush this work for 25, it makes little sense to have this in 25 and the future one in 26. IMO, they should be released together. So this and the followup PR for BigDecimal will probably be integrated only in 26.

In other words, take your time ;-)

@fabioromano1
Copy link
Contributor Author

@rgiulietti Anyway, I think the code has reached a stable version, so it is ready for review.

@fabioromano1
Copy link
Contributor Author

fabioromano1 commented May 17, 2025

@rgiulietti Regarding the tests for n-th root, the tests for sqrt() could be extended in order to test also nthRoot().

@rgiulietti
Copy link
Contributor

@fabioromano1 Now that the jdk25 stabilization branch has been created, work on mainline (branch master) will no longer affect JDK25 (except for backports of bug fixes).
I'll soon start reviewing this PR based on the work of Brent and Zimmermann.

@bridgekeeper
Copy link

bridgekeeper bot commented Jul 4, 2025

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

@fabioromano1
Copy link
Contributor Author

/touch

@openjdk
Copy link

openjdk bot commented Jul 4, 2025

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

@rgiulietti
Copy link
Contributor

@fabioromano1 IIUC, the bulk of the code in the PR is aimed at finding a good starting estimate.
Algorithm 1.14 RootInt in Brent & Zimmermann, p. 27-28, however, works for any initial estimate $u$ meeting $u \ge \lfloor m^{1/k}\rfloor$.

Now, assume $2^{l-1} \le m < 2^l$. We have $m^{1/k} < 2^{l/k} \le 2^{\lceil l/k\rceil}$
Thus, the initial estimate could be simple as $u = 2^{\lceil l/k\rceil}$.

This has the following advantages:

  • It's super-easy to determine.
  • Computing the first $t = (k - 1) s + \lfloor m/s^{k-1}\rfloor$ (L.4 of RootInt) is quite efficient and can be done separately using shifts and additions.

I don't know if this has some negative impact on performance in practice, but IMO the resulting code is easier to review, understand, and maintain.

Maybe worth giving it a try.
WDYT?

@fabioromano1
Copy link
Contributor Author

fabioromano1 commented Jul 10, 2025

@rgiulietti It should be noted that the convergence of Newton's method is quadratic, meaning the number of correct bits doubles with each iteration. This means that, to keep the number of iterations low, the larger the input, the greater the need for as many initial correct bits as possible.

Moreover, the initial estimate gives at most 53 bits, so delegating its computation to Math.pow() or Math.cbrt() should be faster than computing it with 1 initial iteration of Newton's method in long arithmetic and next 6 iterations done in BigInteger's arithmetic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs [email protected] rfr Pull request is ready for review
Development

Successfully merging this pull request may close these issues.

2 participants