Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v17]

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v17]

Jim Laskey-3
> This PR is to introduce a new random number API for the JDK. The primary API is found in RandomGenerator and RandomGeneratorFactory. Further description can be found in the JEP https://openjdk.java.net/jeps/356 .
>
> javadoc can be found at http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/package-summary.html
>
> old PR:  https://github.com/openjdk/jdk/pull/1273

Jim Laskey has updated the pull request incrementally with one additional commit since the last revision:

  Added table of available algorithms.

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1292/files
  - new: https://git.openjdk.java.net/jdk/pull/1292/files/1c17ad35..f1e3699d

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1292&range=16
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1292&range=15-16

  Stats: 181 lines in 3 files changed: 140 ins; 0 del; 41 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1292.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1292/head:pull/1292

PR: https://git.openjdk.java.net/jdk/pull/1292
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v17]

Andrey Turbanov-2
On Thu, 11 Feb 2021 16:02:03 GMT, Jim Laskey <[hidden email]> wrote:

>> This PR is to introduce a new random number API for the JDK. The primary API is found in RandomGenerator and RandomGeneratorFactory. Further description can be found in the JEP https://openjdk.java.net/jeps/356 .
>>
>> javadoc can be found at http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/package-summary.html
>>
>> old PR:  https://github.com/openjdk/jdk/pull/1273
>
> Jim Laskey has updated the pull request incrementally with one additional commit since the last revision:
>
>   Added table of available algorithms.

src/java.base/share/classes/java/util/Random.java line 29:

> 27:
> 28: import java.io.*;
> 29: import java.math.BigInteger;

This import is not actually used

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

PR: https://git.openjdk.java.net/jdk/pull/1292
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v17]

Jim Laskey-3
On Sat, 13 Feb 2021 15:03:53 GMT, Andrey Turbanov <[hidden email]> wrote:

>> Jim Laskey has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Added table of available algorithms.
>
> src/java.base/share/classes/java/util/Random.java line 29:
>
>> 27:
>> 28: import java.io.*;
>> 29: import java.math.BigInteger;
>
> This import is not actually used

good

> src/java.base/share/classes/jdk/internal/util/random/RandomSupport.java line 115:
>
>> 113:     static final String BAD_BOUND = "bound must be positive";
>> 114:     static final String BAD_FLOATING_BOUND = "bound must be finite and positive";
>> 115:     static final String BAD_RANGE = "bound must be greater than origin";
>
> Unused fields in Random class now can be cleaned up: `java.util.Random#BadRange`, `java.util.Random#BadSize`

Good

> src/java.base/share/classes/jdk/internal/util/random/RandomSupport.java line 149:
>
>> 147:      */
>> 148:     public static void checkJumpDistance(double distance) {
>> 149:         if (!(distance > 0.0 && distance < Float.POSITIVE_INFINITY
>
> I wounder if this usage of `Float.POSITIVE_INFINITY` intentional here? Looks a bit suspicious for comparison with `double` argument

Turns out the method is no longer used.

> src/java.base/share/classes/jdk/internal/util/random/RandomSupport.java line 1548:
>
>> 1546:          * @return a stream of (pseudo)randomly chosen {@code int} values
>> 1547:          */
>> 1548:
>
> Is `@Override` intentionally omitted?

The interface method is a default method, so not technically an override.

> src/java.base/share/classes/jdk/internal/util/random/RandomSupport.java line 1943:
>
>> 1941:
>> 1942:         // IllegalArgumentException messages
>> 1943:         static final String BadLogDistance  = "logDistance must be non-negative";
>
> seems unused

Good.

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

PR: https://git.openjdk.java.net/jdk/pull/1292
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v17]

Rémi Forax
On Tue, 16 Feb 2021 14:03:56 GMT, Jim Laskey <[hidden email]> wrote:

>> src/java.base/share/classes/jdk/internal/util/random/RandomSupport.java line 1548:
>>
>>> 1546:          * @return a stream of (pseudo)randomly chosen {@code int} values
>>> 1547:          */
>>> 1548:
>>
>> Is `@Override` intentionally omitted?
>
> The interface method is a default method, so not technically an override.

It's not an override but @Override has a broader semantics than just overriding an existing method.
Given that it helps to understand that this method is part of a larger algorithm, i think this method should be tagged with @Override

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

PR: https://git.openjdk.java.net/jdk/pull/1292
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v17]

Jim Laskey-3
On Tue, 16 Feb 2021 15:54:08 GMT, Rémi Forax <[hidden email]> wrote:

>> The interface method is a default method, so not technically an override.
>
> It's not an override but @Override has a broader semantics than just overriding an existing method.
> Given that it helps to understand that this method is part of a larger algorithm, i think this method should be tagged with @Override

Okay

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

PR: https://git.openjdk.java.net/jdk/pull/1292
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v17]

Jim Laskey-3
In reply to this post by Jim Laskey-3
On Sat, 13 Feb 2021 21:30:12 GMT, Andrey Turbanov <[hidden email]> wrote:

>> Jim Laskey has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Added table of available algorithms.
>
> test/jdk/java/util/Random/RandomTestBsi1999.java line 227:
>
>> 225:     static int checkRunStats(int[] stats) {
>> 226:        int runFailure = 0;
>> 227:            runFailure |= ((2267 <= stats[1]) || (stats[1] <= 2733)) ? 0 : 1;
>
> 1. confusing formatting.
> 2. This condition is always `true`. Looks like `&&` should be used instead of `||`

Correct. Changed.

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

PR: https://git.openjdk.java.net/jdk/pull/1292