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

[Paul Sandoz]

On Tue, 17 Nov 2020 22:21:18 GMT, Jim Laskey <jlaskey at openjdk.org> 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 .
>
> Jim Laskey has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 40 commits:
> 
>  - Merge branch 'master' into 8248862
>  - 8248862: Implement Enhanced Pseudo-Random Number Generators
>    
>    Update package-info.java
>  - 8248862: Implement Enhanced Pseudo-Random Number Generators
>    
>    Updated RandomGeneratorFactory javadoc.
>  - 8248862: Implement Enhanced Pseudo-Random Number Generators
>    
>    Updated documentation for RandomGeneratorFactory.
>  - Merge branch 'master' into 8248862
>  - Merge branch 'master' into 8248862
>  - 8248862: Implement Enhanced Pseudo-Random Number Generators
>    
>    Move RandomGeneratorProperty
>  - Merge branch 'master' into 8248862
>  - 8248862: Implement Enhanced Pseudo-Random Number Generators
>    
>    Clear up javadoc
>  - 8248862; Implement Enhanced Pseudo-Random Number Generators
>    
>    remove RandomGeneratorProperty from API
>  - ... and 30 more: https://git.openjdk.java.net/jdk/compare/f7517386...6fe94c68

I am unsure if the intent is also to support external libraries providing `RandomGenerator` implementations. Currently there is an implicit contract for properties (reflectively invoking a method returning a map with a set of entries with known keys). Since the service provider implementation is the `RandomGenerator` itself, rather than `RandomGeneratorFactory` it is harder expose the meta-data with a clearer contract.

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

> 590: 
> 591:     @Override
> 592:     public Spliterator.OfInt makeIntsSpliterator(long index, long fence, int origin, int bound) {

Unsure if this and the other two methods are intended to be public or not, since they are at the end of the class and override methods of a module private class. In principle there is nothing wrong with such `Spliterator` factories, but wonder if they are really needed given the `Stream` returning methods. The arrangement of classes makes it awkward to hide these methods.

src/java.base/share/classes/java/util/SplittableRandom.java line 171:

> 169:      * RandomGenerator properties.
> 170:      */
> 171:     static Map<RandomGeneratorProperty, Object> getProperties() {

With records exiting preview in 16 this map of properties could i think be represented as a record instance, with better type safety, where `RandomSupport.RandomGeneratorProperty` enum values become typed fields declared on the record class. Something to consider after integration perhaps?

src/java.base/share/classes/java/util/SplittableRandom.java line 211:

> 209:      * http://zimbry.blogspot.com/2011/09/better-bit-mixing-improving-on.html
> 210:      */
> 211:     private static long mix64(long z) {

Usages be replaced with calls to `RandomSupport.mixStafford13`?

src/java.base/share/classes/module-info.java line 250:

> 248:     exports jdk.internal.util.xml.impl to
> 249:         jdk.jfr;
> 250:     exports jdk.internal.util.random;

Unqualified export, should this be `to jdk.random`?

src/jdk.random/share/classes/module-info.java line 50:

> 48:  */
> 49: module jdk.random {
> 50:     uses java.util.random.RandomGenerator;

Are these `uses` declarations needed? `ServiceLoader` is not used by this module, and `RandomSupport` is not a service interface.

src/jdk.random/share/classes/module-info.java line 53:

> 51:     uses RandomSupport;
> 52: 
> 53:     exports jdk.random to

Why is this needed?

src/java.base/share/classes/java/util/random/package-info.java line 50:

> 48:  * given its name.
> 49:  *
> 50:  * <p> The principal supporting class is {@link RandomGenertatorFactor}. This

s/RandomGenertatorFactor/RandomGenertatorFactory

src/java.base/share/classes/java/util/random/package-info.java line 140:

> 138:  *
> 139:  * <p> For applications with no special requirements,
> 140:  * "L64X128MixRandom" has a good balance among speed, space,

The documentation assumes that the `jdk.random` module is present in the JDK image. Perhaps we need to spit the specifics to `jdk.random`?

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

> 1209:                         Udiff = -Udiff;
> 1210:                         U2 = U1;
> 1211:                         U1 -= Udiff;

Updated `U1` never used (recommend running the code through a checker e.g. use IntelliJ)

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

> 329:         }
> 330:         // Finally, we need to make sure the last z words are not all zero.
> 331:         search: {

Nice! a rarely used feature :-)

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

> 1155:         /*
> 1156:          * The tables themselves, as well as a number of associated parameters, are
> 1157:          * defined in class java.util.DoubleZigguratTables, which is automatically

`DoubleZigguratTables` is an inner class of `RandomSupport`

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

> 2893:      * distribution: 0.0330
> 2894:      */
> 2895:     static class DoubleZigguratTables {

make `final`

src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line 167:

> 165:      * Return the properties map for the specified provider.
> 166:      *
> 167:      * @param provider  provider to locate.

Method has no such parameter

src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line 173:

> 171:     @SuppressWarnings("unchecked")
> 172:     private Map<RandomGeneratorProperty, Object> getProperties() {
> 173:         if (properties == null) {

`properties` needs to be marked volatile, and it needs to be assigned at line 182 or line 184.

src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line 148:

> 146:      */
> 147:     private static Map<String, Provider<? extends RandomGenerator>> getFactoryMap() {
> 148:         if (factoryMap == null) {

`factoryMap` needs to be marked volatile when using the double checked locking idiom.

src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line 320:

> 318:                         }
> 319:                     }
> 320:                 }

Add an `assert` statement that `ctor`, `ctorLong` and `ctorBytes` are all non-null?

src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line 331:

> 329:      */
> 330:     private void ensureConstructors() {
> 331:         if (ctor == null) {

This check occurs outside of the synchronized block, field may need to be marked volatile. Unsure about the other dependent fields. Might need to store values from loop in `getConstructors` in locals and then assign in appropriate order, assigning the volatile field last.

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

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