On Thu, 3 Sep 2026 13:52:28 GMT, Dan Heidinga <[email protected]> wrote:
> Thanks Stefan for taking this on!
>
> Some initial comments on places were we missed the `inline`->`value` rename.
> Some of them may be better renamed as `flat` to match the flat array
> precedent.
I agree. There are also other renames that would make sense, like places where
we use "inline type" to refer to value objects. I've refrained from poking to
much at things like that in this PR. Let's figure out what I should do in this
PR and what we should leave for follow-ups.
>
> I haven't looked at everything yet but audited some of the cds/, classfile/,
> and oops/ code as a starting point.
>
> We should also consider what other PRs are in flight that may conflict with
> this. I know Vladimir has his large AOT code caching PR (once the JEP makes
> its way through the process) that it might be worth coordinating together
> about which goes first. Possibly other large PRs in the works as well?
Yes. I'm willing to help out with those.
> src/hotspot/share/classfile/classFileParser.cpp line 6592:
>
>> 6590:
>> 6591: // Set the Klass for the field's index
>> 6592: _inline_layout_info_array->adr_at(field_index)->set_klass(vk);
>
> Should `_inline_layout_info_array` be renamed to `_value_layout_info_array`
> for consistency with the new naming convention?
I was unsure if InlineLayoutInfo only referred to flattened fields and
therefore were sort-of better named as InlineLayoutInfo, FlattenedLayoutInfo,
or ValueLayoutInfo. WDYT?
> src/hotspot/share/classfile/verificationType.hpp line 71:
>
>> 69: // Enum for the _data field
>> 70: enum : uint {
>> 71: // Bottom three bits determine if the type is a reference, value
>> type,
>
> I think this comment is a left over from earlier models. The verifier
> doesn't track value types separately. Will open a separate cleanup bug to
> check the comments in verificationType to remove Inline/Value where it
> doesn't apply
Thanks.
> src/hotspot/share/code/nmethod.hpp line 212:
>
>> 210: uint16_t _entry_offset; // entry point with class check
>> 211: uint16_t _verified_entry_offset; // entry point without class check
>> 212: uint16_t _inline_entry_offset; // value type entry point
>> (unpack all value type args) with class check
>
> Suggestion:
>
> uint16_t _value_entry_offset; // value type entry point (unpack
> all value type args) with class check
Fixed.
> src/hotspot/share/code/nmethod.hpp line 688:
>
>> 686: address inline_entry_point() const { return code_begin()
>> + _inline_entry_offset; } // value type entry point (unpack all
>> value type args)
>> 687: address verified_inline_entry_point() const { return code_begin()
>> + _verified_inline_entry_offset; } // value type entry point (unpack all
>> value type args) without class check
>> 688: address verified_inline_ro_entry_point() const { return code_begin()
>> + _verified_inline_ro_entry_offset; } // value type entry point (only unpack
>> receiver) without class check
>
> Suggestion:
>
> address value_entry_point() const { return code_begin() +
> _value_entry_offset; } // value type entry point (unpack all
> value type args)
> address verified_value_entry_point() const { return code_begin() +
> _verified_value_entry_offset; } // value type entry point (unpack all
> value type args) without class check
> address verified_value_ro_entry_point() const { return code_begin() +
> _verified_value_ro_entry_offset; } // value type entry point (only unpack
> receiver) without class check
Fixed.
> src/hotspot/share/oops/instanceKlassFlags.hpp line 56:
>
>> 54: flag(has_miranda_methods , 1 << 12) /* True if this
>> class has miranda methods in it's vtable */ \
>> 55: flag(has_final_method , 1 << 13) /* True if klass
>> has final method */ \
>> 56: flag(has_inlined_fields , 1 << 14) /* has inlined
>> fields and related embedded section is not empty */ \
>
> This one should be renamed as well but not not entirely sure if it should be
> "value" or "flattened". value to be consistent with the inline->value
> mapping, flattened to be consistent with flat array naming.
>
> There's a few places that need to be consistently modified still related to
> this decision
I think `has_value_fields` would be wrong. I think `has_flattened_fields` would
be a good name. Should I change it to that?
-------------
PR Comment: https://git.openjdk.org/jdk/pull/32673#issuecomment-5527485828
PR Review Comment: https://git.openjdk.org/jdk/pull/32673#discussion_r3925720564
PR Review Comment: https://git.openjdk.org/jdk/pull/32673#discussion_r3925723307
PR Review Comment: https://git.openjdk.org/jdk/pull/32673#discussion_r3925728840
PR Review Comment: https://git.openjdk.org/jdk/pull/32673#discussion_r3925732740
PR Review Comment: https://git.openjdk.org/jdk/pull/32673#discussion_r3925747823
