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

[Chris Hennick]

On Wed, 12 Jun 2024 08:16:25 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> 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 
 uncertain whether the change to this if block is guaranteed or likely to save computation time.

Updated to return `maxValue` instead. Now it's unambiguously a performance optimization.

> src/java.base/share/classes/jdk/internal/util/random/RandomSupport.java line 1222:
> 
>> 1220:             // Math.round rounds toward infinity in conversion to long; adding 1.0 corrects for any
>> 1221:             // downward rounding error in the division
>> 1222:             maxExtraMinus1 = Math.round(1.0 + maxValue / DoubleZigguratTables.exponentialX0);
> 
> We had asked Guy Steele for his inputs on these proposed changes. Guy reviewed these changes and for this specific if/else block change, he notes that (I've paraphrased it):
> 
>> the (proposed new) computation of `maxExtraMinus1` looks okay to me: it’s always okay for maxExtraMinus1 to be a bit larger than you might expect; the only downside is that the (for) loop starting (on the next line) might take extra iterations.

Is Guy saying it's impossible for `maxExtraMinus1` to be too small even without this change?

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

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