On Fri, 18 Sep 2026 12:30:24 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 incrementally with one additional > commit since the last revision: > > StefanK comments and a bug fix More comments after looking at this a bit more. src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp line 2975: > 2973: if (vk->supports_nullable_layouts()) { > 2974: // Zero the null marker (setting it to 1 would be better but would > require an additional register) > 2975: __ strb(zr, Address(r0, vk->layouts().null_marker_offset())); I must say that I find the older code more pleasant to read. Why not keep the `null_marker_offset()` and implement it as a `vk->layouts().null_marker_offset()`? src/hotspot/share/classfile/fieldLayoutBuilder.cpp line 1159: > 1157: assert(_is_abstract_value, "Concrete value types must have at least > one field"); > 1158: layouts().set_payload_offset(_layout->blocks()->size()); > 1159: layouts().set_size_in_bytes_of(LayoutKind::BUFFERED, 0); Does this mean that the `_size[BUFFERED]` has taken on the role to hold the payload size, but we still have `_payload_alignment` and `_payload_offset` in the `LayoutDescriptors`? I can say that I was surprised by this when reading other parts of the code. the code incorrectly because of that. Take a look at this code: layouts().set_payload_alignment(required_alignment); ... layouts().set_size_in_bytes_of(LayoutKind::BUFFERED, largest_layout_of(LayoutKind::BUFFERED, LayoutKind::NULLABLE_ATOMIC_FLAT, LayoutKind::NULLABLE_NON_ATOMIC_FLAT, LayoutKind::NULL_FREE_ATOMIC_FLAT)); It is not clear that the second the payload_alignment and the buffered size goes hand-in-hand here. I'm asking myself why we don't create an array of alignments for for all the layout kinds, get rid of `_payload_size`, `_payload_alignment`, and `_non_atomic_alignment`. The `payload_size()` / `payload_alignment()` functions would query the array, and the `alignment_of` function would be simplified from: int alignment_of(LayoutKind lk) const { assert(has_a(lk), "Layout not available"); switch (lk) { case LayoutKind::BUFFERED: return _payload_alignment; case LayoutKind::NULLABLE_ATOMIC_FLAT: case LayoutKind::NULL_FREE_ATOMIC_FLAT: return size_in_bytes_of(lk); case LayoutKind::NULL_FREE_NON_ATOMIC_FLAT: case LayoutKind::NULLABLE_NON_ATOMIC_FLAT: return _non_atomic_alignment; case LayoutKind::REFERENCE: case LayoutKind::UNKNOWN: break; } ShouldNotReachHere(); return 0; } To something like: int alignment_of(LayoutKind lk) const { assert(has_a(lk), "Layout not available"); return _alignment[lk]; } That would get rid of questions, why are we reading `_payload_alignment` for the Buffered, `size_in_bytes_of` for atomics, and `_non_atomic_alignment` for the non-atomics. The drawback would be a couple of more ints, but I think it would make the code more readable. src/hotspot/share/classfile/fieldLayoutBuilder.cpp line 1264: > 1262: const int required_alignment = MAX2(default_alignment, > 1263: > largest_layout_of(LayoutKind::NULL_FREE_ATOMIC_FLAT, > 1264: > LayoutKind::NULLABLE_ATOMIC_FLAT)); Something is off with the indentation here. src/hotspot/share/classfile/fieldLayoutBuilder.hpp line 308: > 306: FieldGroup* get_or_create_contended_group(int g); > 307: > 308: LayoutDescriptions& layouts() { return _available_layouts; } The rest of the code use `layouts()` but the field is named `_available_layouts`. Why not name the field as `_layouts`? src/hotspot/share/classfile/fieldLayoutBuilder.hpp line 311: > 309: > 310: template<typename T> > 311: int largest_layout_of(T lk) { The name of this function seems a bit off, given that it only tests one layout kind instance. And with that said, is this even used? It looks like this was supposed to be the base case, but the `largest_layout_of` function below doesn't use this function, but instead has its own version of it. src/hotspot/share/oops/layoutKind.hpp line 145: > 143: int _null_marker_offset; > 144: // Size of each LayoutKind. For atomic layouts, the size also acts as > alignment. > 145: int _sizes[static_cast<size_t>(LayoutKind::COUNT)]; // REFERENCE has > no size, so we remove 1 I don't see a removal of 1. Instead it seems like the iteration starts from `BUFFERED`. src/hotspot/share/oops/layoutKind.hpp line 199: > 197: return _payload_alignment; > 198: } > 199: This doesn't follow the style of the surrounding code. Suggestion: int payload_alignment() const { return _payload_alignment; } bool has_payload_alignment() const { return _payload_alignment != NoValue; } void set_payload_alignment(int alignment) { _payload_alignment = alignment; } ------------- PR Review: https://git.openjdk.org/jdk/pull/32276#pullrequestreview-5264288984 PR Review Comment: https://git.openjdk.org/jdk/pull/32276#discussion_r4060681304 PR Review Comment: https://git.openjdk.org/jdk/pull/32276#discussion_r4060520703 PR Review Comment: https://git.openjdk.org/jdk/pull/32276#discussion_r4060463376 PR Review Comment: https://git.openjdk.org/jdk/pull/32276#discussion_r4060295372 PR Review Comment: https://git.openjdk.org/jdk/pull/32276#discussion_r4060317341 PR Review Comment: https://git.openjdk.org/jdk/pull/32276#discussion_r4060389873 PR Review Comment: https://git.openjdk.org/jdk/pull/32276#discussion_r4060258799
