8284493: Improve computeNextExponential tail performance and accuracy

[Pr0methean]

This PR improves both the worst-case performance of nextExponential and nextGaussian and the distribution of output at the tails. It fixes the following imperfections:

• Repeatedly adding DoubleZigguratTables.exponentialX0 to extra causes a rounding error to accumulate at the tail of the distribution (probably starting around 2*exponentialX0 == 0x1.e46eff20739afp3 ~ 15.1); this PR fixes that by tracking the multiple of exponentialX0 as a long. (This distortion is worst when x > 0x1.0p56 since in that case, a rounding error means extra + x == extra.
• Reduces several equations using Math.fma. (This will almost certainly improve performance, and may or may not improve output distribution.)
• Uses the newly-extracted computeWinsorizedNextExponential function to prevent nextGaussian from going into the nextExponential tail twice.

---

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-8284493: Improve computeNextExponential tail performance and accuracy (Enhancement - P4)

Reviewers

• Joe Darcy (@jddarcy - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8131

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

Hi @Pr0methean, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

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 Pr0methean" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

[openjdk]

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

[navyxliu]

/covered
@Pr0methean is an employee of Amazon.

[Pr0methean]

/covered

[bridgekeeper]

Thank you! Please allow for a few business days to verify that your employer has signed the OCA. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

[navyxliu]

hi, @JesperIRL,
Could you help us on OCA coverage?

[Pr0methean]

Should we extract a computeWinsorizedNextExponential that can compute a smaller value for line 1216 to compare the output to, so that line 1218 can realistically be covered in a unit test? Such a method might even be worth exposing in the RandomGenerator interface, in case an approximately exponential distribution is ever needed in a hard real-time system.

[simonis]

/covered

@robilad, can you please verify Chris Hennick's (@Pr0methean) OCA status?
He's an Amazon employee and covered by Amazons OCA.

[mlbridge]

Webrevs

• 21: Full (d5ade1c7)
• 20: Full (1b5656d7)
• 19: Full - Incremental (e4714ec9)
• 18: Full (2470c00a)
• 17: Full (63706c8b)
• 16: Full - Incremental (cc7c425a)
• 15: Full - Incremental (b75139c9)
• 14: Full - Incremental (c94a09e5)
• 13: Full - Incremental (d1c9bafd)
• 12: Full - Incremental (e3f55542)
• 11: Full - Incremental (5919faeb)
• 10: Full - Incremental (cab6b5bb)
• 09: Full - Incremental (db9ff01d)
• 08: Full - Incremental (f74b6872)
• 07: Full - Incremental (d903efe5)
• 06: Full - Incremental (f84e0e03)
• 05: Full - Incremental (13c6d22c)
• 04: Full - Incremental (2c873856)
• 03: Full - Incremental (1daaadac)
• 02: Full - Incremental (c5a28f98)
• 01: Full - Incremental (fa340fb4)
• 00: Full (1fd959cc)

[Pr0methean]

In addition to the changes discussed heretofore, I've also changed line 1382 to eliminate unneeded tail exploration; this should make nextGaussian faster at high percentiles (probably measurable at 99%ile; should definitely be measurable at at 99.99%ile).

[openjdk-notifier]

@Pr0methean Please do not rebase or force-push to an active PR as it invalidates existing review comments. All changes will be squashed into a single commit automatically when integrating. See OpenJDK Developers’ Guide for more information.

[openjdk-notifier]

@Pr0methean Please do not rebase or force-push to an active PR as it invalidates existing review comments. All changes will be squashed into a single commit automatically when integrating. See OpenJDK Developers’ Guide for more information.

[openjdk-notifier]

@Pr0methean Please do not rebase or force-push to an active PR as it invalidates existing review comments. All changes will be squashed into a single commit automatically when integrating. See OpenJDK Developers’ Guide for more information.

[openjdk-notifier]

@Pr0methean Please do not rebase or force-push to an active PR as it invalidates existing review comments. All changes will be squashed into a single commit automatically when integrating. See OpenJDK Developers’ Guide for more information.

[bridgekeeper]

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

[GuySteele]

I have now reviewed all the changes in file .../jdk/internal/util/random/RandomSupport.java . It appears that the method formerly called computeWinsorizedNextExponential has been renamed to computeNextExponentialSoftCapped, and that is a good change. All the changes to the executable code appear to be sound (and, yes, I believe I still remember how all this code is supposed to work—even while admitting that I wrote it some years ago and that a couple of embarrassing mistakes were later discovered and repaired, thank goodness). I looked especially carefully at the change of representation for the variable "extra" from double to long and the new uses of fma. I went back and looked at email exchanges I had with Joe Darcy on May 10, 2022, and I see that many of the concerns I expressed at that time have been addressed.

I do suggest that for extra clarity, the declaration and computation of maxExtraMinus1 at lines 1206--1212 be moved down below to after line 1222, just below the comment that begins "We didn't use the upper part of U1 after all". It may be that good optimizing Java compilers perform this code motion anyway (the code in lines 1213–1221 does not refer to maxExtraMinus1), but it would help a human reader to understand that this code is not part of and does not need to be part of the fast path through the code. Moreover, though a compiler cannot tell that it's okay to move the comparison of maxValue to 0.0 (and the possible forced return of 0.0) off the fast path, I argue that it is indeed okay to do so, because it is always permitted to return a value larger than maxValue, and the fast path does always return a nonnegative value. So I actually argue to move three more lines (in all, lines 1203–1212) to after line 1222.

In all other respects, I recommend that this set of changes be adopted exactly as is.

If making the one change I suggested above might cause adoption to slip from JDK21 to JDK22 (perhaps because of a need for retesting), then I would suggest adopting the code exactly as is and then scheduling the suggested change for JDK22, because the suggested change improves clarity and code speed but should not change the advertised functionality at all.

[Pr0methean]

Moved the code after the fast-path return as requested.

[openjdk]

⚠️ @Pr0methean 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).

[GuySteele]

Latest change looks good. I noted the improvement to the comment beginning "We didn't use the upper part of U1 after all".

[bridgekeeper]

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

[Pr0methean]

@turbanoff @rgiulietti Keep open

[Pr0methean]

@JimLaskey @jddarcy I believe this can now be merged.

[bridgekeeper]

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

[Pr0methean]

@JimLaskey @jddarcy @turbanoff @rgiulietti Could this please be merged?

[jddarcy]

As Guy is not a capital-R reviewer for this project, marking the PR as Approved on his behalf.

[openjdk]

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

8284493: Improve computeNextExponential tail performance and accuracy

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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 (@jddarcy) 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).

[jddarcy]

@Pr0methean , please update your branch with the current contents of the master before issuing the integrate command. Assuming that succeeds as expected, myself or someone else can sponsor your changeset as described in the comment from the Skara bots.

[Pr0methean]

/integrate

[Pr0methean]

@jddarcy Done.

[openjdk]

@Pr0methean
Your change (at version d5ade1c) is now ready to be sponsored by a Committer.

[jddarcy]

/sponsor

[openjdk]

Going to push as commit f975be4.

[openjdk]

@jddarcy @Pr0methean Pushed as commit f975be4.

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