Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v42]
On Tue, 14 Sep 2021 18:58:16 GMT, stefan-zobel wrote: > 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` See https://bugs.openjdk.java.net/browse/JDK-8272866 - PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v42]
On Mon, 5 Apr 2021 14:20:56 GMT, Jim Laskey 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 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 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v42]
> 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 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 category) @implNote was out of date - Clarify all() - ... and 68 more: https://git.openjdk.java.net/jdk/compare/a8005efd...ffd982b0 - Changes: https://git.openjdk.java.net/jdk/pull/1292/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1292=41 Stats: 12973 lines in 27 files changed: 11202 ins; 1590 del; 181 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v41]
> 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: Fix NotCompliantCauseTest to not rely on Random not being a bean - Changes: - all: https://git.openjdk.java.net/jdk/pull/1292/files - new: https://git.openjdk.java.net/jdk/pull/1292/files/5a23b4f1..c5fb77f9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1292=40 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1292=39-40 Stats: 5 lines in 1 file changed: 0 ins; 2 del; 3 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v40]
> 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 with a new target base due to a merge or a rebase. The pull request now contains 76 commits: - 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 category) @implNote was out of date - Clarify all() - Cleaned up ints(), longs(), doubles() - Merge branch 'master' into 8248862 - ... and 66 more: https://git.openjdk.java.net/jdk/compare/011f6d13...5a23b4f1 - Changes: https://git.openjdk.java.net/jdk/pull/1292/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1292=39 Stats: 12968 lines in 26 files changed: 11202 ins; 1588 del; 178 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v39]
> 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: Correct return type of RandomGeneratorFactory.of - Changes: - all: https://git.openjdk.java.net/jdk/pull/1292/files - new: https://git.openjdk.java.net/jdk/pull/1292/files/3de42645..0aa49eda Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1292=38 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1292=37-38 Stats: 6 lines in 1 file changed: 3 ins; 0 del; 3 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v38]
> 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 with a new target base due to a merge or a rebase. The pull request now contains 74 commits: - Merge branch 'master' into 8248862 - CSR review revisions - CSR review updates - Removed @since from nextDouble ThreadLocalRandom - RandomGeneratorFactory::all(Class category) @implNote was out of date - Clarify all() - Cleaned up ints(), longs(), doubles() - Merge branch 'master' into 8248862 - Review revisions - Missing @since - ... and 64 more: https://git.openjdk.java.net/jdk/compare/f3726a87...3de42645 - Changes: https://git.openjdk.java.net/jdk/pull/1292/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1292=37 Stats: 12965 lines in 26 files changed: 11199 ins; 1588 del; 178 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v31]
On Thu, 18 Mar 2021 12:57:16 GMT, Jim Laskey wrote:
>> 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.
isDeprecated was added
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v37]
On Fri, 26 Mar 2021 12:25:43 GMT, Jim Laskey 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: > > CSR review revisions Marked as reviewed by darcy (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v37]
> 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: CSR review revisions - Changes: - all: https://git.openjdk.java.net/jdk/pull/1292/files - new: https://git.openjdk.java.net/jdk/pull/1292/files/be9536ce..84cc93fa Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1292=36 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1292=35-36 Stats: 15 lines in 2 files changed: 4 ins; 6 del; 5 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v36]
> 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: CSR review updates - Changes: - all: https://git.openjdk.java.net/jdk/pull/1292/files - new: https://git.openjdk.java.net/jdk/pull/1292/files/22ea21d2..be9536ce Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1292=35 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1292=34-35 Stats: 12 lines in 2 files changed: 6 ins; 2 del; 4 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v35]
> 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: Removed @since from nextDouble ThreadLocalRandom - Changes: - all: https://git.openjdk.java.net/jdk/pull/1292/files - new: https://git.openjdk.java.net/jdk/pull/1292/files/4562dd13..22ea21d2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1292=34 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1292=33-34 Stats: 4 lines in 1 file changed: 0 ins; 4 del; 0 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v34]
> 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 two additional commits since the last revision: - RandomGeneratorFactory::all(Class category) @implNote was out of date - Clarify all() - Changes: - all: https://git.openjdk.java.net/jdk/pull/1292/files - new: https://git.openjdk.java.net/jdk/pull/1292/files/5e98d915..4562dd13 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1292=33 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1292=32-33 Stats: 16 lines in 2 files changed: 3 ins; 0 del; 13 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v32]
On Thu, 18 Mar 2021 21:43:16 GMT, Joe Darcy 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 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v33]
> 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: Cleaned up ints(), longs(), doubles() - Changes: - all: https://git.openjdk.java.net/jdk/pull/1292/files - new: https://git.openjdk.java.net/jdk/pull/1292/files/63094f9d..5e98d915 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1292=32 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1292=31-32 Stats: 782 lines in 4 files changed: 310 ins; 416 del; 56 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v32]
On Thu, 18 Mar 2021 15:08:56 GMT, Jim Laskey 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 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
Changes requested by darcy (Reviewer).
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.
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.
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.
src/java.base/share/classes/java/util/random/RandomGenerator.java line 1486:
> 1484: * ({@link Long#MAX_VALUE Long.MAX_VALUE}).
> 1485: */
> 1486: default Stream jumps(double
> distance) {
Suggest adding an Override annotation.
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.
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.
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v32]
> 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 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 - Changes: https://git.openjdk.java.net/jdk/pull/1292/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1292=31 Stats: 13653 lines in 26 files changed: 11537 ins; 1821 del; 295 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators
On Tue, 16 Mar 2021 20:37:57 GMT, Tommy Ettinger
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/github/tommyettinger/ds/support/LaserRandom.java)
> 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/java/net/adoptopenjdk/bumblebench/examples).
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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v31]
On Mon, 15 Mar 2021 23:02:33 GMT, Joe Darcy 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 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v31]
On Mon, 15 Mar 2021 23:07:53 GMT, Joe Darcy 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators
On Mon, 15 Mar 2021 20:45:30 GMT, Paul Sandoz wrote:
>> 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.
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/github/tommyettinger/ds/support/LaserRandom.java)
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/java/net/adoptopenjdk/bumblebench/examples).
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.
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v31]
On Mon, 15 Mar 2021 12:54:32 GMT, Jim Laskey 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:
>
> 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?
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v31]
On Mon, 15 Mar 2021 12:54:32 GMT, Jim Laskey 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:
>
> 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 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.
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators
On Fri, 26 Feb 2021 13:15:28 GMT, Rémi Forax
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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v31]
> 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: Missing @since - Changes: - all: https://git.openjdk.java.net/jdk/pull/1292/files - new: https://git.openjdk.java.net/jdk/pull/1292/files/ddb1a30c..9d05aa56 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1292=30 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1292=29-30 Stats: 2 lines in 1 file changed: 1 ins; 1 del; 0 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 18 Nov 2020 13:45:46 GMT, Jim Laskey wrote: >> Need rebase > > Created new PR because of forced push: > https://github.com/openjdk/jdk/pull/1292 LGTM - PR: https://git.openjdk.java.net/jdk/pull/1273
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v30]
> 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: Review revisions - Changes: - all: https://git.openjdk.java.net/jdk/pull/1292/files - new: https://git.openjdk.java.net/jdk/pull/1292/files/e5b87227..ddb1a30c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1292=29 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1292=28-29 Stats: 321 lines in 4 files changed: 79 ins; 203 del; 39 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v29]
> 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 with a new target base due to a merge or a rebase. The pull request now contains 63 commits: - Review requested changes - Merge branch 'master' into 8248862 - Remove conflicts - Use isAnnotationPresent - Introduce isDeprecated - Update javadoc - Merge branch 'master' into 8248862 - Adjust ThreadLocalRandom javadoc inheritence - L32X64StarStarRandom -> L32X64MixRandom - Various corrects - ... and 53 more: https://git.openjdk.java.net/jdk/compare/273f8bdf...e5b87227 - Changes: https://git.openjdk.java.net/jdk/pull/1292/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1292=28 Stats: 13827 lines in 26 files changed: 11646 ins; 1849 del; 332 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v28]
> 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: Use isAnnotationPresent - Changes: - all: https://git.openjdk.java.net/jdk/pull/1292/files - new: https://git.openjdk.java.net/jdk/pull/1292/files/7439c2ba..345a17cc Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1292=27 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1292=26-27 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v27]
> 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: Introduce isDeprecated - Changes: - all: https://git.openjdk.java.net/jdk/pull/1292/files - new: https://git.openjdk.java.net/jdk/pull/1292/files/99e92dd1..7439c2ba Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1292=26 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1292=25-26 Stats: 25 lines in 1 file changed: 23 ins; 0 del; 2 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v26]
> 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: Update javadoc - Changes: - all: https://git.openjdk.java.net/jdk/pull/1292/files - new: https://git.openjdk.java.net/jdk/pull/1292/files/b9094279..99e92dd1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1292=25 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1292=24-25 Stats: 460 lines in 7 files changed: 183 ins; 85 del; 192 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v25]
On Mon, 1 Mar 2021 15:12:46 GMT, Roger Riggs 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v25]
On Mon, 1 Mar 2021 13:23:48 GMT, Jim Laskey wrote:
>> src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java
>> line 232:
>>
>>> 230: Provider 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");
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.
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v25]
On Fri, 26 Feb 2021 21:32:12 GMT, Roger Riggs 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 jump 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 jump forward, by a moderate amount (ex.
* 264) 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
* leap forward, by a large amount (ex. 2128), 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 jump 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v25]
On Fri, 26 Feb 2021 21:25:38 GMT, Roger Riggs 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
> 240:
>
>> 238: * Returns a {@link RandomGenerator} that utilizes the {@code name}
>> 239: * algorithm.
>> 240: *
>
> In javadoc, this seems like is should read as: "utilizes the named algorithm".
> The use of the parameter name is unnecessary in this case because it is
> obvious and readability suffers as is.
Modified.
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v25]
On Fri, 26 Feb 2021 19:30:09 GMT, Roger Riggs 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 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 16:22:32 GMT, Jim Laskey wrote: >> src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java >> line 106: >> >>> 104: * Map of provider classes. >>> 105: */ >>> 106: private static volatile Map>> 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v25]
On Tue, 23 Feb 2021 16:47:59 GMT, Jim Laskey 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 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 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"
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
240:
> 238: * Returns a {@link RandomGenerator} that utilizes the {@code name}
> 239: * algorithm.
> 240: *
In javadoc, this seems like is should read as: "utilizes the named algorithm".
The use of the parameter name is unnecessary in this case because it is obvious
and readability suffers as is.
src/java.base/share/classes/java/util/random/RandomGenerator.java line 1313:
> 1311: * furthermore can easily jump 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.
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".
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.
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.
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators
On Fri, 26 Feb 2021 13:13:19 GMT, Jim Laskey wrote: >> Stayin' alive > > 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 - PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators
On Mon, 4 Jan 2021 13:54:14 GMT, Jim Laskey 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 > > Stayin' alive Looking for some final code reviews. - PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v25]
> 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 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 - Changes: https://git.openjdk.java.net/jdk/pull/1292/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1292=24 Stats: 13693 lines in 26 files changed: 11542 ins; 2066 del; 85 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v24]
> 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: L32X64StarStarRandom -> L32X64MixRandom - Changes: - all: https://git.openjdk.java.net/jdk/pull/1292/files - new: https://git.openjdk.java.net/jdk/pull/1292/files/eeab6454..58a05f4c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1292=23 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1292=22-23 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v23]
> 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: Various corrects - Changes: - all: https://git.openjdk.java.net/jdk/pull/1292/files - new: https://git.openjdk.java.net/jdk/pull/1292/files/61f5d700..eeab6454 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1292=22 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1292=21-22 Stats: 34 lines in 4 files changed: 20 ins; 0 del; 14 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v22]
> 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: Revised javadoc per CSR reviews - Changes: - all: https://git.openjdk.java.net/jdk/pull/1292/files - new: https://git.openjdk.java.net/jdk/pull/1292/files/cfaf7cef..61f5d700 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1292=21 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1292=20-21 Stats: 31 lines in 3 files changed: 28 ins; 0 del; 3 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v21]
On Sat, 20 Feb 2021 00:09:51 GMT, Joe Darcy 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: * Random Number Generator Algorithms Available >> in Java SE > > 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v21]
On Fri, 19 Feb 2021 12:48:05 GMT, Jim Laskey 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: > > Remove tabs from random/package-info.java src/java.base/share/classes/java/util/random/package-info.java line 193: > 191: * > 192: * > 193: * Random Number Generator Algorithms Available > in Java SE 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? - PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 13:55:32 GMT, Jim Laskey 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v21]
> 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: Remove tabs from random/package-info.java - Changes: - all: https://git.openjdk.java.net/jdk/pull/1292/files - new: https://git.openjdk.java.net/jdk/pull/1292/files/9861b4e4..cfaf7cef Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1292=20 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1292=19-20 Stats: 102 lines in 1 file changed: 0 ins; 0 del; 102 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v20]
> 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: Correct copyright notice. - Changes: - all: https://git.openjdk.java.net/jdk/pull/1292/files - new: https://git.openjdk.java.net/jdk/pull/1292/files/7d0ebfc9..9861b4e4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1292=19 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1292=18-19 Stats: 28 lines in 1 file changed: 6 ins; 0 del; 22 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v19]
> 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 with a new target base due to a merge or a rebase. The pull request now contains 50 commits: - Merge branch 'master' into 8248862 - Update tests for RandomGeneratorFactory.all() - Merge branch 'master' into 8248862 - Flatten out use of all() - Add review edits - Added table of available algorithms. - Merge branch 'master' into 8248862 - Update SplittableRandom to remove unnecessary overrides - Updated API based on CSR review. - Update package info to credit LMX algorithm - ... and 40 more: https://git.openjdk.java.net/jdk/compare/f94a8452...7d0ebfc9 - Changes: https://git.openjdk.java.net/jdk/pull/1292/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1292=18 Stats: 13341 lines in 26 files changed: 11195 ins; 2055 del; 91 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v17]
On Sat, 13 Feb 2021 21:30:12 GMT, Andrey Turbanov
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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v18]
> 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 with a new target base due to a merge or a rebase. The pull request now contains 48 commits: - Merge branch 'master' into 8248862 - Flatten out use of all() - Add review edits - Added table of available algorithms. - Merge branch 'master' into 8248862 - Update SplittableRandom to remove unnecessary overrides - Updated API based on CSR review. - Update package info to credit LMX algorithm - Merge branch 'master' into 8248862 - Correct range used by nextBytes - ... and 38 more: https://git.openjdk.java.net/jdk/compare/05301f5f...16f066fe - Changes: https://git.openjdk.java.net/jdk/pull/1292/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1292=17 Stats: 13341 lines in 26 files changed: 11195 ins; 2055 del; 91 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v17]
On Tue, 16 Feb 2021 15:54:08 GMT, Rémi Forax 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v17]
On Tue, 16 Feb 2021 14:03:56 GMT, Jim Laskey 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v17]
On Sat, 13 Feb 2021 15:03:53 GMT, Andrey Turbanov
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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v17]
On Thu, 11 Feb 2021 16:02:03 GMT, Jim Laskey 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/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
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?
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
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
`||`
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v17]
On Thu, 11 Feb 2021 16:02:03 GMT, Jim Laskey 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/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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v17]
On Thu, 11 Feb 2021 16:02:03 GMT, Jim Laskey 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v17]
> 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=1292=16 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1292=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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v16]
> 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 with a new target base due to a merge or a rebase. The pull request now contains 44 commits: - Merge branch 'master' into 8248862 - Update SplittableRandom to remove unnecessary overrides - Updated API based on CSR review. - Update package info to credit LMX algorithm - Merge branch 'master' into 8248862 - Correct range used by nextBytes - Merge branch 'master' into 8248862 - Use annotation for properties. Add getDefault(). - Merge branch 'master' into 8248862 - Introduce RandomGeneratorProperties annotation - ... and 34 more: https://git.openjdk.java.net/jdk/compare/83357b11...1c17ad35 - Changes: https://git.openjdk.java.net/jdk/pull/1292/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1292=15 Stats: 13259 lines in 26 files changed: 9 ins; 2050 del; 90 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v15]
> 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 two additional commits since the last revision: - Update SplittableRandom to remove unnecessary overrides - Updated API based on CSR review. - Changes: - all: https://git.openjdk.java.net/jdk/pull/1292/files - new: https://git.openjdk.java.net/jdk/pull/1292/files/38369702..96f98765 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1292=14 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1292=13-14 Stats: 599 lines in 8 files changed: 311 ins; 253 del; 35 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v14]
On Fri, 29 Jan 2021 00:15:16 GMT, Mark Reinhold 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v14]
On Mon, 18 Jan 2021 16:45:00 GMT, Jim Laskey 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:
>
> 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.
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v14]
> 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: Update package info to credit LMX algorithm - Changes: - all: https://git.openjdk.java.net/jdk/pull/1292/files - new: https://git.openjdk.java.net/jdk/pull/1292/files/772abef6..38369702 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1292=13 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1292=12-13 Stats: 15 lines in 1 file changed: 15 ins; 0 del; 0 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v13]
> 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 with a new target base due to a merge or a rebase. The pull request now contains 40 commits: - Merge branch 'master' into 8248862 - Correct range used by nextBytes - Merge branch 'master' into 8248862 - Use annotation for properties. Add getDefault(). - Merge branch 'master' into 8248862 - Introduce RandomGeneratorProperties annotation - 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 - ... and 30 more: https://git.openjdk.java.net/jdk/compare/061ffc47...772abef6 - Changes: https://git.openjdk.java.net/jdk/pull/1292/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1292=12 Stats: 13205 lines in 26 files changed: 11043 ins; 2046 del; 116 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v12]
On Wed, 6 Jan 2021 16:09:25 GMT, Brett Okken 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v12]
On Wed, 6 Jan 2021 15:36:21 GMT, Jim Laskey 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: > > 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? - PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 6 Jan 2021 15:31:32 GMT, Jim Laskey wrote: >> I kind of like the idea - not sure how expressive a BigInteger string is >> though. I might be able to express as >> BigInteger.ONE.shiftLeft(i).subtract(j).shiftLeft(k). Will ponder. > > Done Also added getDefault and getDefaultFactory to RandomGenerato. - PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 14:07:04 GMT, Rémi Forax 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 > 335: > >> 333: ctorBytes = tmpCtorBytes; >> 334: ctorLong = tmpCtorLong; >> 335: ctor = tmpCtor; > > This one is a volatile write, so the idea is that ctorBytes and ctorLong does > not need to be volatile too, which i think is not true if there is a code > somewhere that uses ctorBytes or ctorLong without reading ctor. > This code is too smart for me, i'm pretty sure it is wrong too on non TSO cpu. The 2 uses call ensureConstructors, which calls this method, which reads ctor. The memory model guarantees this to be safe, even on non tso hardware. https://shipilev.net/blog/2016/close-encounters-of-jmm-kind/#pitfall-avoiding - PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v9]
On Tue, 5 Jan 2021 02:43:08 GMT, Brett Okken 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 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) Thanks. Will update. - PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Thu, 26 Nov 2020 15:41:16 GMT, Jim Laskey 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 > BigInteger.ONE.shiftLeft(i).subtract(j).shiftLeft(k). Will ponder. Done - PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v12]
> 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: Correct range used by nextBytes - Changes: - all: https://git.openjdk.java.net/jdk/pull/1292/files - new: https://git.openjdk.java.net/jdk/pull/1292/files/cb78fa11..f76fa7a7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1292=11 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1292=10-11 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v9]
On Mon, 4 Jan 2021 19:52:18 GMT, Jim Laskey 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 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v11]
> 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 with a new target base due to a merge or a rebase. The pull request now contains 38 commits: - Merge branch 'master' into 8248862 - Use annotation for properties. Add getDefault(). - Merge branch 'master' into 8248862 - Introduce RandomGeneratorProperties annotation - 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 - ... and 28 more: https://git.openjdk.java.net/jdk/compare/7e01bc96...cb78fa11 - Changes: https://git.openjdk.java.net/jdk/pull/1292/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1292=10 Stats: 13205 lines in 26 files changed: 11043 ins; 2046 del; 116 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v10]
> 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 with a new target base due to a merge or a rebase. The pull request now contains 37 commits: - Use annotation for properties. Add getDefault(). - Merge branch 'master' into 8248862 - Introduce RandomGeneratorProperties annotation - 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 - ... and 27 more: https://git.openjdk.java.net/jdk/compare/a6c08813...da9fec11 - Changes: https://git.openjdk.java.net/jdk/pull/1292/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1292=09 Stats: 13205 lines in 26 files changed: 11043 ins; 2046 del; 116 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v9]
> 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 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 - Changes: https://git.openjdk.java.net/jdk/pull/1292/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1292=08 Stats: 13525 lines in 26 files changed: 11366 ins; 2040 del; 119 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators
On Wed, 18 Nov 2020 13:45:12 GMT, Jim Laskey 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 Stayin' alive - PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v8]
> 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: 8248862: Implement Enhanced Pseudo-Random Number Generators Added coverage testing - Changes: - all: https://git.openjdk.java.net/jdk/pull/1292/files - new: https://git.openjdk.java.net/jdk/pull/1292/files/a8cc0795..f7b697ec Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1292=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1292=06-07 Stats: 168 lines in 3 files changed: 166 ins; 2 del; 0 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v7]
> 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: 8248862: Implement Enhanced Pseudo-Random Number Generators Cleanups from Chris H. - Changes: - all: https://git.openjdk.java.net/jdk/pull/1292/files - new: https://git.openjdk.java.net/jdk/pull/1292/files/ca29ff74..a8cc0795 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1292=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1292=05-06 Stats: 13 lines in 1 file changed: 4 ins; 3 del; 6 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v6]
> 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: 8248862: Implement Enhanced Pseudo-Random Number Generators Propagate exception - Changes: - all: https://git.openjdk.java.net/jdk/pull/1292/files - new: https://git.openjdk.java.net/jdk/pull/1292/files/a6851940..ca29ff74 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1292=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1292=04-05 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 14:16:20 GMT, Rémi Forax 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 > 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 BigInteger.ONE.shiftLeft(i).subtract(j).shiftLeft(k). Will ponder. - PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 14:10:17 GMT, Rémi Forax
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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v5]
> 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: 8248862: Implement Enhanced Pseudo-Random Number Generators Override AbstractSplittableGenerator - Changes: - all: https://git.openjdk.java.net/jdk/pull/1292/files - new: https://git.openjdk.java.net/jdk/pull/1292/files/ee8f87c3..a6851940 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1292=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1292=03-04 Stats: 165 lines in 3 files changed: 164 ins; 0 del; 1 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 19:48:32 GMT, Jim Laskey wrote: >> 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 It's because you have added reverse() as a postfix operation so the inference can not flow backward as it should, using Collections.reverseOrder() should fix that `.sorted(Collections.reverseOrder(Comparator.comparingInt(RandomGeneratorFactory::stateBits)))` using some import statics for reverseOrder and comparintInt make the code readable but given it's in the doc, we are out of luck here. - PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 16:26:12 GMT, Rémi Forax wrote: >> Not sure that >> `.sorted(Comparator.comparingInt(RandomGeneratorFactory::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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v4]
> 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 two additional commits since the last revision: - 8248862: Implement Enhanced Pseudo-Random Number Generators Fix extends - 8248862: Implement Enhanced Pseudo-Random Number Generators Use Map.of instead of Map.ofEntries - Changes: - all: https://git.openjdk.java.net/jdk/pull/1292/files - new: https://git.openjdk.java.net/jdk/pull/1292/files/802fa530..ee8f87c3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1292=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1292=02-03 Stats: 169 lines in 15 files changed: 23 ins; 17 del; 129 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 13:54:47 GMT, Rémi Forax
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> fm =
>> getFactoryMap();
>> 235: Provider 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
>> Stream> all(Class
>> category) {
>
> this signature is weird, T is not used in the parameter, so in case return
> any type of Stream> from a type POV, should it be
> ` Stream> 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 of(String name, Class
>> category)
>
> Same issue as above, T is not used as a constraint
agree
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 13:45:46 GMT, Rémi Forax 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 16:22:34 GMT, Jim Laskey wrote:
>> 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
If the second `RandomGeneratorFactory.factoryMap` return a non null value ...
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 15:59:01 GMT, Jim Laskey wrote:
>> src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java
>> line 88:
>>
>>> 86: * {@code
>>> 87: * RandomGeneratorFactory 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::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())`
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 13:38:59 GMT, Rémi Forax
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> 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 15:43:39 GMT, Jim Laskey wrote: >> will investigate > > Needed to use ThreadLocalRandomProxy.proxy otherwise a cast would be required. yes, right ! - PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 13:37:02 GMT, Rémi Forax
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: * {@code
>> 87: * RandomGeneratorFactory 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::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 properties =
>> null;
>
> `= null` is useless here
agree
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 13:55:52 GMT, Jim Laskey 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 13:09:03 GMT, Jim Laskey 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: > > 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
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 13:09:03 GMT, Jim Laskey 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:
>
> 8248862: Implement Enhanced Pseudo-Random Number Generators
>
> Changes to RandomGeneratorFactory requested by @PaulSandoz
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
335:
> 333: ctorBytes = tmpCtorBytes;
> 334: ctorLong = tmpCtorLong;
> 335: ctor = tmpCtor;
This one is a volatile write, so the idea is that ctorBytes and ctorLong does
not need to be volatile too, which i think is not true if there is a code
somewhere that uses ctorBytes or ctorLong without reading ctor.
This code is too smart for me, i'm pretty sure it is wrong too on non TSO cpu.
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 ...
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
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
517:
> 515: ensureConstructors();
> 516: return ctorBytes.newInstance((Object)seed);
> 517: } catch (Exception ex) {
same as above
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 13:09:03 GMT, Jim Laskey 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:
>
> 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> fm =
> getFactoryMap();
> 235: Provider provider =
> fm.get(name.toUpperCase());
again use of toUpperCase() instead of toUpperCase(Locale)
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
250:
> 248: * @return Stream of matching Providers.
> 249: */
> 250: static Stream>
> all(Class category) {
this signature is weird, T is not used in the parameter, so in case return any
type of Stream> from a type POV, should it be
` Stream> all(Class category)` instead ?
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 of(String name, Class
> category)
Same issue as above, T is not used as a constraint
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
288:
> 286: * @throws IllegalArgumentException when either the name or category
> is null
> 287: */
> 288: static RandomGeneratorFactory
> factoryOf(String name, Class category)
Same as above
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
300:
> 298: */
> 299: @SuppressWarnings("unchecked")
> 300: private void getConstructors(Class
> randomGeneratorClass) {
method name get but do side effects
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 13:31:52 GMT, Rémi Forax
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/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.
> 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
agree
> 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
> 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
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 13:24:37 GMT, Rémi Forax
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/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
(instance?) agree
> 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
I had assumed Map.ofEntries() was more efficient but it seems it in turn uses
MapN as well. Will change these cases.
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 13:09:03 GMT, Jim Laskey 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:
>
> 8248862: Implement Enhanced Pseudo-Random Number Generators
>
> Changes to RandomGeneratorFactory requested by @PaulSandoz
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.
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 !
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
175:
> 173: if (props == null) {
> 174: synchronized (provider) {
> 175: if (properties == null) {
same issue with the double check locking
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
177:
> 175: if (properties == null) {
> 176: try {
> 177: Method getProperties =
> provider.type().getDeclaredMethod("getProperties");
Is it not a better way than using reflection here ?
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
180:
> 178:
> PrivilegedExceptionAction> getAction =
> () -> {
> 179: getProperties.setAccessible(true);
> 180: return (Map Object>)getProperties.invoke(null);
Given that we have no control over the fact that the method properties()
doesn't return a Map of something else, this unsafe cast seems dangerous (from
the type system POV).
Having a type representing a reified version of the Map may be a better idea
-
PR: https://git.openjdk.java.net/jdk/pull/1292
