Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8248862: Implement Enhanced Pseudo-Random Number Generators #1292

Closed
wants to merge 78 commits into from

Conversation

@JimLaskey
Copy link
Member

@JimLaskey JimLaskey commented Nov 18, 2020

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: #1273


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8248862: Implement Enhanced Pseudo-Random Number Generators

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/1292/head:pull/1292
$ git checkout pull/1292

Update a local copy of the PR:
$ git checkout pull/1292
$ git pull https://git.openjdk.java.net/jdk pull/1292/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1292

View PR using the GUI difftool:
$ git pr show -t 1292

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/1292.diff

JimLaskey added 25 commits Oct 9, 2020
Remove non-ASCII character from RandomTestBsi1999 test.
New test test/jdk/java/util/Random/RandomCanaryPi.java
Remove L32X64StarStarRandom and MRG32k3a
Remove export of additional random number generators
Move RandomSupport to java.util.random.internal.
Remove PreviewFeature.Feature.RANDOM_NUMBERS
Tighten up RandomGeneratorFactory parameter
Move RandomSupport to jdk.internal.util.random
remove RandomGeneratorProperty from API
Updated documentation for RandomGeneratorFactory.
Updated RandomGeneratorFactory javadoc.
Update package-info.java
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 18, 2020

👋 Welcome back jlaskey! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Nov 18, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 18, 2020

@JimLaskey The following labels will be automatically applied to this pull request:

  • core-libs
  • security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the security label Nov 18, 2020
@PaulSandoz
Copy link
Member

@PaulSandoz PaulSandoz commented Mar 15, 2021

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.

@tommyettinger
Copy link

@tommyettinger tommyettinger commented Mar 16, 2021

Has anyone here benchmarked this? I've run BumbleBench benchmarks (one of the AdoptOpenJDK team's tools, available here) and I'm skeptical of the original claims in JEP 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 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. 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.

@JimLaskey
Copy link
Member Author

@JimLaskey JimLaskey commented Mar 18, 2021

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.

Added the filter.

@JimLaskey
Copy link
Member Author

@JimLaskey JimLaskey commented Mar 18, 2021

Has anyone here benchmarked this? I've run BumbleBench benchmarks (one of the AdoptOpenJDK team's tools, available here) and I'm skeptical of the original claims in JEP 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 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. 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

JimLaskey added 2 commits Mar 18, 2021
@openjdk openjdk bot removed the csr label Mar 26, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 26, 2021

@JimLaskey This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8248862: Implement Enhanced Pseudo-Random Number Generators

Reviewed-by: darcy

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 1 new commit pushed to the master branch:

  • 39719da: 8253266: JList and JTable constructors should clear OPAQUE_SET before calling updateUI

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Mar 26, 2021
@JimLaskey
Copy link
Member Author

@JimLaskey JimLaskey commented Apr 5, 2021

/integrate

@openjdk openjdk bot closed this Apr 5, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Apr 5, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 5, 2021

@JimLaskey Since your change was applied there has been 1 commit pushed to the master branch:

  • 39719da: 8253266: JList and JTable constructors should clear OPAQUE_SET before calling updateUI

Your commit was automatically rebased without conflicts.

Pushed as commit a0ec2cb.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment