8346107: Generators: testing utility for random value generation

[eme64]

For verification testing, it is often critical to generate "interesting" values, to provoke overflows, NaN, etc. And to generate these values in the correct distribution to trigger certain optimizations.

I would like to start a collection of such generators, that can then be used in testing.

The goal is to grow this collection in the future, and add new types. For example byte, char, short, or even Float16.

This will be helpful for the Template framework JDK-8344942, but also other tests.

Related PR, for value verification: #22715

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8346107: Generators: testing utility for random value generation (Enhancement - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/22716/head:pull/22716
$ git checkout pull/22716

Update a local copy of the PR:
$ git checkout pull/22716
$ git pull https://git.openjdk.org/jdk.git pull/22716/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22716

View PR using the GUI difftool:
$ git pr show -t 22716

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/22716.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back epeter! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@eme64 The following label will be automatically applied to this pull request:

• hotspot-compiler

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.

[mlbridge]

Webrevs

• 01: Full - Incremental (219593a9)
• 00: Full (ed64bb91)

[theoweidmannoracle]

I think these classes will be extremely helpful! I left some (nitpicky) comments.

[theoweidmannoracle]

If you make IntGenerator and LongGenerator Interfaces (you can still have the method definitions as default implementations) you could move the shared code between MixedIntGenerator and MixedLongGenerator into a common superclass MixedGenerator. (There might be other trade-offs to this approach though.)

[eme64]

Hmm. I know there is a bit of code duplication. Generics with primitive types would have been nice.

I don't think I want generators of different types to share too much, it would probably create some complicated dependencies that would make extending this to other types harder.

[theoweidmannoracle]

Same as for MixedLong/IntGenerator: Special*Generator could share common logic in a superclass if Double/FloatGenerator are interfaces.

[eme64]

Again: I'm not sure if doing this with more complicated and more generic code is really easier than a bit of code duplication. Not that I'm in general an advocate for code duplication 🙈 😅

[theoweidmannoracle]

What I'm thinking of doesn't involve any generics. I'm talking about specialMinFrequency, specialMaxFrequency, specialCountDown and the logic in next* to decide whether to provide a special value or not now. This is independent from whether float or double is used, as far as I can tell.

[eme64]

Ok. I suppose I could factor this out into some extra class and extra file.
But is that worth it? Then I need to still somehow route all that logic through the constructor, and nextDouble. I think in the end I have more code and more abstractions, I don't yet see how that is worth it.

Maybe you had a better idea in mind that I did not yet think of ;)

[theoweidmannoracle]

To be more clear, without generics:

abstract class FloatingGenerator {
      // specialCountDown detemines in how many iterations we generate the next special value.
     private final int specialMinFrequency;
     private final int specialMaxFrequency;
     private int specialCountDown;

    // .. constructor ...

     protected final bool shouldUseSpecial() {
         specialCountDown--;
         if (specialCountDown <= 0) {
             specialCountDown = RANDOM.nextInt(specialMinFrequency, specialMaxFrequency);
             return true;
         } else {
             return false;
         }  
     }
}

Then in SpecialDoubleGenerator:

     @Override
     public double nextDouble() {
         if (shouldUseSpecial()) {
             int r = RANDOM.nextInt(VALUES.length);
             return VALUES[r];
         } else {
             return backgroundGenerator.nextDouble();
         }
     }

If you make FloatingGenerator generic, though, of course, and rename the methods from nextDouble and nextFloat to just next for the generators, then everything except for the list of special values itself could be moved into FloatingGenerator. That would result in very clean code in my opinion. If you need to adjust the logic for special selection later, with the current approach, that requires changes in multiple locations, which in my experience, ultimately leads to inconsistencies.

[theoweidmannoracle]

I posted my second comment without seeing your latest comment. So it's not really a reply to that.

[eme64]

Hmm. I would have to make the classes FloatGenerator and DoubleGenerator interfaces. But having default methods for interfaces is not really the recommended way, as far as I know. And then I cannot make those default methods final, which sucks - java does not allow it.

Alternative: I make a separate class RandomDistance to compose into the others, that one can query with isDistanceReached. Would that be worth it you think?

[eme64]

@theoweidmannoracle Thanks a lot for reviewing, and all the suggestions. I addressed everything except your idea with sharing code between MixedIntGenerator and MixedLongGenerator, and between SpecialDoubleGenerator and SpecialFloatGenerator.

I would have to make the classes FloatGenerator and DoubleGenerator interfaces. But having default methods for interfaces is not really the recommended way, as far as I know. And then I cannot make those default methods final, which sucks - java does not allow it. So I would have to create the shared code in a separate class, that would then be used by composition in the other classes.

I really don't think that abstraction is worth it. It would end up with the same amount of code in the original files, plus the extra code in the new shared file.

If you don't agree with my opinion, then I suppose we have to let a 3rd person decide ;)

[eme64]

Thanks @theoweidmannoracle @chhagedorn for the offline design-session yesterday. It was very interesting.

We decided @theoweidmannoracle would take over this task, and refactor it substantially. I'll close it now, and @theoweidmannoracle can open his own PR.