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 [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 [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 [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 [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 [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: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 [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 [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 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: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 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: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: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: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
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
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
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
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
-
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/RandomGenerator.java line 745: > 743: * if the period is unknown. > 744: */ > 745: BigInteger UNKNOWN_PERIOD = BigInteger.ZERO; move those 3 values into RandomGeneratorFactory ? - PR: https://git.openjdk.java.net/jdk/pull/1292
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/security/SecureRandom.java line 223:
> 221: Map.entry(RandomGeneratorProperty.IS_HARDWARE, false)
> 222: );
> 223: }
Using Map.of() instead of Map.ofEntries() should simplify the code
src/java.base/share/classes/java/util/Random.java line 129:
> 127: Map.entry(RandomGeneratorProperty.IS_HARDWARE, false)
> 128: );
> 129: }
Using Map.of() should simplify the code
src/java.base/share/classes/java/util/SplittableRandom.java line 181:
> 179: Map.entry(RandomGeneratorProperty.IS_HARDWARE, false)
> 180: );
> 181: }
Using Map.of() should simplify the code
src/java.base/share/classes/java/util/concurrent/ThreadLocalRandom.java line
169:
> 167: Map.entry(RandomGeneratorProperty.IS_STOCHASTIC, false),
> 168: Map.entry(RandomGeneratorProperty.IS_HARDWARE, false)
> 169: );
Using Map.of() should simplify the code
src/java.base/share/classes/java/util/concurrent/ThreadLocalRandom.java line
433:
> 431: private static final class ThreadLocalRandomProxy extends Random {
> 432: @java.io.Serial
> 433: static final long serialVersionUID = 0L;
should be private
src/java.base/share/classes/java/util/concurrent/ThreadLocalRandom.java line
444:
> 442: }
> 443:
> 444: private static final AbstractSpliteratorGenerator proxy = new
> ThreadLocalRandomProxy();
should be declared inside ThreadLocalRandomProxy, so the value is only
initialized when proxy() is called
src/java.base/share/classes/java/util/concurrent/ThreadLocalRandom.java line
453:
> 451: * @return a {@code RandomGenerator} object that uses {@code
> ThreadLocalRandom}
> 452: */
> 453: public static RandomGenerator proxy() {
proxy is a generic name, is there a better name here ?
src/java.base/share/classes/java/util/concurrent/ThreadLocalRandom.java line
459:
> 457: // Methods required by class AbstractSpliteratorGenerator
> 458: public Spliterator.OfInt makeIntsSpliterator(long index, long fence,
> int origin, int bound) {
> 459: return new RandomIntsSpliterator(proxy, index, fence, origin,
> bound);
should use proxy()
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
> 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 - Changes: - all: https://git.openjdk.java.net/jdk/pull/1292/files - new: https://git.openjdk.java.net/jdk/pull/1292/files/9d6d1a94..802fa530 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1292=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1292=01-02 Stats: 54 lines in 1 file changed: 23 ins; 7 del; 24 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 00:30:53 GMT, Paul Sandoz 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 40 commits:
>>
>> - Merge branch 'master' into 8248862
>> - 8248862: Implement Enhanced Pseudo-Random Number Generators
>>
>>Update package-info.java
>> - 8248862: Implement Enhanced Pseudo-Random Number Generators
>>
>>Updated RandomGeneratorFactory javadoc.
>> - 8248862: Implement Enhanced Pseudo-Random Number Generators
>>
>>Updated documentation for RandomGeneratorFactory.
>> - Merge branch 'master' into 8248862
>> - Merge branch 'master' into 8248862
>> - 8248862: Implement Enhanced Pseudo-Random Number Generators
>>
>>Move RandomGeneratorProperty
>> - Merge branch 'master' into 8248862
>> - 8248862: Implement Enhanced Pseudo-Random Number Generators
>>
>>Clear up javadoc
>> - 8248862; Implement Enhanced Pseudo-Random Number Generators
>>
>>remove RandomGeneratorProperty from API
>> - ... and 30 more:
>> https://git.openjdk.java.net/jdk/compare/f7517386...6fe94c68
>
> src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
> 148:
>
>> 146: */
>> 147: private static Map>
>> getFactoryMap() {
>> 148: if (factoryMap == null) {
>
> `factoryMap` needs to be marked volatile when using the double checked
> locking idiom.
fixing
> src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
> 320:
>
>> 318: }
>> 319: }
>> 320: }
>
> Add an `assert` statement that `ctor`, `ctorLong` and `ctorBytes` are all
> non-null?
Only `ctor` is required but yes.
> src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
> 331:
>
>> 329: */
>> 330: private void ensureConstructors() {
>> 331: if (ctor == null) {
>
> This check occurs outside of the synchronized block, field may need to be
> marked volatile. Unsure about the other dependent fields. Might need to store
> values from loop in `getConstructors` in locals and then assign in
> appropriate order, assigning the volatile field last.
okay
-
PR: https://git.openjdk.java.net/jdk/pull/1273
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 18 Nov 2020 00:29:36 GMT, Paul Sandoz 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 40 commits:
>>
>> - Merge branch 'master' into 8248862
>> - 8248862: Implement Enhanced Pseudo-Random Number Generators
>>
>>Update package-info.java
>> - 8248862: Implement Enhanced Pseudo-Random Number Generators
>>
>>Updated RandomGeneratorFactory javadoc.
>> - 8248862: Implement Enhanced Pseudo-Random Number Generators
>>
>>Updated documentation for RandomGeneratorFactory.
>> - Merge branch 'master' into 8248862
>> - Merge branch 'master' into 8248862
>> - 8248862: Implement Enhanced Pseudo-Random Number Generators
>>
>>Move RandomGeneratorProperty
>> - Merge branch 'master' into 8248862
>> - 8248862: Implement Enhanced Pseudo-Random Number Generators
>>
>>Clear up javadoc
>> - 8248862; Implement Enhanced Pseudo-Random Number Generators
>>
>>remove RandomGeneratorProperty from API
>> - ... and 30 more:
>> https://git.openjdk.java.net/jdk/compare/f7517386...6fe94c68
>
> src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
> 173:
>
>> 171: @SuppressWarnings("unchecked")
>> 172: private Map getProperties() {
>> 173: if (properties == null) {
>
> `properties` needs to be marked volatile, and it needs to be assigned at line
> 182 or line 184.
One of them foggy days.
-
PR: https://git.openjdk.java.net/jdk/pull/1273
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Mon, 23 Nov 2020 14:57:59 GMT, Jim Laskey wrote:
>> src/java.base/share/classes/module-info.java line 250:
>>
>>> 248: exports jdk.internal.util.xml.impl to
>>> 249: jdk.jfr;
>>> 250: exports jdk.internal.util.random;
>>
>> Unqualified export, should this be `to jdk.random`?
>
> I guess you are right. Until we have a defined SPI we should restrict.
On the other hand:
public class Random extends AbstractSpliteratorGenerator
^
error: warnings found and -Werror specified
public final class SplittableRandom extends AbstractSplittableGenerator {
-
PR: https://git.openjdk.java.net/jdk/pull/1273
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Tue, 17 Nov 2020 23:46:12 GMT, Paul Sandoz 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 40 commits:
>>
>> - Merge branch 'master' into 8248862
>> - 8248862: Implement Enhanced Pseudo-Random Number Generators
>>
>>Update package-info.java
>> - 8248862: Implement Enhanced Pseudo-Random Number Generators
>>
>>Updated RandomGeneratorFactory javadoc.
>> - 8248862: Implement Enhanced Pseudo-Random Number Generators
>>
>>Updated documentation for RandomGeneratorFactory.
>> - Merge branch 'master' into 8248862
>> - Merge branch 'master' into 8248862
>> - 8248862: Implement Enhanced Pseudo-Random Number Generators
>>
>>Move RandomGeneratorProperty
>> - Merge branch 'master' into 8248862
>> - 8248862: Implement Enhanced Pseudo-Random Number Generators
>>
>>Clear up javadoc
>> - 8248862; Implement Enhanced Pseudo-Random Number Generators
>>
>>remove RandomGeneratorProperty from API
>> - ... and 30 more:
>> https://git.openjdk.java.net/jdk/compare/f7517386...6fe94c68
>
> src/jdk.random/share/classes/module-info.java line 53:
>
>> 51: uses RandomSupport;
>> 52:
>> 53: exports jdk.random to
>
> Why is this needed?
Removing
> src/java.base/share/classes/java/util/random/package-info.java line 50:
>
>> 48: * given its name.
>> 49: *
>> 50: * The principal supporting class is {@link RandomGenertatorFactor}.
>> This
>
> s/RandomGenertatorFactor/RandomGenertatorFactory
fixing
> src/java.base/share/classes/java/util/random/package-info.java line 140:
>
>> 138: *
>> 139: * For applications with no special requirements,
>> 140: * "L64X128MixRandom" has a good balance among speed, space,
>
> The documentation assumes that the `jdk.random` module is present in the JDK
> image. Perhaps we need to spit the specifics to `jdk.random`?
But jdk.random isn't really a public API.
> src/java.base/share/classes/jdk/internal/util/random/RandomSupport.java line
> 1211:
>
>> 1209: Udiff = -Udiff;
>> 1210: U2 = U1;
>> 1211: U1 -= Udiff;
>
> Updated `U1` never used (recommend running the code through a checker e.g.
> use IntelliJ)
I noticed that before. I think it's a symmetry thing - will check with Guy.
> src/java.base/share/classes/jdk/internal/util/random/RandomSupport.java line
> 1157:
>
>> 1155: /*
>> 1156: * The tables themselves, as well as a number of associated
>> parameters, are
>> 1157: * defined in class java.util.DoubleZigguratTables, which is
>> automatically
>
> `DoubleZigguratTables` is an inner class of `RandomSupport`
Late change fixing.
> src/java.base/share/classes/jdk/internal/util/random/RandomSupport.java line
> 2895:
>
>> 2893: * distribution: 0.0330
>> 2894: */
>> 2895: static class DoubleZigguratTables {
>
> make `final`
fixing
> src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
> 167:
>
>> 165: * Return the properties map for the specified provider.
>> 166: *
>> 167: * @param provider provider to locate.
>
> Method has no such parameter
Fixing
-
PR: https://git.openjdk.java.net/jdk/pull/1273
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Tue, 17 Nov 2020 21:22:28 GMT, Paul Sandoz 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 40 commits:
>>
>> - Merge branch 'master' into 8248862
>> - 8248862: Implement Enhanced Pseudo-Random Number Generators
>>
>>Update package-info.java
>> - 8248862: Implement Enhanced Pseudo-Random Number Generators
>>
>>Updated RandomGeneratorFactory javadoc.
>> - 8248862: Implement Enhanced Pseudo-Random Number Generators
>>
>>Updated documentation for RandomGeneratorFactory.
>> - Merge branch 'master' into 8248862
>> - Merge branch 'master' into 8248862
>> - 8248862: Implement Enhanced Pseudo-Random Number Generators
>>
>>Move RandomGeneratorProperty
>> - Merge branch 'master' into 8248862
>> - 8248862: Implement Enhanced Pseudo-Random Number Generators
>>
>>Clear up javadoc
>> - 8248862; Implement Enhanced Pseudo-Random Number Generators
>>
>>remove RandomGeneratorProperty from API
>> - ... and 30 more:
>> https://git.openjdk.java.net/jdk/compare/f7517386...6fe94c68
>
> src/java.base/share/classes/java/util/Random.java line 592:
>
>> 590:
>> 591: @Override
>> 592: public Spliterator.OfInt makeIntsSpliterator(long index, long
>> fence, int origin, int bound) {
>
> Unsure if this and the other two methods are intended to be public or not,
> since they are at the end of the class and override methods of a module
> private class. In principle there is nothing wrong with such `Spliterator`
> factories, but wonder if they are really needed given the `Stream` returning
> methods. The arrangement of classes makes it awkward to hide these methods.
Re the properties general comment: I moved properties to RandomSupport based on
the notion that the SPI work with come later.
Re makeIntsSpliterator: These methods aren't exposed in the java.util.Random
API I guess no harm done. The only solution I can think of is to create an
intermediate implementor, but that leaves the methods exposed as well.
> src/java.base/share/classes/java/util/SplittableRandom.java line 171:
>
>> 169: * RandomGenerator properties.
>> 170: */
>> 171: static Map getProperties() {
>
> With records exiting preview in 16 this map of properties could i think be
> represented as a record instance, with better type safety, where
> `RandomSupport.RandomGeneratorProperty` enum values become typed fields
> declared on the record class. Something to consider after integration perhaps?
Yes.
> src/java.base/share/classes/java/util/SplittableRandom.java line 211:
>
>> 209: *
>> http://zimbry.blogspot.com/2011/09/better-bit-mixing-improving-on.html
>> 210: */
>> 211: private static long mix64(long z) {
>
> Usages be replaced with calls to `RandomSupport.mixStafford13`?
We were careful to not change the sequences (from fixed seed) generated by
existing prngs. This was an edge case.
> src/java.base/share/classes/module-info.java line 250:
>
>> 248: exports jdk.internal.util.xml.impl to
>> 249: jdk.jfr;
>> 250: exports jdk.internal.util.random;
>
> Unqualified export, should this be `to jdk.random`?
I guess you are right. Until we have a defined SPI we should restrict.
-
PR: https://git.openjdk.java.net/jdk/pull/1273
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
[Sorry it took so long. Have been on break.] From Guy: Thanks for the forward. Here are my thoughts: Good question from Rémi. If we consider PRNGs to have started at about the time of von Neumann, circa 1946, then I would say that we have been inventing a new category about once every 25 years or so: jumpable, multi-level jumpable, cryptographically secure, splittable. Twenty years ago we would just have one or more levels of jumping/leaping. I think SecureRandom appeared in 2002 (in J2SE 1.4), and the first version of SplittableRandom was in 2014. So I could be wrong, but I really don’t expect to have to add any more interfaces in the next decade or two. I think we will get more benefit from the better type checking than we would get with optional methods. —Guy > On Nov 17, 2020, at 7:18 PM, Remi Forax wrote: > > An honest question, > why do we need so many interfaces for the different categories of > RandomGenerator ? > > My fear is that we are encoding the state of our knowledge of the different > kinds of random generators now so it will not be pretty in the future when > new categories of random generator are discovered/invented. > If we can take example of the past to predict the future, 20 years ago, what > should have been the hierarchy at that time. > Is it not reasonable to think that we will need new kinds of random generator > in the future ? > > I wonder if it's not better to have one interface and several optional > methods like we have with the collections, it means that we are loosing the > possibilities to precisely type a method that only works with a precise type > of generator but it will be more future proof. > > Rémi > > - Mail original - >> De: "Jim Laskey" >> À: "build-dev" , "core-libs-dev" >> , >> security-dev@openjdk.java.net >> Envoyé: Mardi 17 Novembre 2020 23:21:18 >> Objet: Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators >> [v3] > >>> This PR is to introduce a new random number API for the JDK. The primary >>> API is >>> found in RandomGenerator and RandomGeneratorFactory. Further description >>> can be >>> found in the JEP https://openjdk.java.net/jeps/356 . >> >> Jim Laskey has updated the pull request with a new target base due to a >> merge or >> a rebase. The pull request now contains 40 commits: >> >> - Merge branch 'master' into 8248862 >> - 8248862: Implement Enhanced Pseudo-Random Number Generators >> >> Update package-info.java >> - 8248862: Implement Enhanced Pseudo-Random Number Generators >> >> Updated RandomGeneratorFactory javadoc. >> - 8248862: Implement Enhanced Pseudo-Random Number Generators >> >> Updated documentation for RandomGeneratorFactory. >> - Merge branch 'master' into 8248862 >> - Merge branch 'master' into 8248862 >> - 8248862: Implement Enhanced Pseudo-Random Number Generators >> >> Move RandomGeneratorProperty >> - Merge branch 'master' into 8248862 >> - 8248862: Implement Enhanced Pseudo-Random Number Generators >> >> Clear up javadoc >> - 8248862; Implement Enhanced Pseudo-Random Number Generators >> >> remove RandomGeneratorProperty from API >> - ... and 30 more: >> https://git.openjdk.java.net/jdk/compare/f7517386...6fe94c68 >> >> - >> >> Changes: https://git.openjdk.java.net/jdk/pull/1273/files >> Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1273=02 >> Stats: 14891 lines in 31 files changed: 0 ins; 3704 del; 77 mod >> Patch: https://git.openjdk.java.net/jdk/pull/1273.diff >> Fetch: git fetch https://git.openjdk.java.net/jdk pull/1273/head:pull/1273 >> >> PR: https://git.openjdk.java.net/jdk/pull/1273
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
An honest question, why do we need so many interfaces for the different categories of RandomGenerator ? My fear is that we are encoding the state of our knowledge of the different kinds of random generators now so it will not be pretty in the future when new categories of random generator are discovered/invented. If we can take example of the past to predict the future, 20 years ago, what should have been the hierarchy at that time. Is it not reasonable to think that we will need new kinds of random generator in the future ? I wonder if it's not better to have one interface and several optional methods like we have with the collections, it means that we are loosing the possibilities to precisely type a method that only works with a precise type of generator but it will be more future proof. Rémi - Mail original - > De: "Jim Laskey" > À: "build-dev" , "core-libs-dev" > , > security-dev@openjdk.java.net > Envoyé: Mardi 17 Novembre 2020 23:21:18 > Objet: Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators > [v3] >> This PR is to introduce a new random number API for the JDK. The primary API >> is >> found in RandomGenerator and RandomGeneratorFactory. Further description can >> be >> found in the JEP https://openjdk.java.net/jeps/356 . > > Jim Laskey has updated the pull request with a new target base due to a merge > or > a rebase. The pull request now contains 40 commits: > > - Merge branch 'master' into 8248862 > - 8248862: Implement Enhanced Pseudo-Random Number Generators > > Update package-info.java > - 8248862: Implement Enhanced Pseudo-Random Number Generators > > Updated RandomGeneratorFactory javadoc. > - 8248862: Implement Enhanced Pseudo-Random Number Generators > > Updated documentation for RandomGeneratorFactory. > - Merge branch 'master' into 8248862 > - Merge branch 'master' into 8248862 > - 8248862: Implement Enhanced Pseudo-Random Number Generators > > Move RandomGeneratorProperty > - Merge branch 'master' into 8248862 > - 8248862: Implement Enhanced Pseudo-Random Number Generators > > Clear up javadoc > - 8248862; Implement Enhanced Pseudo-Random Number Generators > > remove RandomGeneratorProperty from API > - ... and 30 more: > https://git.openjdk.java.net/jdk/compare/f7517386...6fe94c68 > > - > > Changes: https://git.openjdk.java.net/jdk/pull/1273/files > Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1273=02 > Stats: 14891 lines in 31 files changed: 0 ins; 3704 del; 77 mod > Patch: https://git.openjdk.java.net/jdk/pull/1273.diff > Fetch: git fetch https://git.openjdk.java.net/jdk pull/1273/head:pull/1273 > > PR: https://git.openjdk.java.net/jdk/pull/1273
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 18 Nov 2020 13:18:30 GMT, Jim Laskey wrote: >> I am unsure if the intent is also to support external libraries providing >> `RandomGenerator` implementations. Currently there is an implicit contract >> for properties (reflectively invoking a method returning a map with a set of >> entries with known keys). Since the service provider implementation is the >> `RandomGenerator` itself, rather than `RandomGeneratorFactory` it is harder >> expose the meta-data with a clearer contract. > > Need rebase Created new PR because of forced push: https://github.com/openjdk/jdk/pull/1292 - PR: https://git.openjdk.java.net/jdk/pull/1273
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 18 Nov 2020 00:51:43 GMT, Paul Sandoz 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 40 commits: >> >> - Merge branch 'master' into 8248862 >> - 8248862: Implement Enhanced Pseudo-Random Number Generators >> >>Update package-info.java >> - 8248862: Implement Enhanced Pseudo-Random Number Generators >> >>Updated RandomGeneratorFactory javadoc. >> - 8248862: Implement Enhanced Pseudo-Random Number Generators >> >>Updated documentation for RandomGeneratorFactory. >> - Merge branch 'master' into 8248862 >> - Merge branch 'master' into 8248862 >> - 8248862: Implement Enhanced Pseudo-Random Number Generators >> >>Move RandomGeneratorProperty >> - Merge branch 'master' into 8248862 >> - 8248862: Implement Enhanced Pseudo-Random Number Generators >> >>Clear up javadoc >> - 8248862; Implement Enhanced Pseudo-Random Number Generators >> >>remove RandomGeneratorProperty from API >> - ... and 30 more: >> https://git.openjdk.java.net/jdk/compare/f7517386...6fe94c68 > > I am unsure if the intent is also to support external libraries providing > `RandomGenerator` implementations. Currently there is an implicit contract > for properties (reflectively invoking a method returning a map with a set of > entries with known keys). Since the service provider implementation is the > `RandomGenerator` itself, rather than `RandomGeneratorFactory` it is harder > expose the meta-data with a clearer contract. Need rebase - PR: https://git.openjdk.java.net/jdk/pull/1273
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Tue, 17 Nov 2020 22:21: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 .
>
> Jim Laskey has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains 40 commits:
>
> - Merge branch 'master' into 8248862
> - 8248862: Implement Enhanced Pseudo-Random Number Generators
>
>Update package-info.java
> - 8248862: Implement Enhanced Pseudo-Random Number Generators
>
>Updated RandomGeneratorFactory javadoc.
> - 8248862: Implement Enhanced Pseudo-Random Number Generators
>
>Updated documentation for RandomGeneratorFactory.
> - Merge branch 'master' into 8248862
> - Merge branch 'master' into 8248862
> - 8248862: Implement Enhanced Pseudo-Random Number Generators
>
>Move RandomGeneratorProperty
> - Merge branch 'master' into 8248862
> - 8248862: Implement Enhanced Pseudo-Random Number Generators
>
>Clear up javadoc
> - 8248862; Implement Enhanced Pseudo-Random Number Generators
>
>remove RandomGeneratorProperty from API
> - ... and 30 more:
> https://git.openjdk.java.net/jdk/compare/f7517386...6fe94c68
I am unsure if the intent is also to support external libraries providing
`RandomGenerator` implementations. Currently there is an implicit contract for
properties (reflectively invoking a method returning a map with a set of
entries with known keys). Since the service provider implementation is the
`RandomGenerator` itself, rather than `RandomGeneratorFactory` it is harder
expose the meta-data with a clearer contract.
src/java.base/share/classes/java/util/Random.java line 592:
> 590:
> 591: @Override
> 592: public Spliterator.OfInt makeIntsSpliterator(long index, long fence,
> int origin, int bound) {
Unsure if this and the other two methods are intended to be public or not,
since they are at the end of the class and override methods of a module private
class. In principle there is nothing wrong with such `Spliterator` factories,
but wonder if they are really needed given the `Stream` returning methods. The
arrangement of classes makes it awkward to hide these methods.
src/java.base/share/classes/java/util/SplittableRandom.java line 171:
> 169: * RandomGenerator properties.
> 170: */
> 171: static Map getProperties() {
With records exiting preview in 16 this map of properties could i think be
represented as a record instance, with better type safety, where
`RandomSupport.RandomGeneratorProperty` enum values become typed fields
declared on the record class. Something to consider after integration perhaps?
src/java.base/share/classes/java/util/SplittableRandom.java line 211:
> 209: *
> http://zimbry.blogspot.com/2011/09/better-bit-mixing-improving-on.html
> 210: */
> 211: private static long mix64(long z) {
Usages be replaced with calls to `RandomSupport.mixStafford13`?
src/java.base/share/classes/module-info.java line 250:
> 248: exports jdk.internal.util.xml.impl to
> 249: jdk.jfr;
> 250: exports jdk.internal.util.random;
Unqualified export, should this be `to jdk.random`?
src/jdk.random/share/classes/module-info.java line 50:
> 48: */
> 49: module jdk.random {
> 50: uses java.util.random.RandomGenerator;
Are these `uses` declarations needed? `ServiceLoader` is not used by this
module, and `RandomSupport` is not a service interface.
src/jdk.random/share/classes/module-info.java line 53:
> 51: uses RandomSupport;
> 52:
> 53: exports jdk.random to
Why is this needed?
src/java.base/share/classes/java/util/random/package-info.java line 50:
> 48: * given its name.
> 49: *
> 50: * The principal supporting class is {@link RandomGenertatorFactor}.
> This
s/RandomGenertatorFactor/RandomGenertatorFactory
src/java.base/share/classes/java/util/random/package-info.java line 140:
> 138: *
> 139: * For applications with no special requirements,
> 140: * "L64X128MixRandom" has a good balance among speed, space,
The documentation assumes that the `jdk.random` module is present in the JDK
image. Perhaps we need to spit the specifics to `jdk.random`?
src/java.base/share/classes/jdk/internal/util/random/RandomSupport.java line
1211:
> 1209: Udiff = -Udiff;
> 1210: U2 = U1;
> 1211: U1 -= Udiff;
Updated `U1` never used (recommend running the code through a checker e.g. use
IntelliJ)
src/java.base/share/classes/jdk/internal/util/random/RandomSupport.java line
331:
> 329: }
> 330: // Finally, we need to make sure the last z words are not all
> zero.
> 331: search: {
Nice! a rarely used feature :-)
src/java.base/share/classes/jdk/internal/util/random/RandomSupport.java line
1157:
> 1155: /*
> 1156:
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
> This PR is to introduce a new random number API for the JDK. The primary API > is found in RandomGenerator and RandomGeneratorFactory. Further description > can be found in the JEP https://openjdk.java.net/jeps/356 . Jim Laskey has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 40 commits: - Merge branch 'master' into 8248862 - 8248862: Implement Enhanced Pseudo-Random Number Generators Update package-info.java - 8248862: Implement Enhanced Pseudo-Random Number Generators Updated RandomGeneratorFactory javadoc. - 8248862: Implement Enhanced Pseudo-Random Number Generators Updated documentation for RandomGeneratorFactory. - Merge branch 'master' into 8248862 - Merge branch 'master' into 8248862 - 8248862: Implement Enhanced Pseudo-Random Number Generators Move RandomGeneratorProperty - Merge branch 'master' into 8248862 - 8248862: Implement Enhanced Pseudo-Random Number Generators Clear up javadoc - 8248862; Implement Enhanced Pseudo-Random Number Generators remove RandomGeneratorProperty from API - ... and 30 more: https://git.openjdk.java.net/jdk/compare/f7517386...6fe94c68 - Changes: https://git.openjdk.java.net/jdk/pull/1273/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1273=02 Stats: 14891 lines in 31 files changed: 0 ins; 3704 del; 77 mod Patch: https://git.openjdk.java.net/jdk/pull/1273.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1273/head:pull/1273 PR: https://git.openjdk.java.net/jdk/pull/1273
