On Tue, 10 Mar 2026 11:54:20 GMT, Frederic Parain <[email protected]> wrote:
>> 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
>
> Frederic Parain has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Fix generation of short values
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.
test/hotspot/jtreg/runtime/valhalla/inlinetypes/field_layout/ValueClassGenerator.java
line 146:
> 144: c =
> (char)random.nextInt(Character.MAX_VALUE+1);
> 145:
> 146: } 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.)
test/hotspot/jtreg/runtime/valhalla/inlinetypes/field_layout/ValueClassGenerator.java
line 151:
> 149: false, new String[] {"(char)1",
> "Character.MAX_VALUE", "Character.MIN_VALUE"});
> 150: primitiveTypes.add(new PrimitiveDesc("char", false, charVals));
> 151: String[] shortVals = getPregeneratedValues(() -> {
Thinking about it, we could get less complexity if we converted
`Supplier<String>` to a Function<Set<String>, String>` whereby the `Set`
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.
test/hotspot/jtreg/runtime/valhalla/inlinetypes/field_layout/ValueClassGenerator.java
line 341:
> 339:
> 340: String generateSource() {
> 341: String src =
> abstractValueClassTemplate.replaceAll("<class_modifiers>", "public abstract
> value class");
FYI: `String#replaceAll` takes a regex as a string argument. A normal
`String#replace` will already replace all occurrences. This doesn't really
matter since our search string does not contain meta characters, but given that
we call `generateSource` quite a few times it might eat CPU cycles compiling
the regex for every replacement.
test/hotspot/jtreg/runtime/valhalla/inlinetypes/field_layout/ValueClassGenerator.java
line 497:
> 495: }
> 496:
> 497: 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.
test/hotspot/jtreg/runtime/valhalla/inlinetypes/field_layout/ValueComparisonTest.java
line 90:
> 88: Path tempWorkDir;
> 89: try {
> 90: 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:
https://github.com/openjdk/valhalla/blob/432870d43a3ef3ffb181e5909454af58754febc9/test/lib/jdk/test/lib/Utils.java#L808
test/hotspot/jtreg/runtime/valhalla/inlinetypes/field_layout/ValueRandomLayoutTest.java
line 64:
> 62: List<String> argsList = new ArrayList<>();
> 63: String classpath = System.getProperty("java.class.path") +
> System.getProperty("path.separator") +
> 64: Paths.get("").toAbsolutePath() +
Shouldn't this already be included by jtreg somehow? Not sure, but I've not
seen other similar tests use it.
test/hotspot/jtreg/runtime/valhalla/inlinetypes/field_layout/ValueRandomLayoutTest.java
line 140:
> 138: Path currentDir = Paths.get("").toAbsolutePath();
> 139: try {
> 140: tempWorkDir = Files.createTempDirectory(currentDir,
> "generatedClasses_" + seed);
See other comment on temp dir creation.
-------------
PR Review:
https://git.openjdk.org/valhalla/pull/2202#pullrequestreview-3923804760
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2202#discussion_r2913014116
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2202#discussion_r2913066366
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2202#discussion_r2913089185
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2202#discussion_r2913103080
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2202#discussion_r2913151031
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2202#discussion_r2913157846
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2202#discussion_r2913161300
