RFR: 8332086: Remove the usage of ServiceLoader in j.u.r.RandomGeneratorFactory

[Raffaello Giulietti]

On Mon, 13 May 2024 13:06:04 GMT, Jaikiran Pai <jpai at 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