On Fri, 18 Sep 2026 09:33:16 GMT, Johan Sjölen <[email protected]> wrote:
>> The fields declaring layout support for value classes are duplicated across >> the FieldLayoutBuilder, InlineKlass, and ClassFileParser. I suggest that we >> move all of these into a new class, called AvailableLayouts. >> >> This rids us of many duplicated lines of code, and makes keeping the names >> consistent easier (today, they are not consistent). >> >> For the design of `AvailableLayouts`, I decided on putting the size fields >> into an array which is indexed by casting `LayoutKind` into an `int`. This >> saves us code bloat, both in the repo, and also in our binaries. When I've >> looked at the generated code for the inlined callsites of >> `InlineKlass::layout_size_in_bytes`, the switch is compiled down into table >> dispatch. Now, we can just have it be a load from an object offset. This >> ought to be faster as well, as no prediction needs to take place. >> >> There's a lot of added `const` stuff in this PR as well, as issues with >> const-correctness came up during the refactoring. >> >> --------- >> - [X] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Johan Sjölen has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 30 commits: > > - Merge remote-tracking branch 'origin/master' into simplify2 > - Service agent fixes > - Merge branch 'master' into simplify2 > - Check first > - Need to include BUFFERED > - Remove faulting assert (maybe worth looking into?) > - Rename to largest_layout_of > - Use least_restrictive_layout_of > - Fix bug > - Use a ternary and remove the default value > - ... and 20 more: https://git.openjdk.org/jdk/compare/f33a724f...c834aa37 I started to look at this because I'm poking at some of these fields. (Investigating the difference between layout kind for fields [which includes REFERENCE, but not BUFFERED] and layout kinds of value payloads [which includes BUFFERED but not REFERENCE]) I don't know if this PR is about to land, than let's integrate it, but if not I think there are a few nits that I think would be good to deal with. Many of the nits are about looking at the surrounding code and following the pre-existing style. src/hotspot/share/oops/layoutKind.hpp line 98: > 96: NULLABLE_NON_ATOMIC_FLAT = 5, // flat, include a null marker, > non-atomic, only used for strict final non-static fields > 97: UNKNOWN = 6, // used for uninitialized fields of > type LayoutKind > 98: COUNT = UNKNOWN Adding COUNT here pollutes the LayoutKind type with something that is not a layout kind. Could this be a constant outside of the enum? src/hotspot/share/oops/layoutKind.hpp line 138: > 136: // The different layouts available for a particular Klass > 137: struct LayoutDescriptions { > 138: constexpr static int NoValue = -1; // Unsupported layouts are assigned > this value Suggestion: constexpr static int NoValue = -1; // Unsupported layouts are assigned this value src/hotspot/share/oops/layoutKind.hpp line 145: > 143: // Size of each LayoutKind. For atomic layouts, the size also acts as > alignment. > 144: int _sizes[static_cast<size_t>(LayoutKind::COUNT)]; // REFERENCE has > no size, so we remove 1 > 145: LayoutDescriptions() Suggestion: LayoutDescriptions() src/hotspot/share/oops/layoutKind.hpp line 156: > 154: set_size_in_bytes_of(LayoutKind::NULL_FREE_ATOMIC_FLAT, NoValue); > 155: set_size_in_bytes_of(LayoutKind::NULLABLE_ATOMIC_FLAT, NoValue); > 156: set_size_in_bytes_of(LayoutKind::NULLABLE_NON_ATOMIC_FLAT, NoValue); Odd indentation: Suggestion: : _payload_alignment(NoValue), _non_atomic_alignment(NoValue), _payload_offset(NoValue), _null_marker_offset(NoValue), _sizes() { set_size_in_bytes_of(LayoutKind::REFERENCE, heapOopSize); set_size_in_bytes_of(LayoutKind::BUFFERED, NoValue); set_size_in_bytes_of(LayoutKind::NULL_FREE_NON_ATOMIC_FLAT, NoValue); set_size_in_bytes_of(LayoutKind::NULL_FREE_ATOMIC_FLAT, NoValue); set_size_in_bytes_of(LayoutKind::NULLABLE_ATOMIC_FLAT, NoValue); set_size_in_bytes_of(LayoutKind::NULLABLE_NON_ATOMIC_FLAT, NoValue); src/hotspot/share/oops/valueKlass.hpp line 190: > 188: } > 189: > 190: public: This creates an inconsistency with the private: above. src/hotspot/share/oops/valueKlass.hpp line 198: > 196: return members().layouts(); > 197: } > 198: Have you considered if this style matches the rest of the class? src/hotspot/share/oops/valueKlass.hpp line 228: > 226: void set_null_reset_value_offset(int offset) { > members()._null_reset_value_offset = offset; } > 227: > 228: void set_layouts(LayoutDescriptions& other) { const LayoutDescriptor& ? ------------- PR Review: https://git.openjdk.org/jdk/pull/32276#pullrequestreview-5247368835 PR Review Comment: https://git.openjdk.org/jdk/pull/32276#discussion_r4046384692 PR Review Comment: https://git.openjdk.org/jdk/pull/32276#discussion_r4046389845 PR Review Comment: https://git.openjdk.org/jdk/pull/32276#discussion_r4046393578 PR Review Comment: https://git.openjdk.org/jdk/pull/32276#discussion_r4046398102 PR Review Comment: https://git.openjdk.org/jdk/pull/32276#discussion_r4046413574 PR Review Comment: https://git.openjdk.org/jdk/pull/32276#discussion_r4046427617 PR Review Comment: https://git.openjdk.org/jdk/pull/32276#discussion_r4046416876
