On Fri, 20 Nov 2020 23:30:03 GMT, Roger Baumgartner <github.com+1472572+rogermb@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 .

javadoc can be found at http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/...

old PR: https://github.com/openjdk/jdk/pull/1273

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

...

147: * 148: * <p> For an application running in a 32-bit hardware environment and using 149: * only one thread or a small number of threads, may be a good choice.

I think the name of the suitable algorithm is missing here.

Yes - "L32X64MixRandom". Thank you. ------------- PR: https://git.openjdk.java.net/jdk/pull/1292

Rémi,

On Nov 21, 2020, at 8:03 AM, Remi Forax <forax@univ-mlv.fr> wrote:

Ok, i've taking the time to read some literature about random generators because for me the Mersenne Twister was still the king.

The current API proposed as clearly two levels, you have the user level and the implementation level, at least the implementation level should seen as a SPI

Yes, We agree. It was decided that we work on the SPI separately from the original JEP. IMHO the implementation issues are too complex for a single JEP. So, the goal is to release the user level now, and refine the SPI at a later date. Only RandomGenerator (with descendant interfaces) and RandomGeneratorFactory will be public facing in the first release.

RandomGenerator is the user facing API, for me it should be the sole interface exposed by the API, the others (leap, arbitrary leap and split) should be optional methods.

Fair enough, but if your task requires leapable, you might be disappointed when you get an UnsupportedOperationException invoking leap. I personally like the "to the quick" ability of using LeapableGenerator for narrowing down the search. LeapableGenerator leapable = LeapableGenerator.all().findFirst().orElseThrow(); Open for discussion.

In term of factory methods, we should have user driven methods: - getDefaultGenerator() that currently returns a L64X128MixRandom and can be changed in the future - getFastGenerator() that currently returns a Xoroshiro128PlusPlus and can be changed in the future - getDefault[Splitable|Leapable|etc]Generator that returns a default generator with the methods splits|leaps|etc defined - of / getByName that returns a specific generator by its name (but mov ed in a SPI class)

I'm concerned that the "can be changed in the future" aspect of your default methods will create a false reliance. We sort of have default now with the java.util.Random class. If we were to change the underpinnings of Random all heck would break loose. We already ran into that aspect during testing - tests that relied on sequences from fixed seeds. We try to discourage the use of 'of', but there is a class of user (machine learning for example) that wants to be able to specify exactly. Often, choosing a specific fast prng for testing and then a more sophisticated one for production. The main purpose for RandomGenerator is swapability.

The example in the documentation should use getDefaultGenerator() and not of() to avoid the problem all the programming languages currently have by having over-specified that the default generator is a Mersenne Twister.

I have a problem with this as well. It would make it difficult to deprecate java.util.Random.

All methods that returns a stream of the available implementations should be moved in the SPI package.

Open for discussion.

Rémi

Cheers, -- Jim

--- An honest question, why do we need so many interfaces for the different categories of RandomGenerator ?

My fear is that we are encoding the state of our knowledge of the different kinds of random generators now so it will not be pretty in the future when new categories of random generator are discovered/invented. If we can take example of the past to predict the future, 20 years ago, what should have been the hierarchy at that time. Is it not reasonable to think that we will need new kinds of random generator in the future ?

I wonder if it's not better to have one interface and several optional methods like we have with the collections, it means that we are loosing the possibilities to precisely type a method that only works with a precise type of generator but it will be more future proof.

Rémi

----- Mail original -----

...

De: "Jim Laskey" <jlaskey@openjdk.java.net> À: "core-libs-dev" <core-libs-dev@openjdk.java.net>, "security-dev" <security-dev@openjdk.java.net> Envoyé: Mercredi 18 Novembre 2020 14:52:56 Objet: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators

...

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/...

old PR: https://github.com/openjdk/jdk/pull/1273

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

Commit messages: - 8248862: Implement Enhanced Pseudo-Random Number Generators - 8248862: Implement Enhanced Pseudo-Random Number Generators - 8248862: Implement Enhanced Pseudo-Random Number Generators - 8248862: Implement Enhanced Pseudo-Random Number Generators - 8248862: Implement Enhanced Pseudo-Random Number Generators - 8248862; Implement Enhanced Pseudo-Random Number Generators - 8248862: Implement Enhanced Pseudo-Random Number Generators - 8248862: Implement Enhanced Pseudo-Random Number Generators - 8248862: Implement Enhanced Pseudo-Random Number Generators - 8248862: Implement Enhanced Pseudo-Random Number Generators - ... and 15 more: https://git.openjdk.java.net/jdk/compare/f7517386...2b3e4ed7

Changes: https://git.openjdk.java.net/jdk/pull/1292/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1292&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8248862 Stats: 13319 lines in 25 files changed: 11110 ins; 2132 del; 77 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

On Wed, 25 Nov 2020 13:09:03 GMT, Jim Laskey <jlaskey@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 .

javadoc can be found at http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/...

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:

8248862: Implement Enhanced Pseudo-Random Number Generators

Changes to RandomGeneratorFactory requested by @PaulSandoz

src/java.base/share/classes/java/security/SecureRandom.java line 223:

221: Map.entry(RandomGeneratorProperty.IS_HARDWARE, false) 222: ); 223: }

Using Map.of() instead of Map.ofEntries() should simplify the code src/java.base/share/classes/java/util/Random.java line 129:

127: Map.entry(RandomGeneratorProperty.IS_HARDWARE, false) 128: ); 129: }

Using Map.of() should simplify the code src/java.base/share/classes/java/util/SplittableRandom.java line 181:

179: Map.entry(RandomGeneratorProperty.IS_HARDWARE, false) 180: ); 181: }

Using Map.of() should simplify the code src/java.base/share/classes/java/util/concurrent/ThreadLocalRandom.java line 169:

167: Map.entry(RandomGeneratorProperty.IS_STOCHASTIC, false), 168: Map.entry(RandomGeneratorProperty.IS_HARDWARE, false) 169: );

Using Map.of() should simplify the code src/java.base/share/classes/java/util/concurrent/ThreadLocalRandom.java line 433:

431: private static final class ThreadLocalRandomProxy extends Random { 432: @java.io.Serial 433: static final long serialVersionUID = 0L;

should be private src/java.base/share/classes/java/util/concurrent/ThreadLocalRandom.java line 444:

442: } 443: 444: private static final AbstractSpliteratorGenerator proxy = new ThreadLocalRandomProxy();

should be declared inside ThreadLocalRandomProxy, so the value is only initialized when proxy() is called src/java.base/share/classes/java/util/concurrent/ThreadLocalRandom.java line 453:

451: * @return a {@code RandomGenerator} object that uses {@code ThreadLocalRandom} 452: */ 453: public static RandomGenerator proxy() {

proxy is a generic name, is there a better name here ? src/java.base/share/classes/java/util/concurrent/ThreadLocalRandom.java line 459:

457: // Methods required by class AbstractSpliteratorGenerator 458: public Spliterator.OfInt makeIntsSpliterator(long index, long fence, int origin, int bound) { 459: return new RandomIntsSpliterator(proxy, index, fence, origin, bound);

should use proxy() ------------- PR: https://git.openjdk.java.net/jdk/pull/1292

On Wed, 25 Nov 2020 13:09:03 GMT, Jim Laskey <jlaskey@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 .

javadoc can be found at http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/...

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:

8248862: Implement Enhanced Pseudo-Random Number Generators

Changes to RandomGeneratorFactory requested by @PaulSandoz

src/java.base/share/classes/java/util/random/RandomGenerator.java line 745:

743: * if the period is unknown. 744: */ 745: BigInteger UNKNOWN_PERIOD = BigInteger.ZERO;

move those 3 values into RandomGeneratorFactory ? ------------- PR: https://git.openjdk.java.net/jdk/pull/1292

On Wed, 25 Nov 2020 13:55:52 GMT, Jim Laskey <jlaskey@openjdk.org> wrote:

...

src/java.base/share/classes/java/util/concurrent/ThreadLocalRandom.java line 453:

...

451: * @return a {@code RandomGenerator} object that uses {@code ThreadLocalRandom} 452: */ 453: public static RandomGenerator proxy() {

proxy is a generic name, is there a better name here ?

will investigate

agree

...

src/java.base/share/classes/java/util/concurrent/ThreadLocalRandom.java line 459:

...

457: // Methods required by class AbstractSpliteratorGenerator 458: public Spliterator.OfInt makeIntsSpliterator(long index, long fence, int origin, int bound) { 459: return new RandomIntsSpliterator(proxy, index, fence, origin, bound);

should use proxy()

will investigate

Needed to use ThreadLocalRandomProxy.proxy otherwise a cast would be required. ------------- PR: https://git.openjdk.java.net/jdk/pull/1292

On Wed, 25 Nov 2020 13:55:32 GMT, Jim Laskey <jlaskey@openjdk.org> wrote:

...

src/java.base/share/classes/java/util/random/RandomGenerator.java line 745:

...

743: * if the period is unknown. 744: */ 745: BigInteger UNKNOWN_PERIOD = BigInteger.ZERO;

move those 3 values into RandomGeneratorFactory ?

Will ponder on this one. I tend to agree with you since they are most likely to be used for filtering factories RandomGeneratorFactory querying was a later development. period() originally was a RandomGenerator query, but got moved when more queries were added. This will require a CSR and will come later.

Now unused and removed. ------------- PR: https://git.openjdk.java.net/jdk/pull/1292

On Wed, 25 Nov 2020 13:37:02 GMT, Rémi Forax <github.com+828220+forax@openjdk.org> wrote:

...

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

8248862: Implement Enhanced Pseudo-Random Number Generators

Changes to RandomGeneratorFactory requested by @PaulSandoz

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

...

86: * <pre>{@code 87: * RandomGeneratorFactory<RandomGenerator> best = RandomGenerator.all() 88: * .sorted((f, g) -> Integer.compare(g.stateBits(), f.stateBits()))

use Comparator.comparingInt(RandomGenerator::stateBits) instead of the lambda

Not sure that `.sorted(Comparator.comparingInt(RandomGeneratorFactory<RandomGenerator>::stateBits).reversed())` is simpler than `.sorted((f, g) -> Integer.compare(g.stateBits(), f.stateBits()))`.

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

...

114: * Map of properties for provider. 115: */ 116: private volatile Map<RandomGeneratorProperty, Object> properties = null;

`= null` is useless here

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

On Wed, 25 Nov 2020 16:26:12 GMT, Rémi Forax <github.com+828220+forax@openjdk.org> wrote:

...

Not sure that `.sorted(Comparator.comparingInt(RandomGeneratorFactory<RandomGenerator>::stateBits).reversed())` is simpler than `.sorted((f, g) -> Integer.compare(g.stateBits(), f.stateBits()))`.

At least, it's more clear that it's reversed, i've initially miss the fact that f and g are swapped. And :: is able to do inference so, i believe it can be written `.sorted(Comparator.comparingInt(RandomGeneratorFactory::stateBits).reversed())`

Unfortunately it couldn't be inferred ------------- PR: https://git.openjdk.java.net/jdk/pull/1292

On Wed, 25 Nov 2020 13:38:59 GMT, Rémi Forax <github.com+828220+forax@openjdk.org> wrote:

...

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

8248862: Implement Enhanced Pseudo-Random Number Generators

Changes to RandomGeneratorFactory requested by @PaulSandoz

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

...

104: * Map of provider classes. 105: */ 106: private static volatile Map<String, Provider<? extends RandomGenerator>> factoryMap;

should be FACTORY_MAP given that it's a static field

will fall out of using class holder idiom

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

...

149: if (fm == null) { 150: synchronized (RandomGeneratorFactory.class) { 151: if (RandomGeneratorFactory.factoryMap == null) {

if `RandomGeneratorFactory.factoryMap` is not null, it returns null because the value is not stored in fm.

Please use the class holder idiom (cf Effective Java) instead of using the double-check locking pattern.

? set in 148 and 152, but class holder idiom +1 ------------- PR: https://git.openjdk.java.net/jdk/pull/1292

On Wed, 25 Nov 2020 16:22:32 GMT, Jim Laskey <jlaskey@openjdk.org> wrote:

...

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

...

104: * Map of provider classes. 105: */ 106: private static volatile Map<String, Provider<? extends RandomGenerator>> factoryMap;

should be FACTORY_MAP given that it's a static field

will fall out of using class holder idiom

IntelliJ warns that this volatile field is unused. It has been replaced by the method getFactoryMap(). ------------- PR: https://git.openjdk.java.net/jdk/pull/1292

On Wed, 25 Nov 2020 13:45:46 GMT, Rémi Forax <github.com+828220+forax@openjdk.org> wrote:

...

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

8248862: Implement Enhanced Pseudo-Random Number Generators

Changes to RandomGeneratorFactory requested by @PaulSandoz

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

...

155: .stream() 156: .filter(p -> !p.type().isInterface()) 157: .collect(Collectors.toMap(p -> p.type().getSimpleName().toUpperCase(),

toUpperCase() depends on the Locale !

It was a lame thing to do anyway - removed ------------- PR: https://git.openjdk.java.net/jdk/pull/1292

On Wed, 25 Nov 2020 13:54:47 GMT, Rémi Forax <github.com+828220+forax@openjdk.org> wrote:

...

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

8248862: Implement Enhanced Pseudo-Random Number Generators

Changes to RandomGeneratorFactory requested by @PaulSandoz

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

...

233: throws IllegalArgumentException { 234: Map<String, Provider<? extends RandomGenerator>> fm = getFactoryMap(); 235: Provider<? extends RandomGenerator> provider = fm.get(name.toUpperCase());

again use of toUpperCase() instead of toUpperCase(Locale)

removed

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

...

248: * @return Stream of matching Providers. 249: */ 250: static <T extends RandomGenerator> Stream<RandomGeneratorFactory<T>> all(Class<? extends RandomGenerator> category) {

this signature is weird, T is not used in the parameter, so in case return any type of Stream<RandomGeneratorFactory<T>> from a type POV, should it be ` <T extends RandomGenerator> Stream<RandomGeneratorFactory<T>> all(Class<? extends T> category)` instead ?

agree

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

...

267: * @throws IllegalArgumentException when either the name or category is null 268: */ 269: static <T> T of(String name, Class<? extends RandomGenerator> category)

Same issue as above, T is not used as a constraint

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

On Wed, 25 Nov 2020 14:10:17 GMT, Rémi Forax <github.com+828220+forax@openjdk.org> wrote:

...

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

8248862: Implement Enhanced Pseudo-Random Number Generators

Changes to RandomGeneratorFactory requested by @PaulSandoz

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

...

495: ensureConstructors(); 496: return ctorLong.newInstance(seed); 497: } catch (Exception ex) {

this one is very dubious because the result in an exception is thrown is a random generator with the wrong seed

This is explained in the docs.

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

...

478: } catch (Exception ex) { 479: // Should never happen. 480: throw new IllegalStateException("Random algorithm " + name() + " is missing a default constructor");

chain the exception ...

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

On Wed, 25 Nov 2020 13:09:03 GMT, Jim Laskey <jlaskey@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 .

javadoc can be found at http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/...

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:

8248862: Implement Enhanced Pseudo-Random Number Generators

Changes to RandomGeneratorFactory requested by @PaulSandoz

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

44: import java.util.stream.Stream; 45: import jdk.internal.util.random.RandomSupport.RandomGeneratorProperty; 46:

Instead of calling a method properties to create a Map, it's usually far easier to use an annotation, annotation values supports less runtime type so BigInteger is not supported but using a String instead should be OK. ------------- PR: https://git.openjdk.java.net/jdk/pull/1292

On Thu, 26 Nov 2020 15:41:16 GMT, Jim Laskey <jlaskey@openjdk.org> wrote:

...

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

...

44: import java.util.stream.Stream; 45: import jdk.internal.util.random.RandomSupport.RandomGeneratorProperty; 46:

Instead of calling a method properties to create a Map, it's usually far easier to use an annotation, annotation values supports less runtime type so BigInteger is not supported but using a String instead should be OK.

I kind of like the idea - not sure how expressive a BigInteger string is though. I might be able to express as <i, j, k> BigInteger.ONE.shiftLeft(i).subtract(j).shiftLeft(k). Will ponder.

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

On Mon, 4 Jan 2021 13:54:14 GMT, Jim Laskey <jlaskey@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 .

javadoc can be found at http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/...

old PR: https://github.com/openjdk/jdk/pull/1273

Stayin' alive

Looking for some final code reviews. ------------- PR: https://git.openjdk.java.net/jdk/pull/1292

On Fri, 26 Feb 2021 13:15:28 GMT, Rémi Forax <github.com+828220+forax@openjdk.org> wrote:

...

Looking for some final code reviews.

I still don't like the fact that the factory uses reflection but i don't see how to do better

This is now looking very nicely structured. The only thing i am unsure are the details around `RandomGenerator` being a service provider interface. The documentation mentions this at various points (mostly as implementation notes), but it's not really called out on `RandomGenerator`. Trying out the patch, I can implement `RandomGenerator` and register it as a service: public class AlwaysZero implements RandomGenerator { @Override public long nextLong() { return 0; } } ... RandomGenerator alwaysZero = RandomGenerator.of("AlwaysZero"); Is that intended? (esp. since the annotation `RandomGeneratorProperties` is not public). If i rename the above to `L32X64MixRandom` an `ExceptionInInitializerError` is produced due to duplicate keys. I suspect you want to filter out the service providers to those that only declare `RandomGeneratorProperties`, thereby restricting to providers only supplied by the platform. ------------- PR: https://git.openjdk.java.net/jdk/pull/1292

On Tue, 16 Mar 2021 20:37:57 GMT, Tommy Ettinger <github.com+160684+tommyettinger@openjdk.org> wrote:

...

This is now looking very nicely structured.

The only thing i am unsure are the details around `RandomGenerator` being a service provider interface. The documentation mentions this at various points (mostly as implementation notes), but it's not really called out on `RandomGenerator`.

Trying out the patch, I can implement `RandomGenerator` and register it as a service:

public class AlwaysZero implements RandomGenerator { @Override public long nextLong() { return 0; } } ... RandomGenerator alwaysZero = RandomGenerator.of("AlwaysZero");

Is that intended? (esp. since the annotation `RandomGeneratorProperties` is not public). If i rename the above to `L32X64MixRandom` an `ExceptionInInitializerError` is produced due to duplicate keys.

I suspect you want to filter out the service providers to those that only declare `RandomGeneratorProperties`, thereby restricting to providers only supplied by the platform.

Has anyone here benchmarked this? I've run BumbleBench benchmarks (one of the AdoptOpenJDK team's tools, [available here](https://github.com/adoptium/bumblebench)) and I'm skeptical of the original claims in [JEP 356](https://openjdk.java.net/jeps/356), namely:

...

...a new class of splittable PRNG algorithms (LXM) has also been discovered that are almost as fast [as SplittableRandom]...

On my machine, L64X128MixRandom's algorithm is 53% slower than SplittableRandom, a halving in performance that I think would be inaccurate to describe as "almost as fast." I've benchmarked on Java 8 HotSpot (Windows 10, x64) and Java 15 OpenJ9 (same machine). On HotSpot, which I think is the main concern, I go from 1021770880 (over 1 billion) random longs per second with SplittableRandom to 479769280 (over 479 million) with my (I believe faithful) implementation of L64X128MixRandom. That is where I observed the 53% reduction. While SplittableRandom specifically seems to perform relatively badly on OpenJ9, with 893283072 longs per second (893 million), other similar random number generators do extremely well on OpenJ9; [LaserRandom](https://github.com/tommyettinger/jdkgdxds/blob/master/src/main/java/com/gith...) generates 4232752128 random longs per second (4.2 billion) where L64X128MixRandom gets 840015872 (840 million). My benchmark repo is a mess, but if anyone wants to see and verify, [here it is](https://github.com/tommyettinger/assorted-benchmarks/tree/master/src/main/ja...). JMH benchmarks might provide different or just additional useful information; I've only run BumbleBench.

One could make the argument that getting a random long from a PRNG takes so little time in the first place that it is unlikely to be a bottleneck, and by that logic, LXM is "almost as fast." However, if random generation is not being called often enough for its speed to matter, you are exceedingly unlikely to need so many (over 9 quintillion) parallel streams or such a long period per stream ((2 to the 192) minus (2 to the 64)). LXM is also 1-dimensionally equidistributed, while the foundation it is built on should allow 4-dimensional equidistribution (with the caveat of any LFSR that an all-zero state is impossible), with the same memory use per generator (4 longs), a longer period, and comparable quality using `xoshiro256**`, or possibly `xoshiro256++`, giving up streams but permitting twice as many leaps as LXM has streams if you maintain the same period as L64X128MixRandom.

If I were implementing a PRNG to operate in a future official version of the JVM, I would definitely look into ways to use AES-NI, and I think the interfaces provided here should be valuable for any similar addition, even if I disagree with the provided implementations of those interfaces. Thank you for your time.

This is now looking very nicely structured.

The only thing i am unsure are the details around `RandomGenerator` being a service provider interface. The documentation mentions this at various points (mostly as implementation notes), but it's not really called out on `RandomGenerator`.

Trying out the patch, I can implement `RandomGenerator` and register it as a service:

```java public class AlwaysZero implements RandomGenerator { @Override public long nextLong() { return 0; } } ... RandomGenerator alwaysZero = RandomGenerator.of("AlwaysZero"); ```

Is that intended? (esp. since the annotation `RandomGeneratorProperties` is not public). If i rename the above to `L32X64MixRandom` an `ExceptionInInitializerError` is produced due to duplicate keys.

I suspect you want to filter out the service providers to those that only declare `RandomGeneratorProperties`, thereby restricting to providers only supplied by the platform.

Added the filter.

Has anyone here benchmarked this? I've run BumbleBench benchmarks (one of the AdoptOpenJDK team's tools, [available here](https://github.com/adoptium/bumblebench)) and I'm skeptical of the original claims in [JEP 356](https://openjdk.java.net/jeps/356), namely:

...

...a new class of splittable PRNG algorithms (LXM) has also been discovered that are almost as fast [as SplittableRandom]...

On my machine, L64X128MixRandom's algorithm is 53% slower than SplittableRandom, a halving in performance that I think would be inaccurate to describe as "almost as fast." I've benchmarked on Java 8 HotSpot (Windows 10, x64) and Java 15 OpenJ9 (same machine). On HotSpot, which I think is the main concern, I go from 1021770880 (over 1 billion) random longs per second with SplittableRandom to 479769280 (over 479 million) with my (I believe faithful) implementation of L64X128MixRandom. That is where I observed the 53% reduction. While SplittableRandom specifically seems to perform relatively badly on OpenJ9, with 893283072 longs per second (893 million), other similar random number generators do extremely well on OpenJ9; [LaserRandom](https://github.com/tommyettinger/jdkgdxds/blob/master/src/main/java/com/gith...) generates 4232752128 random longs per second (4.2 billion) where L64X128MixRandom gets 840015872 (840 million). My benchmark repo is a mess, but if anyone wants to see and verify, [here it is](https://github.com/tommyettinger/assorted-benchmarks/tree/master/src/main/ja...). JMH benchmarks might provide different or just additional useful information; I've only run BumbleBench.

One could make the argument that getting a random long from a PRNG takes so little time in the first place that it is unlikely to be a bottleneck, and by that logic, LXM is "almost as fast." However, if random generation is not being called often enough for its speed to matter, you are exceedingly unlikely to need so many (over 9 quintillion) parallel streams or such a long period per stream ((2 to the 192) minus (2 to the 64)). LXM is also 1-dimensionally equidistributed, while the foundation it is built on should allow 4-dimensional equidistribution (with the caveat of any LFSR that an all-zero state is impossible), with the same memory use per generator (4 longs), a longer period, and comparable quality using `xoshiro256**`, or possibly `xoshiro256++`, giving up streams but permitting twice as many leaps as LXM has streams if you maintain the same period as L64X128MixRandom.

If I were implementing a PRNG to operate in a future official version of the JVM, I would definitely look into ways to use AES-NI, and I think the interfaces provided here should be valuable for any similar addition, even if I disagree with the provided implementations of those interfaces. Thank you for your time.

Thanks for this feedback. -- Guy ------------- PR: https://git.openjdk.java.net/jdk/pull/1292

On Mon, 4 Jan 2021 19:52:18 GMT, Jim Laskey <jlaskey@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 .

javadoc can be found at http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/...

old PR: https://github.com/openjdk/jdk/pull/1273

Jim Laskey has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 34 commits:

- Merge branch 'master' into 8248862 - 8248862: Implement Enhanced Pseudo-Random Number Generators

Added coverage testing - 8248862: Implement Enhanced Pseudo-Random Number Generators

Cleanups from Chris H. - 8248862: Implement Enhanced Pseudo-Random Number Generators

Propagate exception - 8248862: Implement Enhanced Pseudo-Random Number Generators

Override AbstractSplittableGenerator - 8248862: Implement Enhanced Pseudo-Random Number Generators

Fix extends - 8248862: Implement Enhanced Pseudo-Random Number Generators

Use Map.of instead of Map.ofEntries - 8248862: Implement Enhanced Pseudo-Random Number Generators

Changes to RandomGeneratorFactory requested by @PaulSandoz - Review changes

@PaulSandoz and @rogermb - 8248862: Implement Enhanced Pseudo-Random Number Generators

Update package-info.java - ... and 24 more: https://git.openjdk.java.net/jdk/compare/f80c63b3...e75cc844

src/java.base/share/classes/java/util/random/RandomGenerator.java line 439:

437: * Fills a user-supplied byte array with generated byte values 438: * (pseudo)randomly chosen uniformly from the range of values between -128 439: * (inclusive) and 255 (inclusive).

Should this be between -128 (inclusive) and **127** (inclusive) ------------- PR: https://git.openjdk.java.net/jdk/pull/1292

On Wed, 6 Jan 2021 16:09:25 GMT, Brett Okken <github.com+2996845+bokken@openjdk.org> wrote:

...

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

Correct range used by nextBytes

src/java.base/share/classes/java/util/random/RandomGenerator.java line 147:

...

145: * 146: * @implNote Since algorithms will improve over time, there is no 147: * guarantee that this method will return the same algorithm each time.

is there a guarantee that within a running jvm each invocation will return the same algorithm?

In practice terms yes. The point is that the user should not assume they will be getting the same algorithm each time. ------------- PR: https://git.openjdk.java.net/jdk/pull/1292

On Fri, 29 Jan 2021 00:15:16 GMT, Mark Reinhold <mr@openjdk.org> wrote:

...

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

Update package info to credit LMX algorithm

src/java.base/share/classes/java/util/random/RandomGenerator.java line 110:

...

108: /** 109: * Returns an instance of {@link RandomGenerator} that utilizes the 110: * {@code name} algorithm.

Shouldn't this method, and related methods, mention the fact that `RandomGenerator` instances are located as services? I see no mention of of that fact anywhere, unless I missed it, but I do see the `uses` and `provides` declarations in the module declaration. A paragraph explaining how services are used here, perhaps in the package specification, would be ideal.

I agree. Will add. ------------- PR: https://git.openjdk.java.net/jdk/pull/1292

On Thu, 11 Feb 2021 16:02:03 GMT, Jim Laskey <jlaskey@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 .

javadoc can be found at http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/...

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

On Tue, 16 Feb 2021 14:03:56 GMT, Jim Laskey <jlaskey@openjdk.org> 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

On Thu, 11 Feb 2021 16:02:03 GMT, Jim Laskey <jlaskey@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 .

javadoc can be found at http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/...

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/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` ------------- PR: https://git.openjdk.java.net/jdk/pull/1292

On Sat, 13 Feb 2021 21:30:12 GMT, Andrey Turbanov <github.com+741251+turbanoff@openjdk.org> 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

On Sat, 20 Feb 2021 00:09:51 GMT, Joe Darcy <darcy@openjdk.org> wrote:

...

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

Remove tabs from random/package-info.java

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

...

191: * 192: * 193: * <h2><a id="algorithms">Random Number Generator Algorithms Available in Java SE</a></h2>

Some comments and questions on the spec status of this table: is the intentional to require all of these algorithms in all compliant implementation of Java SE or just in the JDK reference implementation? Is the list intended to be exhaustive, meaning no other algorithms should be findable?

I recommend clarifying the intended Java SE vs JDK status here. Also, I recommend including wording along the lines of "this list may change in the future" and "an implementation may provide additional algorithms" etc.

Also, to aid future evolution of the set of algorithms, was there consideration of an "isDeprecated" predicate so that algorithms could be so-marked for a while before being dropped?

Precise advice on wording please. The table reflect algorithms guaranteed findable in JDK 17. Whether they are required in all other deployments is something the CSR review should define. "this list may change in the future" is a good idea. Not sure "isDeprecated" can be done in a meaningful way. This would require that users would have to add that filter to every query or have all() not include deprecated algorithms. Better to be prepared to handle not finding the algorithm. ------------- PR: https://git.openjdk.java.net/jdk/pull/1292

On Fri, 26 Feb 2021 19:30:09 GMT, Roger Riggs <rriggs@openjdk.org> wrote:

...

Jim Laskey has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 57 commits:

- Merge branch 'master' into 8248862 - Adjust ThreadLocalRandom javadoc inheritence - L32X64StarStarRandom -> L32X64MixRandom - Various corrects - Revised javadoc per CSR reviews - Remove tabs from random/package-info.java - Correct copyright notice. - Merge branch 'master' into 8248862 - Update tests for RandomGeneratorFactory.all() - Merge branch 'master' into 8248862 - ... and 47 more: https://git.openjdk.java.net/jdk/compare/0257caad...b9094279

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

...

230: Provider<? extends RandomGenerator> provider = fm.get(name); 231: if (!isSubclass(category, provider)) { 232: throw new IllegalArgumentException(name + " is an unknown random number generator");

The message is a bit vague. How about:

"The random number generator does not support the category"

throw new IllegalArgumentException("The random number generator "" + name + "" can not be located"); ------------- PR: https://git.openjdk.java.net/jdk/pull/1292

On Mon, 1 Mar 2021 15:12:46 GMT, Roger Riggs <rriggs@openjdk.org> wrote:

...

throw new IllegalArgumentException("The random number generator "" + name + "" can not be located");

The message only captures the failure if the result of `fm.get()` is null. It does not capture the failure if the name is found but does not support the category.

if (provider == null) { throw new IllegalArgumentException("No implementation of the random number generator algorithm "" + name + "" is available"); } else if (!isSubclass(category, provider)) { throw new IllegalArgumentException("The random number generator algorithm "" + name + "" is not implemented with the interface "" + category.simpleName() + """); } ------------- PR: https://git.openjdk.java.net/jdk/pull/1292

On Fri, 26 Feb 2021 21:32:12 GMT, Roger Riggs <rriggs@openjdk.org> wrote:

...

Jim Laskey has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 57 commits:

- Merge branch 'master' into 8248862 - Adjust ThreadLocalRandom javadoc inheritence - L32X64StarStarRandom -> L32X64MixRandom - Various corrects - Revised javadoc per CSR reviews - Remove tabs from random/package-info.java - Correct copyright notice. - Merge branch 'master' into 8248862 - Update tests for RandomGeneratorFactory.all() - Merge branch 'master' into 8248862 - ... and 47 more: https://git.openjdk.java.net/jdk/compare/0257caad...b9094279

src/java.base/share/classes/java/util/random/RandomGenerator.java line 1313:

...

1311: * furthermore can easily <i>jump</i> to an arbitrarily specified distant 1312: * point in the state cycle. 1313: *

The similarity of the first sentence of each of the Jumpable, Leapable, and arbitrarlyJumpable interface is so similar as to obscure the differences. You have to read 25 words in to be able to find the description that makes them different. The italics help but should include the whole of the phrase that distinguishes them. Alternatively, move the phrase to the beginning of the sentence or drop the redundant phrasing. "provide a common protocol of objects that generate pseudorandom sequences of numbers of Boolean values", etc.

Jumpable: * This interface is designed to provide a common protocol for objects that * generate pseudorandom sequences of numbers (or Boolean values) and * furthermore can easily <i>jump</i> forward, by a moderate amount (ex. * 2⁶⁴) to a distant point in the state cycle. Leapable: * This interface is designed to provide a common protocol for objects that * generate sequences of pseudorandom numbers (or Boolean values) and * furthermore can easily not only jump but also * <i>leap</i> forward, by a large amount (ex. 2¹²⁸), to a * very distant point in the state cycle. ArbitrarilyJumpable: * This interface is designed to provide a common protocol for objects that * generate sequences of pseudorandom numbers (or Boolean values) and * furthermore can easily <i>jump</i> forward, by an arbitrary amount, to a * distant point in the state cycle.

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

...

53: * 54: * A specific {@link RandomGeneratorFactory} can be located by using the 55: * {@link RandomGenerator#factoryOf(String)} method, where the argument string

Broken link: the method is in this class. should be just "#factoryOf".

Modified

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

...

598: try { 599: ensureConstructors(); 600: return ctorBytes.newInstance((Object)seed);

IntelliJ warns that the cast to (Object) is redundant.

Modified

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

...

27: 28: import java.lang.reflect.Constructor; 29: import java.lang.reflect.Method;

Used import.

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

On Mon, 15 Mar 2021 23:02:33 GMT, Joe Darcy <darcy@openjdk.org> wrote:

...

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

Missing @since

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

...

133: * number generator which is maintained by method {@link #next}. 134: * 135: * @implSpec <p>The invocation {@code new Random(seed)} is equivalent to:

This is not conventional formatting for an implSpec section. I suggest putting implSpec on its own line and then left-justifying the text as normal. A new paragraph tag is no needed at the beginning of implSpec.

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

On Mon, 15 Mar 2021 23:07:53 GMT, Joe Darcy <darcy@openjdk.org> wrote:

...

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

Missing @since

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

...

60: @Retention(RetentionPolicy.RUNTIME) 61: @Target(ElementType.TYPE) 62: public @interface RandomGeneratorProperties {

Should the is-deprecated information be stored here?

I don't think so. That would require the deprecation state be stored in two places. I think it's sufficient to rely on the presence of @Deprecated. ------------- PR: https://git.openjdk.java.net/jdk/pull/1292

On Thu, 18 Mar 2021 21:43:16 GMT, Joe Darcy <darcy@openjdk.org> wrote:

...

Jim Laskey has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 67 commits:

- Merge branch 'master' into 8248862 - Review revisions - Missing @since - Review revisions - Review requested changes - Merge branch 'master' into 8248862 - Remove conflicts - Use isAnnotationPresent - Introduce isDeprecated - Update javadoc - ... and 57 more: https://git.openjdk.java.net/jdk/compare/8c8d1b31...63094f9d

src/java.base/share/classes/java/util/random/RandomGenerator.java line 1290:

...

1288: * @return a new object that is a copy of this generator 1289: */ 1290: LeapableGenerator copy();

Suggest adding an Override annotation here and possibly to inheritDoc the text from Jumpable.copy.

Fails due to result variance.

src/java.base/share/classes/java/util/random/RandomGenerator.java line 1455:

...

1453: * period of this generator 1454: */ 1455: void jump(double distance);

Suggest Override and inheritDoc combo.

jump(double distance) does not occur in superinterface. The ability to jump arbitrarily is specific to ArbitrarilyJumpableGenerator.

src/java.base/share/classes/java/util/random/RandomGenerator.java line 1465:

...

1463: * @implSpec The default implementation invokes jump(jumpDistance()). 1464: */ 1465: default void jump() { jump(jumpDistance()); }

Should be able to avoid defining the jump method in this subinterface.

jump(double distance) does not occur in superinterface. The ability to jump arbitrarily is specific to ArbitrarilyJumpableGenerator.

src/java.base/share/classes/java/util/random/RandomGenerator.java line 1486:

...

1484: * ({@link Long#MAX_VALUE Long.MAX_VALUE}). 1485: */ 1486: default Stream<ArbitrarilyJumpableGenerator> jumps(double distance) {

Suggest adding an Override annotation.

jump(double distance) does not occur in superinterface. The ability to jump arbitrarily is specific to ArbitrarilyJumpableGenerator.

src/java.base/share/classes/java/util/random/RandomGenerator.java line 1521:

...

1519: * {@link ArbitrarilyJumpableGenerator#leapDistance() leapDistance}(). 1520: */ 1521: default void leap() { jump(leapDistance()); }

Should need to define this method in the subinterface of Leapable.

jump(double distance) does not occur in superinterface. The ability to jump arbitrarily is specific to ArbitrarilyJumpableGenerator.

src/java.base/share/classes/java/util/random/RandomGenerator.java line 1538:

...

1536: * returns the copy. 1537: */ 1538: default ArbitrarilyJumpableGenerator copyAndJump(double distance) {

Suggest Override and inheritDoc.

jump(double distance) does not occur in superinterface. The ability to jump arbitrarily is specific to ArbitrarilyJumpableGenerator. ------------- PR: https://git.openjdk.java.net/jdk/pull/1292

On Fri, 26 Mar 2021 12:25:43 GMT, Jim Laskey <jlaskey@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 .

javadoc can be found at http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/...

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:

CSR review revisions

Marked as reviewed by darcy (Reviewer). ------------- PR: https://git.openjdk.java.net/jdk/pull/1292

On Mon, 5 Apr 2021 14:20:56 GMT, Jim Laskey <jlaskey@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 .

javadoc can be found at http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/...

old PR: https://github.com/openjdk/jdk/pull/1273

Jim Laskey has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 78 commits:

- Merge branch 'master' into 8248862 - Fix NotCompliantCauseTest to not rely on Random not being a bean - Merge branch 'master' into 8248862 - Correct return type of RandomGeneratorFactory.of - Merge branch 'master' into 8248862 - CSR review revisions - CSR review updates - Removed @since from nextDouble ThreadLocalRandom - RandomGeneratorFactory::all(Class<T> category) @implNote was out of date - Clarify all() - ... and 68 more: https://git.openjdk.java.net/jdk/compare/a8005efd...ffd982b0

The package javadoc of java.util.random says that `mixLea32` is used as a mixing function in the L64 and L128 generators which doesn't seem to be correct. That should probably read `mixLea64` ------------- PR: https://git.openjdk.java.net/jdk/pull/1292