On Wed, 22 Apr 2026 09:12:39 GMT, Paul Hübner <[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). > > 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? That's a weird mechanism of scope-level protection and control via glogal flags that is used everywhere in the project. It is very error-prone, indeed. I'm preparing a better, stack-based one. For now, the method is only called by produceFunctionDefinitions(), which for the ValueKlassFactory sets explicitly `.setIsSynchronizedAllowed(false)`. > 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? 1.0 * (1 - 0.8<sup>n</sup>), where n is the number of methods, IIUC. Identity-classes have 0.2 * (1 - 0.8<sup>n</sup>), accordingly. Honestly, I just wanted a yet one easily-controllable coefficient here. > 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? 1.0 * (1 - 0.8 <sup>n</sup>), where n is the number of methods, IIUC. Honestly, I just wanted to add a simple, easily-tuned coefficient here. For instance classes this is `0.2 * ...` > 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? I honestly don't trust the BytecodeGenerator at all and didn't want to add support here. Bytecodes generated by javac for value classes do not differ with a single flag or something, they are different in many places. But we can add support in the future. Implications are following - users of the library would need to switch off support for value klasses in the ProductionParams when they use the BytecodeGenerator. ------------- PR Review Comment: https://git.openjdk.org/valhalla/pull/2346#discussion_r3125488363 PR Review Comment: https://git.openjdk.org/valhalla/pull/2346#discussion_r3125547043 PR Review Comment: https://git.openjdk.org/valhalla/pull/2346#discussion_r3128686425 PR Review Comment: https://git.openjdk.org/valhalla/pull/2346#discussion_r3125560022
