On Tue, 21 Apr 2026 10:05:01 GMT, Evgeny Nikitin <[email protected]> wrote:
> Currently JITTester only generates common, instance classes. This PR suggests > a value classes support for the tool. > > Specifically, it: > - adds a ValueKlass as an AST node carrier for the new type > - moves most of KlassFactory internals into generic AbstractKlassFactory > - adds smaller concrete KlassFactory and ValueKlassFactory. > - makes sure that we don't generate mutable fields in the value classes > - blocks synchronized usage on value-class methods > - raises probability to be abstract for the value classes - to check deeper > hierarchies. > - adds a parameter governing the balance between instance and value classes. > > --------- > - [x] I confirm that I make this contribution in accordance with the [OpenJDK > Interim AI Policy](https://openjdk.org/legal/ai). Thank you for implementing this. I do not have much experience with jittester, so I've left a more surface-level review. Some further comments based on your description: > adds smaller concrete KlassFactory and ValueKlassFactory. What could be worth exploring is a non-uniform distribution of value class payload size. I think the lower and upper ends of that spectrum are particularly interesting: payloads small enough will be flattenable in arrays, and extremely large will stress the VM more. > blocks synchronized usage on value-class methods Just to make sure -- you don't generate any `synchronized (obj)` mutex blocks right? As these would need to be updated to ensure `obj` has identity. One more big-picture remark: post JEP 401, we may see enhancements in nullability and/or non-atomicity. While these AFAIK do not have designs that are finalized, one thing that I think might be useful to think about is whether this current implementation can be extended without too much friction. If, for example, annotations around the class declaration and/or object usage are introduced, can we extend `ValueKlass` or do we need to split it into multiple classes? test/hotspot/jtreg/testlibrary/jittester/src/jdk/test/lib/jittester/classes/ValueKlass.java line 34: > 32: > 33: /** > 34: * A simple POJO representing Value Class in the AST. Would there be value (pun not intended) in generating value records at some point (in the future)? test/hotspot/jtreg/testlibrary/jittester/src/jdk/test/lib/jittester/factories/AbstractKlassFactory.java line 69: > 67: this.level = level; > 68: interfaces = new ArrayList<>(); > 69: abstractProbabilityAdjustment = 0.2; // Probability to consider > making class an abstract s/an abstract/abstract test/hotspot/jtreg/testlibrary/jittester/src/jdk/test/lib/jittester/factories/IRNodeBuilder.java line 364: > 362: getMemberFunctionsArgLimit(), getComplexityLimit(), > getStatementLimit(), > 363: getOperatorLimit(), getLevel(), getFlags(), > isSynchronizedAllowed); > 364: isSynchronizedAllowed = true; I'm not following why the first invocation respects the local `isSynchronizationAllowed`, but then we explicitly set it to be allowed. What happens if we call `getFunctionDefinitionBlockFactory` twice on a value class? test/hotspot/jtreg/testlibrary/jittester/src/jdk/test/lib/jittester/factories/ValueKlassFactory.java line 45: > 43: super(name, complexityLimit, memberFunctionsLimit, > memberFunctionsArgLimit, > 44: statementsInFunctionLimit, operatorLimit, level); > 45: abstractProbabilityAdjustment = 1.0; // We want more abstract > value classes IIUC we now have a 100% probability * 20% probability of making it abstract? test/hotspot/jtreg/testlibrary/jittester/src/jdk/test/lib/jittester/factories/ValueKlassFactory.java line 45: > 43: super(name, complexityLimit, memberFunctionsLimit, > memberFunctionsArgLimit, > 44: statementsInFunctionLimit, operatorLimit, level); > 45: abstractProbabilityAdjustment = 1.0; // We want more abstract > value classes So as I understand, we have a 100% probability to go to make a class abstract with 20% probability? test/hotspot/jtreg/testlibrary/jittester/src/jdk/test/lib/jittester/visitors/ByteCodeVisitor.java line 1098: > 1096: @Override > 1097: public byte[] visit(ValueKlass node) { > 1098: throw new UnsupportedOperationException("ByteCodeVisitor > doesn't support Value classes"); What are the implications of this? Is this something that's planned in a follow-up? test/hotspot/jtreg/testlibrary/jittester/src/jdk/test/lib/jittester/visitors/JavaCodeVisitor.java line 663: > 661: TypeKlass thisKlass = node.getThisKlass(); > 662: String r = (ProductionParams.enableStrictFP.value() ? "strictfp > " : "") > 663: + (thisKlass.isFinal() ? "final " : "") Value classes are implicitly final unless explicitly denoted as abstract. ------------- PR Review: https://git.openjdk.org/valhalla/pull/2346#pullrequestreview-4153311574 PR Review Comment: https://git.openjdk.org/valhalla/pull/2346#discussion_r3122713405 PR Review Comment: https://git.openjdk.org/valhalla/pull/2346#discussion_r3122751561 PR Review Comment: https://git.openjdk.org/valhalla/pull/2346#discussion_r3122856646 PR Review Comment: https://git.openjdk.org/valhalla/pull/2346#discussion_r3123149620 PR Review Comment: https://git.openjdk.org/valhalla/pull/2346#discussion_r3123456121 PR Review Comment: https://git.openjdk.org/valhalla/pull/2346#discussion_r3123462758 PR Review Comment: https://git.openjdk.org/valhalla/pull/2346#discussion_r3123465990
