8379355: [lworld] Improve test coverage of fields layouts and substitutability test with randomly generated classes by fparain · Pull Request #2202 · openjdk/valhalla

fparain · 2026-03-05T20:26:13Z

This patch adds new tests of fields layouts and the substitutability method using randomly generated methods.
In its current form, the value class generator is limited to the JEP 401 model, meaning it doesn't support null-restricted fields nor non-atomic values.

The value class generator (ValueClassGenerator.java) is used by ValueRandomLayoutTest.java which runs the FieldLayoutAnalyzer on all generated classes to verify invariants of layouts.
The value class generator is also used by ValueComparisonTest.java which tests the substitutability method. The generated value classes have a set of pre-computed values than can be used for instance initializations and value comparisons.

Testing in progress tier1-4

Fred

Progress

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

Issue

JDK-8379355: [lworld] Improve test coverage of fields layouts and substitutability test with randomly generated classes (Enhancement - P4)

Reviewers

Paul Hübner (@Arraying - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2202

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/2202.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2026-03-05T20:27:22Z

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

openjdk · 2026-03-05T20:33:10Z

@fparain This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8379355: [lworld] Improve test coverage of fields layouts and substitutability test with randomly generated classes

Reviewed-by: phubner

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 2 new commits pushed to the lworld branch:

f81526c: 8378531: [lworld] Remove obj_at(int) from objArrayOopDesc and flatArrayOopDesc
4e71493: 8378771: [lworld] JFR Cooperative Sampling support for needs_stack_repair methods and frames

Please see this link for an up-to-date comparison between the source branch of this pull request and the lworld branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the lworld branch, type /integrate in a new comment.

mlbridge · 2026-03-05T20:40:15Z

Webrevs

Arraying

Nice test, thanks for the work. I've done a first pass of feedback mostly on the jtreg side of things to ensure that everything goes smoothly.

Arraying · 2026-03-06T12:59:54Z

+        shortVals[2] = "(short)1";
+        shortVals[3] = "Short.MAX_VALUE";
+        shortVals[4] = "Short.MIN_VALUE";
+        for (int i = 5; i < NUM_PREDEFINED_VALUES; i++) {


Same comment for short and int as for char. Would be nice to also factor out the generation into its own method. For example:
void generateUniqueRandom(int low, int high, Function<Integer, Object> mapper, Object[] destination) and then call it e.g. generateUniqueRandom(shortMin, shortMax, i -> (short) i, shortVals.

Arraying · 2026-03-06T13:04:35Z

+  static ProcessBuilder exec(boolean useAtomicFlat, boolean useNullableAtomicFlat,
+                             boolean useNullableNonAtomicFlat, String... args) throws Exception {
+    List<String> argsList = new ArrayList<>();
+    Collections.addAll(argsList, "--enable-preview");


Not necessary, ProcessTools will add it.

I've tried to remove it, but then the test fails because of an unsupported class file version.

That really shouldn't happen... we investigate once it's integrated.

fparain · 2026-03-09T19:22:45Z

Thank you @Arraying for the review.
In the latest version, the generation of the pre-computed values of primitive type has been re-implemented with much more code factorization. The work directory has also been changed, now each set of generated classes is stored in its own subdirectory of the scratch directory.

Arraying

Nice work, this is already much easier to read! I've left some follow-up comments for improving readability even further. None of them are super strictly necessary, but I feel like it may be useful for me to write down some painpoints I had in understanding.

The file handling is much better. I think it should still be made consistent with the convention of the other tests that we have that create temp files (i.e., use our own util for it rather than Java's). If you disagree, let me know, but then I'd want to take an extra closer look to ensure we don't leave any stale files anywhere.

Arraying · 2026-03-10T16:34:18Z

+                                                    do {
+                                                        c = (char)random.nextInt(Character.MAX_VALUE+1);
+
+                                                    } while (c == Character.MAX_VALUE || c == Character.MIN_VALUE);


The existence loop is really confusing to me. Can we not constrain the nextInt to be within 0 to (MAX-MIN) and then offset it by MIN? (Or adjust the bounds by -1, +1, etc., but the point is then we'd always get the correct number.)

I prefer to follow the same pattern for each primitive type.

Arraying · 2026-03-10T16:43:18Z

+                                                  },
+                              false, new String[] {"(char)1", "Character.MAX_VALUE", "Character.MIN_VALUE"});
+      primitiveTypes.add(new PrimitiveDesc("char", false, charVals));
+      String[] shortVals = getPregeneratedValues(() -> {


Thinking about it, we could get less complexity if we converted Supplier<String> to a Function<Set, String>whereby theSet` represents all of the already computed values. So then our lambda becomes something like:

String v = null; do { v = "(short)" + (random.nextInt(0, (shortMax - shortMin)) + shortMin); } while (existing.contains(v))

Using a Set would also simplify getPregeneratedValues greatly. We won't need to do the duplicate checking etc. ourselves. I would suggest a TreeSet for backing so we are deterministic. We can really easily turn the Set into a String[] at the end before we return.

I want to preserve the hard coded values with their original writing ('MAX_VALUE' and others) when generating the source file of the test classes. Still doable with the approach you propose, but then each value would have to be stored in both String and numerical format. I don't think this complexity is justified for such a minor piece of code.

Arraying · 2026-03-10T16:49:15Z

+        return (int)Math.ceil(Math.exp(random.nextInt(16)/7.0));
+    }
+
+    void printStatistics(ArrayList<? extends ValueClassDesc> list) {


This whole method could probably be simplified with a mapreduce-style stream/collect but it will get the job done.

Initially, it was a debug code that I kept because it can give a quick insight if the distributions used to generate the classes behave as expected or not. Again, not a critical part of the code.

Arraying · 2026-03-10T16:56:46Z

+        Path currentDir =  Paths.get("").toAbsolutePath();
+        Path tempWorkDir;
+        try {
+            tempWorkDir = Files.createTempDirectory(currentDir, "generatedClasses_" + seed);


Sorry, I gave you the wrong name in the earlier comment. I meant Utils not Path.
Use this instead:

valhalla/test/lib/jdk/test/lib/Utils.java

Line 808 in 432870d

public static Path createTempFile(String prefix, String suffix, FileAttribute<?>... attrs) throws IOException {

This code and the one in Utils.java are equivalent. The tests have been updated to use jdk/lib/Utils code for the sake of integration with JTREG. FYI, most tests in the runtime directory still use Files.createTemp[Directory|File] instead of the jdk/lib/Utils version.

We should migrate to Utils, I believe there is an effort in mainline for this. Thanks for changing!

Arraying

Looks good, great work!

fparain · 2026-03-11T17:23:04Z

Thanks @Arraying for the reviews and the suggestions.
/integrate

openjdk · 2026-03-11T17:25:24Z

Going to push as commit 56caf4a.
Since your change was applied there have been 2 commits pushed to the lworld branch:

f81526c: 8378531: [lworld] Remove obj_at(int) from objArrayOopDesc and flatArrayOopDesc
4e71493: 8378771: [lworld] JFR Cooperative Sampling support for needs_stack_repair methods and frames

Your commit was automatically rebased without conflicts.

openjdk · 2026-03-11T17:25:41Z

@fparain Pushed as commit 56caf4a.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

fparain added 6 commits February 27, 2026 15:13

Add tests using randomly generated values

e7eadf8

Remove old developement print statements

e71846a

Add an identity class to possible field types

a0abec7

Add abstract value classes and inheritance to generated classes

9a521b5

Fix layout output

7d87361

Test tuning

26951e4

Fix end of file return and remove dead code

2be1098

openjdk Bot changed the title ~~8379355 [lworld] Improve test coverage of fields layouts and substitutability test with randomly generated classe~~ 8379355: [lworld] Improve test coverage of fields layouts and substitutability test with randomly generated classes Mar 5, 2026

openjdk Bot added the rfr Pull request is ready for review label Mar 5, 2026

Arraying reviewed Mar 6, 2026

View reviewed changes

fparain added 3 commits March 6, 2026 13:17

Fix work directory, identation and other minor fixes

36b82d5

Refactor generation of primitives pre-generated values

613da08

Remove hard coded version number

eb62d71

fparain added 2 commits March 9, 2026 16:51

Fix classpath for Windows machines

94f170b

Fix generation of short values

edf9d7a

Arraying reviewed Mar 10, 2026

View reviewed changes

fparain added 2 commits March 10, 2026 15:10

Merge remote-tracking branch 'upstream/lworld' into generated_tests

79f1ee8

Fix String replacement and temporary directories creation

b3bacce

Arraying approved these changes Mar 11, 2026

View reviewed changes

openjdk Bot added the ready Pull request is ready to be integrated label Mar 11, 2026

openjdk Bot added the integrated Pull request has been integrated label Mar 11, 2026

openjdk Bot closed this Mar 11, 2026

openjdk Bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 11, 2026

Conversation

fparain commented Mar 5, 2026 • edited by openjdk Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper Bot commented Mar 5, 2026

Uh oh!

openjdk Bot commented Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge Bot commented Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Arraying left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fparain commented Mar 9, 2026

Uh oh!

Arraying left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Arraying left a comment

Choose a reason for hiding this comment

Uh oh!

fparain commented Mar 11, 2026

Uh oh!

openjdk Bot commented Mar 11, 2026

Uh oh!

openjdk Bot commented Mar 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

fparain commented Mar 5, 2026 •

edited by openjdk Bot

Loading

openjdk Bot commented Mar 5, 2026 •

edited

Loading

mlbridge Bot commented Mar 5, 2026 •

edited

Loading