On Mon, 27 Apr 2026 19:58:18 GMT, Frederic Parain <[email protected]> wrote:
>> This patch adds a simple policy to put a limit on field flattening to >> prevent instances from becoming too large. It is easy to generate very huge >> instances with nested flat values, so huge that the JVM struggles to >> allocate its internal data structure to handle such classes. The new policy >> allocates a flattening budget to each class, and field flattening in this >> class stops when the budget is exhausted. The limit can be tuned with a VM >> parameter. >> >> Tested on Mach5 tier1-4 >> >> --------- >> - [X] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Frederic Parain has updated the pull request incrementally with two > additional commits since the last revision: > > - Silently skip the test when flattening conditions are not satisfied > - Add argument and CDS validation of the new VM option This seems useful to have, thanks! Left a comment about flag level, left some other remarks about test maintainability. src/hotspot/share/classfile/fieldLayoutBuilder.cpp line 805: > 803: _fields_size_sum(0), > 804: _declared_nonstatic_fields_count(0), > 805: _flattening_budget(FlatteningBudget), Nit: initialize this somewhere else, and then assert that the `FlatteningBudget` flag is `>0`. While this is constrained by the flag, I still think it's good to have an extra safeguard in place, just in case the flag changes later on. src/hotspot/share/runtime/globals.hpp line 835: > 833: "before no flattening attempts are made, <0 indicates no > limit") \ > 834: > \ > 835: product(intx, FlatteningBudget, 1024, > \ The flag should be diagnostic to be consistent. Flags such as `UseArrayFlattening` are set to diagnostic. I don't think this is more "important" than `UseArrayFlattening`. Let me know if you disagree. src/hotspot/share/runtime/globals.hpp line 836: > 834: > \ > 835: product(intx, FlatteningBudget, 1024, > \ > 836: "Maximum size dedicated to flat fields in an instance") > \ Nit: it would be nice to specify the unit (bytes) here. test/hotspot/jtreg/runtime/valhalla/inlinetypes/TestFlatteningBudget.java line 95: > 93: static HotSpotDiagnosticMXBean hsDiag = > ManagementFactory.getPlatformMXBean(HotSpotDiagnosticMXBean.class); > 94: > 95: static int getIntVMOption(String vmOption) { FYI: We have a much nicer interface through `WhiteBox`, which would prevent having to write all the boilerplate around `ManagementFactory`. test/hotspot/jtreg/runtime/valhalla/inlinetypes/TestFlatteningBudget.java line 111: > 109: // Test cannot work properly if field flattening is disabled > 110: if (!useFieldFlattening) { > 111: System.out.println("Test skipped because field flattening > is disabled"); The "proper" way of doing this is by throwing a `SkippedException`. ------------- PR Review: https://git.openjdk.org/valhalla/pull/2372#pullrequestreview-4188182543 PR Review Comment: https://git.openjdk.org/valhalla/pull/2372#discussion_r3153647952 PR Review Comment: https://git.openjdk.org/valhalla/pull/2372#discussion_r3153617347 PR Review Comment: https://git.openjdk.org/valhalla/pull/2372#discussion_r3153634891 PR Review Comment: https://git.openjdk.org/valhalla/pull/2372#discussion_r3153628397 PR Review Comment: https://git.openjdk.org/valhalla/pull/2372#discussion_r3153655280
