8346107: Generators: testing utility for random value generation #22941

theoweidmannoracle · 2025-01-07T08:58:17Z

This PR is a refactoring and partial rewrite of #22716 by @eme64. The goals remain the same:

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

The refactoring makes use of generics, rendering the generators library more flexible by default, by allowing it work with arbitrary types (with special features for Comparable types), improving the composability of different generators and streamlining the client API for simplicity. This allows test authors to quickly compose their own distributions and generators if necessary. An overview of this functionality is provided in the Generators javadoc.

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)

Contributors

Emanuel Peter <[email protected]>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22941

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-01-07T08:59:19Z

👋 Welcome back tweidmann! 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 · 2025-01-07T09:00:03Z

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

openjdk · 2025-01-07T09:01:05Z

@theoweidmannoracle 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.

theoweidmannoracle · 2025-01-07T09:01:10Z

/contributor @eme64

openjdk · 2025-01-07T09:01:52Z

@theoweidmannoracle Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

/contributor add @openjdk-bot
/contributor add duke
/contributor add J. Duke <[email protected]>

User names can only be used for users in the census associated with this repository. For other contributors you need to supply the full name and email address.

theoweidmannoracle · 2025-01-07T09:02:36Z

/contributor add @eme64

openjdk · 2025-01-07T09:02:54Z

@theoweidmannoracle
Contributor Emanuel Peter <[email protected]> successfully added.

mlbridge · 2025-01-07T09:18:34Z

Webrevs

eme64

Nice work! I have a few comments in the code.

I am also wondering if we can still do this:

pick a random int distribution -> suppose we get mixed special + uniform
now sample from different int ranges, all from that same underlying distribution:
- [0, max]
- [10, 20]
- ...

I think it is not possible:

gen = Generators.ints();
// gen is not resrictable, sadly... but I would like to do
gen1 = gen.restrict(0, max);
v1 = gen1.next();
gen2 = gen.restrict(10, 20);
v2 = gen2.next()

If that is indeed not possible:
How can we ensure the continuity of the distribution across different range restrictions, if we want to pick a random distribution?

Edit: let me explain why I need this.

Suppose we will work with the Template Framework, once we have it.

#{g1:int_con(0,int_max)}
#{g2:int_con(0,1000)}
#{g3:int_con}

Basically: I am sampling 3 values from different distributions. Now suppose some thing crazy only happens when all 3 values are close to powers of 2. To make that somewhat likely, I would like to first draw a random distribution (which may be the one with only powers of 2, and neighbors), and then draw all values from that distribution.

test/hotspot/jtreg/compiler/lib/generators/EmptyGeneratorException.java

eme64 · 2025-01-07T13:17:00Z

test/hotspot/jtreg/compiler/lib/generators/Generator.java

+    /**
+     * Returns the next value from the stream.
+     */
+    T next();


Should this not have a @return?

Why don't you run something like:
/oracle-work/jdk-fork0/build/linux-x64-debug/jdk/bin/javadoc -sourcepath test/hotspot/jtreg:./test/lib compiler.lib.generators
And make sure you don't have any errors/warnings :)

What would you like to see here with @return? I don't think there's any more information I can provide? Do you think the entire doc comment should be like below? In my experience that can be detrimental for some IDEs as they will only show the main text (for a lack of a better word) in some circumstances.

/** * @returns the next value from the stream. */

To get away with all the warnings would mean documenting every single parameter everywhere (all lo's and hi's). Do you think that's really necessary?

test/hotspot/jtreg/compiler/lib/generators/GeneratorBase.java

eme64 · 2025-01-07T13:25:55Z

test/hotspot/jtreg/compiler/lib/generators/Generators.java

+ * "interesting" distributions which might trigger various behaviours in
+ * optimizations.
+ * <p>
+ * Normally, clients get the default Generators instance by referring to the static variable {@link #G}.


It would be nice to have an example test that uses it as you would expect.

test/hotspot/jtreg/compiler/lib/generators/Generators.java

test/hotspot/jtreg/compiler/lib/generators/RandomnessSource.java

eme64 · 2025-01-07T13:50:25Z

test/hotspot/jtreg/testlibrary_tests/generators/tests/TestGenerators.java

+    static void testMixed() {
+        mockRandomness
+                .checkEmpty()
+                .enqueueInteger(0, 10, 7)
+                .enqueueInteger(0, 10, 5)
+                .enqueueInteger(0, 31, 4)
+                .enqueueInteger(0, 10, 1)
+                .enqueueInteger(0, 31, 18);
+        var g = mockGS.mixed(mockGS.uniformInts(0, 30), mockGS.single(-1), 7, 3);
+        Asserts.assertEQ(g.next(), -1);
+        Asserts.assertEQ(g.next(), 4);
+        Asserts.assertEQ(g.next(), 18);
+    }


It would be nice if you told us / a future person who extends this, what this mocking does, and how it works.

Do you mean specifically how this test here works or how the mocking works? Or both?

Maybe just say what values you feed in, and why it produces the results.
That should hopefully help a future person who tries to extend the test for their own type.

I documented MockRandomnessSource and added comments to this test case. I hope that provides enough clarifications to understand the other tests as it is always the same pattern (and this is definitely the most complicated one).

eme64 and others added 14 commits December 12, 2024 16:54

JDK-8346107

37216a5

wip float generators

f046670

more float generators

c82170c

double generators

ed64bb9

Extra documentation, and review suggestions v1

cc8f74c

rm whitespace

ea5b4ce

use Generators.Random, rm some imports

219593a

Some refactoring

445b6be

Implement most changes

d089207

Delete patches.iml

df97545

Update SingleValueGenerator.java

87d616a

Improve documentation, naming, more restrictable

01f81e7

Add new lines

65fc024

Add copyright

e8745e6

Fix typo

940ac12

openjdk bot added the hotspot-compiler [email protected] label Jan 7, 2025

Improve documentation

36066fa

theoweidmannoracle marked this pull request as ready for review January 7, 2025 09:13

openjdk bot added the rfr Pull request is ready for review label Jan 7, 2025

eme64 suggested changes Jan 7, 2025

View reviewed changes

theoweidmannoracle added 2 commits January 7, 2025 16:56

Naming and documentation improvements

fb08fbe

Improve phrasing

bb5074d

theoweidmannoracle added 4 commits January 8, 2025 13:57

Allow more restricting

776afec

Improve MockRandomnessSource documentation

5bb3fe9

Rename

b3408c0

Add safe restrict

6e247dc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8346107: Generators: testing utility for random value generation #22941

8346107: Generators: testing utility for random value generation #22941

theoweidmannoracle commented Jan 7, 2025 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 7, 2025

openjdk bot commented Jan 7, 2025

openjdk bot commented Jan 7, 2025

theoweidmannoracle commented Jan 7, 2025

openjdk bot commented Jan 7, 2025

theoweidmannoracle commented Jan 7, 2025

openjdk bot commented Jan 7, 2025

mlbridge bot commented Jan 7, 2025 •

edited

Loading

eme64 left a comment •

edited

Loading

eme64 Jan 7, 2025

theoweidmannoracle Jan 7, 2025

eme64 Jan 7, 2025

eme64 Jan 7, 2025

theoweidmannoracle Jan 7, 2025

eme64 Jan 8, 2025

theoweidmannoracle Jan 8, 2025 •

edited

Loading

8346107: Generators: testing utility for random value generation #22941

Are you sure you want to change the base?

8346107: Generators: testing utility for random value generation #22941

Conversation

theoweidmannoracle commented Jan 7, 2025 • edited by openjdk bot Loading

Progress

Issue

Contributors

Reviewing

bridgekeeper bot commented Jan 7, 2025

openjdk bot commented Jan 7, 2025

openjdk bot commented Jan 7, 2025

theoweidmannoracle commented Jan 7, 2025

openjdk bot commented Jan 7, 2025

theoweidmannoracle commented Jan 7, 2025

openjdk bot commented Jan 7, 2025

mlbridge bot commented Jan 7, 2025 • edited Loading

Webrevs

eme64 left a comment • edited Loading

Choose a reason for hiding this comment

eme64 Jan 7, 2025

Choose a reason for hiding this comment

theoweidmannoracle Jan 7, 2025

Choose a reason for hiding this comment

eme64 Jan 7, 2025

Choose a reason for hiding this comment

eme64 Jan 7, 2025

Choose a reason for hiding this comment

theoweidmannoracle Jan 7, 2025

Choose a reason for hiding this comment

eme64 Jan 8, 2025

Choose a reason for hiding this comment

theoweidmannoracle Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

theoweidmannoracle commented Jan 7, 2025 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 7, 2025 •

edited

Loading

eme64 left a comment •

edited

Loading

theoweidmannoracle Jan 8, 2025 •

edited

Loading