On Tue, 17 Nov 2020 22:21:18 GMT, Jim Laskey <jlas...@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
