On Wed, 12 Jun 2024 08:16:25 GMT, Jaikiran Pai <[email protected]> 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
