On Thu, 5 Mar 2026 20:26:13 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
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.
test/hotspot/jtreg/runtime/valhalla/inlinetypes/field_layout/TestAbstractValueTemplate.java.template
line 27:
> 25: import java.lang.reflect.Field;
> 26:
> 27: <class_modifiers> <class_name> <super_class> {
If this class is abstract, as this file name implies, it would be good to
hardcore `abstract` here. I see that for both this and the value template,
`<class modifiers>` is replaced with a constant string. Can we not have that
in-line? It would make the test easier to read/understand w/o having to go back
and forth.
test/hotspot/jtreg/runtime/valhalla/inlinetypes/field_layout/TestAbstractValueTemplate.java.template
line 29:
> 27: <class_modifiers> <class_name> <super_class> {
> 28: static Object[] predefined;
> 29: <fields declarations>
Nit: not following `snake_case` like other placeholders.
test/hotspot/jtreg/runtime/valhalla/inlinetypes/field_layout/TestValueTemplate.java.template
line 71:
> 69: throw new RuntimeException("Unexpected non-equal field/array
> comparison with value at index " + i);
> 70: }
> 71: for (int j = 0; j < <NUM_PREDEFINED>; j++) {
Nit: indentation and alignment for this method isn't consistent. Makes it hard
to read.
test/hotspot/jtreg/runtime/valhalla/inlinetypes/field_layout/TestValueTemplate.java.template
line 92:
> 90: }
> 91:
> 92: public boolean existInPredefinedValueSet() {
Unused?
test/hotspot/jtreg/runtime/valhalla/inlinetypes/field_layout/ValueClassGenerator.java
line 60:
> 58: String path = System.getProperty("test.src");
> 59: classTemplate = Files.readString(Path.of(path +
> File.separator + "TestValueTemplate.java.template"));
> 60: abstractValueClassTemplate = Files.readString(Path.of(path +
> File.separator + "TestAbstractValueTemplate.java.template"));
FYI: `File.separator` is not necessary, you can do
`Files.readString(Path.of(path, "xyz.template"));`
test/hotspot/jtreg/runtime/valhalla/inlinetypes/field_layout/ValueClassGenerator.java
line 133:
> 131: charVals[1] = "Character.MAX_VALUE";
> 132: charVals[2] = "Character.MIN_VALUE";
> 133: for (int i = 3; i < NUM_PREDEFINED_VALUES; i++) {
This block makes sense conceptually but is quite complex. I would suggest:
1. Create a range of integers from min char to max char.
2. Shuffle the list.
3. Take the first NUM_PREDEFINED_VALUES - 3.
4. Map each integer to a char.
This way you are guaranteed to have unique characters and it's more extensible
for the future, imo.
test/hotspot/jtreg/runtime/valhalla/inlinetypes/field_layout/ValueClassGenerator.java
line 137:
> 135: String s = null;
> 136: while (!foundNewVal) {
> 137: char c = (char)random.nextInt();
Why not constrain char ranges as `nextInt` params?
test/hotspot/jtreg/runtime/valhalla/inlinetypes/field_layout/ValueClassGenerator.java
line 159:
> 157: shortVals[3] = "Short.MAX_VALUE";
> 158: shortVals[4] = "Short.MIN_VALUE";
> 159: 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`.
test/hotspot/jtreg/runtime/valhalla/inlinetypes/field_layout/ValueClassGenerator.java
line 627:
> 625: }
> 626:
> 627: void writeValueClasses() {
I think this is the wrong approach. Instead, I would suggest to use
`Path.createTempFile` which uses the `scratch` directory by jtreg and is the
recommended way to create temporary files. We should not manually be cleaning
the filesystem, the runner generally knows best.
test/hotspot/jtreg/runtime/valhalla/inlinetypes/field_layout/ValueClassGenerator.java
line 670:
> 668: }
> 669: ArrayList<String> optionList = new ArrayList<>();
> 670: optionList.addAll(Arrays.asList("-source", "27"));
Is there a programmatic way to get the current version? Hardcoding it will give
us a headache if/when the version changes.
test/hotspot/jtreg/runtime/valhalla/inlinetypes/field_layout/ValueRandomLayoutTest.java
line 25:
> 23:
> 24: /*
> 25: * @test
Might need a randomness tag.
test/hotspot/jtreg/runtime/valhalla/inlinetypes/field_layout/ValueRandomLayoutTest.java
line 59:
> 57: boolean useNullableNonAtomicFlat, String...
> args) throws Exception {
> 58: List<String> argsList = new ArrayList<>();
> 59: Collections.addAll(argsList, "--enable-preview");
Not necessary, `ProcessTools` will add it.
-------------
PR Review:
https://git.openjdk.org/valhalla/pull/2202#pullrequestreview-3903532532
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2202#discussion_r2895594352
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2202#discussion_r2895597637
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2202#discussion_r2895609759
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2202#discussion_r2895612713
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2202#discussion_r2895617593
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2202#discussion_r2895639728
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2202#discussion_r2895647401
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2202#discussion_r2895656313
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2202#discussion_r2895688031
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2202#discussion_r2895699339
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2202#discussion_r2895669320
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2202#discussion_r2895674870
