CSR-RFR: 8256817: Better support ALPN byte wire values in SunJSSE
Hi Xuelei/Jamil/Tony/others(?), I need a reviewer for this CSR, in preparation for: CSR: https://bugs.openjdk.java.net/browse/JDK-8256817 Bug: https://bugs.openjdk.java.net/browse/JDK-8254631 Draft Change: https://github.com/openjdk/jdk/pull/1440 8254631: Better support ALPN byte wire values in SunJSSE It is the same approach as we have discussed internally, but after further analysis, I now consider the interoperability risk minimal due to a latent bug that was discovered during prototyping/testing. I'd like to get this putback into JDK 16, so will need to be reviewed shortly after break to make the mid-December RDP1 deadline. Thanks, Brad
Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v5]
On Thu, 12 Nov 2020 16:34:29 GMT, Anthony Scarpino wrote: >> This is an update for all of Valerie's comments and adding a test to verify >> different buffer types and configurations > > The latest update should address all outstanding comments. The biggest > change was to the test, which had major reorganization and created tests that > increments data sizes for update and doFinal ops byte-by-byte to check for > any errors. That should reduce concerns about buffer corruption. The biggest part of this change is the addition of overlap protection and the tests to verify it for GCM, as there were none in the open repo. Additionally, GCMBufferTest had some significant changes to clean it up and handle offsets better. All tests pass. With RDP1 coming, I want to get this into the repo soon, so please limit comments to bugs. Any "nice to have" changes, they can be added onto follow-on changes I plan. - PR: https://git.openjdk.java.net/jdk/pull/411
Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v5]
> 8253821: Improve ByteBuffer performance with GCM Anthony Scarpino has updated the pull request incrementally with five additional commits since the last revision: - test updates - test check mixup - Overlap protection - Updated code comments, all tests pass - Updated code comments, all tests pass - Changes: - all: https://git.openjdk.java.net/jdk/pull/411/files - new: https://git.openjdk.java.net/jdk/pull/411/files/8580c6ec..836bf3ed Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=411=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=411=03-04 Stats: 745 lines in 6 files changed: 543 ins; 48 del; 154 mod Patch: https://git.openjdk.java.net/jdk/pull/411.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/411/head:pull/411 PR: https://git.openjdk.java.net/jdk/pull/411
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: 8257083: Security infra test failures caused by JDK-8202343
On Wed, 25 Nov 2020 20:24:52 GMT, Sean Mullan wrote: > There are several infra test failures that were caused by the fix for > JDK-8202343 (Disable TLS 1.0 and 1.1). > > The problem is that > test/jdk/javax/net/ssl/TLSCommon/interop/JdkProcClient.java is designed to be > run with different versions of the JDK such as JDK 8 but now calls > SecurityUtils.removeFromDisabledTlsAlgs() which calls APIs such as List.of() > that are not available in JDK 8. > > The fix is to create a private method which does the same thing as > SecurityUtils.removeFromDisabledTlsAlgs() but doesn't depend on newer APIs. Marked as reviewed by xuelei (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1442
RFR: 8257083: Security infra test failures caused by JDK-8202343
There are several infra test failures that were caused by the fix for JDK-8202343 (Disable TLS 1.0 and 1.1). The problem is that test/jdk/javax/net/ssl/TLSCommon/interop/JdkProcClient.java is designed to be run with different versions of the JDK such as JDK 8 but now calls SecurityUtils.removeFromDisabledTlsAlgs() which calls APIs such as List.of() that are not available in JDK 8. The fix is to create a private method which does the same thing as SecurityUtils.removeFromDisabledTlsAlgs() but doesn't depend on newer APIs. - Commit messages: - 8257083: Security infra test failures caused by JDK-8202343 Changes: https://git.openjdk.java.net/jdk/pull/1442/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1442=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8257083 Stats: 24 lines in 1 file changed: 21 ins; 2 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1442.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1442/head:pull/1442 PR: https://git.openjdk.java.net/jdk/pull/1442
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 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
