8332086: Remove the usage of ServiceLoader in j.u.r.RandomGeneratorFactory #19212
Conversation
|
👋 Welcome back rgiulietti! A progress list of the required criteria for merging this PR into |
|
@rgiulietti This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 143 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
openjdk
bot
added
csr
|
@rgiulietti The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
|
This is a followup of 18932 in which all random number generator algorithms have been moved to module Reliance on Moreover, methods Tests in tier1-tier3 pass after adaptations on the existing |
| synchronized (rgClass) { | ||
| if (properties == null) { // double-checking idiom | ||
| RandomGeneratorProperties props = rgClass.getDeclaredAnnotation(RandomGeneratorProperties.class); | ||
| Objects.requireNonNull(props, rgClass + " missing annotation"); |
There was a problem hiding this comment.
Hello Raffaello, with the RandomGenerator implementations now all residing within the java.base module, I think an additional advantage of that is that, it will now allow us to remove this internal RandomGeneratorProperties annotation and thus this reflection code.
I think one way to do that would be something like this within this RandomGeneratorFactory class itself:
private record RandomGenEntry(Class<?> randomGenClass, int i, int j,
int k, int equiDistribution, boolean stochastic,
boolean hardware) {
}
private static final Map<String, RandomGenEntry> FACTORY_MAP = ... // construct the mapwhere the FACTORY_MAP will be keyed to the alogrithm and the value will be a record which holds these additional details about the RandomGenerator.
This current PR is about getting rid of ServiceLoader usage. So if you want to remove the usage of this annotation and reflection is a separate PR that's fine with me. Furthermore, although I don't see the necessity of an annotation for what we are doing here, if you think that the removal of the annotation and reflection isn't worth doing, that is OK too.
There was a problem hiding this comment.
Thinking a bit more, I think we can even get rid of the reflection used in create() methods implementation, if we wanted to, by doing something like this:
private record RandomGenEntry(Class<? extends RandomGenerator> randomGenClass, int i, int j,
int k, int equiDistribution, boolean stochastic,
boolean hardware) {
RandomGenerator create() {
String algo = randomGenClass.getSimpleName();
return switch (algo) {
case "Random" -> new Random();
case "L128X1024MixRandom" -> new L128X1024MixRandom();
case "Xoshiro256PlusPlus" -> new Xoshiro256PlusPlus();
// ... so on for the rest
default -> throw new InternalError("should not happen");
};
}
RandomGenerator create(long seed) {
String algo = randomGenClass.getSimpleName();
return switch (algo) {
case "Random", "SplittableRandom", "SecureRandom" -> {
throw new UnsupportedOperationException("cannot construct with a long seed");
}
case "L128X1024MixRandom" -> new L128X1024MixRandom(seed);
case "Xoshiro256PlusPlus" -> new Xoshiro256PlusPlus(seed);
// ... so on for the rest
default -> throw new InternalError("should not happen");
};
}
RandomGenerator create(byte[] seed) {
String algo = randomGenClass.getSimpleName();
return switch (algo) {
case "Random", "SplittableRandom", "SecureRandom" -> {
throw new UnsupportedOperationException("cannot construct with a byte[] seed");
}
case "L128X1024MixRandom" -> new L128X1024MixRandom(seed);
case "Xoshiro256PlusPlus" -> new Xoshiro256PlusPlus(seed);
// ... so on for the rest
default -> throw new InternalError("should not happen");
};
}
}
private static final Map<String, RandomGenEntry> FACTORY_MAP = ... // construct the mapThere was a problem hiding this comment.
@jaikiran I agree that we can simplify even more. I just wanted to change as little as possible in this PR to facilitate reviews.
Shall I push your proposed changes in this PR or is a followup PR preferable?
There was a problem hiding this comment.
A followup PR is fine. I think once this PR which addresses the API aspects (like removal of ServiceLoader and then updates to the create() method javadoc) is integrated, then the subsequent PR can just be all internal implementation changes like the proposed removal of reflection.
There was a problem hiding this comment.
Not sure if that's an issue - but if you wanted/needed to delay the loading of those random generator classes that will not be used until something actually wants to use them, you could consider using a Supplier<Class<? extends RandomGenerator>> or a Supplier<RandomGenEntry> for the FACTORY_MAP values?
There was a problem hiding this comment.
@dfuch You mean not loading the whole batch but only individual classes, as need arises?
There was a problem hiding this comment.
yes - I do not know if loading all the classes in one batch could be an issue at startup (I'd expect Random to be loaded early) - but if it proves to be, then using a Supplier to load them on demand might do the trick. If creating Random or SecureRandom does not trigger this code - and if those classes are not loaded at startup - then maybe that's a non issue and you can just ignore my comment.
There was a problem hiding this comment.
Loading of Random, SecureRandom, SplittableRandom, or ThreadLocalRandom does not trigger the loading of RandomGeneratorFactory.
| properties = provider.type().getDeclaredAnnotation(RandomGeneratorProperties.class); | ||
| Objects.requireNonNull(properties, provider.type() + " missing annotation"); | ||
| if (properties == null) { // volatile load | ||
| synchronized (rgClass) { |
There was a problem hiding this comment.
The synchronization on rgClass to intialize an instance field properties appears odd here. I think this should be synchronized on this.
| } | ||
| private void ensureConstructors() { | ||
| if (ctor == null) { // volatile load | ||
| synchronized (rgClass) { |
There was a problem hiding this comment.
Here too I think we should synchronize on this - we would want to allow multiple different instances of a RandomGeneratorFactory for the same RandomGenerator class type to be able to concurrently instantiate their individual instance fields (like the ctor(s) and properties).
There was a problem hiding this comment.
Then I would even remove the double-checking idiom, the volatile on ctor and properties, and declare methods getProperties() and ensureConstructors() as synchronized.
I'm not sure that the double-checking optimization brings much value on contemporary JVMs.
But I feel that the followup PR discussed before wouldn't need synchronized at all.
WDYT?
There was a problem hiding this comment.
Then I would even remove the double-checking idiom, the volatile on ctor and properties, and declare methods getProperties() and ensureConstructors() as synchronized.
I'm not sure that the double-checking optimization brings much value on contemporary JVMs.
Making the methods synchronized would bring in a penalty that there will always be a monitor entry at every call site, even after the properites and ctor(s) are initialized. Ideally, we should just do all of this intialization in the constructor of the RandomGeneratorFactory, the one which takes the Class<> type of the RandomGenerator. We can then make the properties and the ctor(s) all final and not have to worry about any synchronization or volatile semantics. You would of course have to rework the ensureConstructors to not throw an exception at that time.
But I feel that the followup PR discussed before wouldn't need synchronized at all.
Correct. The more I think about it, I think cleaning up all this in this PR itself might make both reviewing and the implementation a bit more simpler. What's your thoughts?
There was a problem hiding this comment.
OK, will do all the work in this PR.
| private static SimpleImmutableEntry<String, RandomGeneratorProperties> | ||
| entry(Class<? extends RandomGenerator> rgClass, String name, String group, | ||
| int i, int j, int k, int equidistribution, | ||
| int flags) { | ||
| return new SimpleImmutableEntry<>(name, |
There was a problem hiding this comment.
This should use Map.Entry<…> as the return type, which in turn allows using Map::entry(K, V) as the implementation, as neither the name key, nor the value is ever null:
| private static SimpleImmutableEntry<String, RandomGeneratorProperties> | |
| entry(Class<? extends RandomGenerator> rgClass, String name, String group, | |
| int i, int j, int k, int equidistribution, | |
| int flags) { | |
| return new SimpleImmutableEntry<>(name, | |
| private static Map.Entry<String, RandomGeneratorProperties> | |
| entry(Class<? extends RandomGenerator> rgClass, String name, String group, | |
| int i, int j, int k, int equidistribution, | |
| int flags) { | |
| return Map.entry(name, |
There was a problem hiding this comment.
@ExE-Boss Thanks for the suggestion.
|
Thank you for these updates Raffaello. I think the implementation is looking much more simpler and cleaner now. I've a few documentation related review comments which I've added inline. I haven't reviewed the test change yet. |
| * algorithm to the static method {@link RandomGenerator#of}, in which case the | ||
| * no-arguments constructor for that implementation is used: | ||
| * algorithm to the static method {@link RandomGenerator#of}, in which case no | ||
| * seed is specified by the caller: |
There was a problem hiding this comment.
Perhaps reword this to:
... in which case a {@code RandomGenerator} is constructed without any seed value:
There was a problem hiding this comment.
An already existing issue in the specification in this file is that, a few lines below, we note that:
There are three groups of random number generator algorithm provided in Java: the Legacy group, the LXM group, and the Xoroshiro/Xoshiro group.
The "three" groups is misleading I think, since both later in the table as well as the group() method implementation on RandomGeneratorFactory, we return four distinct values "Legacy", "LXM", "Xoroshiro" and "Xoshiro". Should we reword this part of the documentation to remove the mention of "three"?
| * <a href="package-summary.html#algorithms">algorithm</a> chosen, | ||
| * and providing a starting long seed. | ||
| * If a long seed is not supported by the algorithm, | ||
| * an {@link UnsupportedOperationException} is thrown. |
There was a problem hiding this comment.
Perhaps reword this and the other create(byte[]) method to something like:
* Create an instance of {@link RandomGenerator} based on the
* <a href="package-summary.html#algorithms">algorithm</a> chosen,
* and the provided {@code seed}.
* If the {@code RandomGenerator} doesn't support instantiation through
* a {@code seed} of type {@code long} then this method throws an
* an {@link UnsupportedOperationException}.
| return Map.entry(name, | ||
| new RandomGeneratorProperties(rgClass, name, group, | ||
| i, j, k, equidistribution, | ||
| flags | (rgClass.isAnnotationPresent(Deprecated.class) ? DEPRECATED : 0))); |
There was a problem hiding this comment.
Hello Raffaello, this is the final remaining reflection usage and even this I think isn't required now that all the random generator implementations reside within java.base as an implementation detail.
I think we should just skip this annotation check here and set DEPRECATED bit on the flags to 0 for all implementations. When/if we do deprecate any of the random generators, we can just come here and switch that bit to on for the specific random generator when instantiating this RandomGeneratorProperties record. I had a brief look at the code and the documentation in package-info.java of java/util/random and we don't mention that we rely on the @Deprecated annotation to determine whether an algorithm is deprecated. I think that's a good thing.
There was a problem hiding this comment.
Yes, I thought about this the other day but decided for a bit more conservative approach, relying on the annotation.
But I agree that, since the meta-information now resides in RandomGeneratorProperties, we might "migrate" the deprecation status here as well.
There was a problem hiding this comment.
Since the legacy generators are public classes, they can be publicly deprecated, so in the last commit the DEPRECATED bit for them still relies on the annotation, which IMO is the authoritative "source of truth".
For the 10 other algorithms, which are accessible only via RandomFactoryGenerator, we can rely on the info in RandomProperties.
There was a problem hiding this comment.
The deprecation process of algorithms is unclear - whether it should be tied with the @Deprecation of a class that provides that algorithm. We will have more clarity if/when we do deprecate these algorithms. The specification in its current form doesn't tie it to the @Deprecation annotation, which is a good thing. So your proposed change in this PR looks fine to me, since it will still allow us to move away from the @Deprecated annotation check if we wanted to, in a subsequent release/change.
| rng = null; | ||
| try { | ||
| rng = factory.create(12345L); | ||
| } catch (UnsupportedOperationException ignore) { |
There was a problem hiding this comment.
I think now that we know for sure which algorithm instances don't support which type of seed value and since throwing UnsupportedOperationException is now a specification of the create(...) methods, we should probably do something like this:
diff --git a/test/jdk/java/util/Random/RandomTestCoverage.java b/test/jdk/java/util/Random/RandomTestCoverage.java
index be12d188198..6e5c36e13c3 100644
--- a/test/jdk/java/util/Random/RandomTestCoverage.java
+++ b/test/jdk/java/util/Random/RandomTestCoverage.java
@@ -171,8 +171,37 @@ static void coverFactory(RandomGeneratorFactory factory) {
boolean isSplittable = factory.isSplittable();
coverRandomGenerator(factory.create());
- coverRandomGenerator(factory.create(12345L));
- coverRandomGenerator(factory.create(new byte[] {1, 2, 3, 4, 5, 6, 7, 8}));
+
+ String algo = factory.name();
+ // test create(long)
+ switch (algo) {
+ // SecureRandom doesn't have long constructors so we expect
+ // UnsupportedOperationException
+ case "SecureRandom" -> {
+ try {
+ factory.create(12345L);
+ throw new AssertionError("RandomGeneratorFactory.create(long) was expected" +
+ "to throw UnsupportedOperationException for " + algo + " but didn't");
+ } catch (UnsupportedOperationException uoe) {
+ // expected
+ }
+ }
+ default -> coverRandomGenerator(factory.create(12345L));
+ }
+ // test create(byte[])
+ switch (algo) {
+ // these don't have byte[] constructors so we expect UnsupportedOperationException
+ case "Random", "SplittableRandom" -> {
+ try {
+ factory.create(new byte[] {1, 2, 3, 4, 5, 6, 7, 8});
+ throw new AssertionError("RandomGeneratorFactory.create(byte[]) was expected" +
+ "to throw UnsupportedOperationException for " + algo + " but didn't");
+ } catch (UnsupportedOperationException uoe) {
+ // expected
+ }
+ }
+ default -> coverRandomGenerator(factory.create(new byte[] {1, 2, 3, 4, 5, 6, 7, 8}));
+ }
}
There was a problem hiding this comment.
So you want to be very specific. OK.
| } | ||
| if (rng != null) { | ||
| coverRandomGenerator(rng); | ||
| } | ||
| } | ||
|
|
||
| static void coverDefaults() { |
There was a problem hiding this comment.
This test method appears to test the calls to getDefault() methods on RandomGeneratorFactory and RandomGenerator classes, but it isn't being called in the test. We should call this method from main() to have test coverage for those methods.
|
/issue JDK-8332476 |
|
/issue add JDK-8332476 |
|
@rgiulietti |
|
@rgiulietti |
| * <p> There are three groups of random number generator algorithm provided | ||
| * in Java: the Legacy group, the LXM group, and the Xoroshiro/Xoshiro group. | ||
| * <p> Random number generator algorithms are organized in groups, | ||
| * as described <a href="package-summary.html#algorithms">below</a>. |
There was a problem hiding this comment.
FYI, it is possible to link to an anchor in a package-info file using syntax like
{@linkplain java.util.random##algorithms below}
openjdk
bot
added
ready
|
/integrate |
|
Going to push as commit 42e3c84.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@rgiulietti Pushed as commit 42e3c84. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
All random number generator algorithms are implemented in module
java.base. The usage ofServiceLoaderinj.u.r.RandomGeneratorFactoryis no longer needed.Progress
Warning
8332476: j.u.r.RandomGeneratorFactor.create(long|byte[]) should throw rather than silently fallback to no-arg create()Issues
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19212/head:pull/19212$ git checkout pull/19212Update a local copy of the PR:
$ git checkout pull/19212$ git pull https://git.openjdk.org/jdk.git pull/19212/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 19212View PR using the GUI difftool:
$ git pr show -t 19212Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19212.diff
Webrev
Link to Webrev Comment