RFR: 8346107: Generators: testing utility for random value generation

[theoweidmannoracle]

On Thu, 12 Dec 2024 15:56:45 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

> 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](https://bugs.openjdk.org/browse/JDK-8344942), but also other tests.
> 
> Related PR, for value verification: https://github.com/openjdk/jdk/pull/22715

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

test/hotspot/jtreg/compiler/lib/generators/AnyBitsDoubleGenerator.java line 30:

> 28: 
> 29: /**
> 30:  * Provide a any-bits double distribution random generator, i.e. the bits are uniformly sampled,

Nitpicking: I would suggest "Provides" instead, also below. (See, for example, https://www.oracle.com/uk/technical-resources/articles/java/javadoc-tool.html#styleguide.)

test/hotspot/jtreg/compiler/lib/generators/AnyBitsDoubleGenerator.java line 34:

> 32:  */
> 33: public final class AnyBitsDoubleGenerator extends DoubleGenerator {
> 34:     private static final Random RANDOM = Utils.getRandomInstance();

Would it make sense to use the definition Generators.RANDOM everywhere instead of having this definition in every generator class?

test/hotspot/jtreg/compiler/lib/generators/DoubleGenerator.java line 31:

> 29: 
> 30: /**
> 31:  * Define interface of double generators.

More nitpicking: I would omit "define", also below.

test/hotspot/jtreg/compiler/lib/generators/IntGenerator.java line 56:

> 54:      */
> 55:     public void fill(MemorySegment ms, int lo, int hi) {
> 56:         for (long i = 0; i < ms.byteSize() / 4; i++ ) {

Suggestion:

        for (long i = 0; i < ms.byteSize() / 4; i++) {

test/hotspot/jtreg/compiler/lib/generators/LongGenerator.java line 55:

> 53:      * Fill it with longs from the generators distribution.
> 54:      */
> 55:     public void fill(MemorySegment ms, long lo, long hi) {

Why are the two methods above final and both `fill` are not?

test/hotspot/jtreg/compiler/lib/generators/MixedIntGenerator.java line 34:

> 32:  * Mixed results between UniformIntGenerator and SpecialIntGenerator.
> 33:  */
> 34: public final class MixedIntGenerator extends IntGenerator {

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.)

test/hotspot/jtreg/compiler/lib/generators/SpecialDoubleGenerator.java line 32:

> 30:  * Provide a double distribution picked from a list of special values, including NaN, zero, int, etc.
> 31:  */
> 32: public final class SpecialDoubleGenerator extends DoubleGenerator {

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

test/hotspot/jtreg/compiler/lib/generators/SpecialDoubleGenerator.java line 56:

> 54:     private int specialCountDown;
> 55: 
> 56:     public SpecialDoubleGenerator(DoubleGenerator backgroundGenerator, int specialMinFrequency, int specialMaxFrequency) {

I think it would be nice to a brief description of what the specialMin/MaxFrequency parameters mean exactly. Also for the float version.

-------------

PR Review: https://git.openjdk.org/jdk/pull/22716#pullrequestreview-2511054561
PR Review Comment: https://git.openjdk.org/jdk/pull/22716#discussion_r1889772705
PR Review Comment: https://git.openjdk.org/jdk/pull/22716#discussion_r1889779179
PR Review Comment: https://git.openjdk.org/jdk/pull/22716#discussion_r1889773241
PR Review Comment: https://git.openjdk.org/jdk/pull/22716#discussion_r1889775599
PR Review Comment: https://git.openjdk.org/jdk/pull/22716#discussion_r1889792403
PR Review Comment: https://git.openjdk.org/jdk/pull/22716#discussion_r1889783286
PR Review Comment: https://git.openjdk.org/jdk/pull/22716#discussion_r1889787108
PR Review Comment: https://git.openjdk.org/jdk/pull/22716#discussion_r1889797186