RFR: 8326227: Rounding error that may distort computeNextGaussian results [v3]

[Jaikiran Pai]

On Wed, 21 Feb 2024 02:18:08 GMT, Chris Hennick <duke at openjdk.org> wrote:

>> This provides a slightly more accurate bounding limit for `computeNextExponentialSoftCapped` when calling it from `computeNextGaussian`. This could cause the `while (computeNextExponentialSoftCapped(rng, limit) < limit)` check in `computeNextGaussian` on line 1402 to always be true, making the `nextGaussian` runtime unbounded in the worst case; but more likely, it would give a result that was truncated too close to zero.
>> 
>> This change is being tested prior to submission to OpenJDK by https://github.com/openjdk/jdk/pull/17703/commits/b8be051cbf40a6a05fafc6a2c76942e9e0b11fdf.
>
> Chris Hennick has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Bug fix: add-exports was for wrong package

src/java.base/share/classes/jdk/internal/util/random/RandomSupport.java line 1214:

> 1212:         // We didn't use the upper part of U1 after all.  We'll probably be able to use it later.
> 1213:         if (maxValue <= DoubleZigguratTables.exponentialX0) {
> 1214:             return DoubleZigguratTables.exponentialX0;

Chris, could you explain why this change to this if block is necessary? Guy notes that (I've paraphrased):

> I can see that (without this proposed change here), if `maxValue` is not greater than `DoubleZigguratTables.exponentialX0`, then the subsequent computation for `maxExtraMinus1`, a few lines below using `Math.round()`, will compute 1 or 2 for the value of `maxExtraMinus1`. But with this proposed change, it just returns `DoubleZigguratTables.exponentialX0`, which by the contract of `computeNextExponentialSoftCapped` is okay (the doc says "{@code maxValue} is a "soft" cap in that a value larger than {@code maxValue} may be returned in order to save a calculation" and I remember that that is true). So that is also okay. The motivation for doing so would be that it saves computation time overall. The part I don't quite understand yet is the judgment that it will in fact save computation time overall. If you take advantage of the "soft cap" then it could cause additional iteration of the loop in `computeNextGaussian` where this `computeNextExponentialSoftCapped` method gets called. I'm u
 ncertain whether the change to this if block is guaranteed or likely to save computation time.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/17703#discussion_r1636024966