On Mon, 13 May 2024 13:06:04 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java >> line 190: >> >>> 188: if (properties == null) { // double-checking idiom >>> 189: RandomGeneratorProperties props = >>> rgClass.getDeclaredAnnotation(RandomGeneratorProperties.class); >>> 190: Objects.requireNonNull(props, rgClass + " missing >>> annotation"); >> >> 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 map >> >> >> where 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. > > 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 map @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? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/19212#discussion_r1598459168