On Tue, 27 Jan 2026 09:58:49 GMT, Joel Sikström <[email protected]> wrote:
>> Hello,
>>
>> Please refer to the JBS issue for a more detailed description of the
>> background of this change. In summary, I suggest we only keep the array of
>> InlineLayoutInfo for InstanceKlasses which need it, which are Klasses that
>> have fields that have been inlined.
>>
>> To make the transition to this easier, I suggest we change the following
>> properties in FieldLayoutBuilder:
>>
>> _has_inline_type_fields
>> _has_flattening_information
>>
>> to
>>
>> _has_inlineable_fields
>> _has_inlined_fields
>>
>> The `_has_inlineable_fields` property is only used for printing and
>> `_has_inlined_fields` is the property we expose out to the ClassFileParser,
>> telling us that this class has inlined fields, so the array of
>> InlineLayoutInfo must be "preserved" and is possible to read from. Hence,
>> the array is now only safe to access if `InstanceKlass::has_inlined_fields`
>> is true, or simply if the actual field being accessed is flat
>> (`fieldDescriptor::is_flat`).
>>
>> I only found one place (in ciReplay.cpp) where we access the array of
>> InlineLayoutInfo even though we might not have any inlined fields and only
>> fields that are inlineable. I've changed this to use the normal "reference"
>> path for fields that aren't flat.
>>
>> Testing:
>> * Oracle's tier1-5, hotspot_valhalla and jdk_valhalla
>
> Joel Sikström has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Exception check should really be an assert
> - Move inlineable check to static helper
Thanks a lot for doing this, the new names are much better and less memory
usage is always good. I left some comments, the most pressing one I'd like to
resolve is the notion of having flat fields when a value object is buffered.
src/hotspot/share/classfile/classFileParser.cpp line 6306:
> 6304: name->as_C_string(),
> _class_name->as_C_string());
> 6305: } else {
> 6306: // Non value class are allowed by the current spec, but
> it could be an indication of an issue so let's log a warning
If you're doing another round of iterations, a very nitpicky change is to
change this "warning" to "info". Seems I missed that when changing the log
levels, sorry.
src/hotspot/share/classfile/classFileParser.cpp line 6402:
> 6400: // never allocated for an InstanceKlass which has no need for this
> information.
> 6401: if (_inline_layout_info_array == nullptr) {
> 6402: _inline_layout_info_array =
> MetadataFactory::new_array<InlineLayoutInfo>(_loader_data,
We also have a `set_inline_layout_info_array`. AFAICT it's only used in
`ClassFileParser::apply_parsed_class_metadata` and
`InstanceKlass::deallocate_contents`. Maybe it'd be good to keep
`_inline_layout_info_array` setting consistent.
src/hotspot/share/classfile/fieldLayoutBuilder.cpp line 108:
> 106: }
> 107:
> 108: if (!fieldinfo.field_flags().is_injected() &&
What's the rationale of not inlining injected fields? Is there a reason we need
to treat them specially? I think I see this behaviour in the removed code as
well, but I'm curious why this is the case.
src/hotspot/share/classfile/fieldLayoutBuilder.cpp line 877:
> 875: assert(_inline_layout_info_array != nullptr, "Array must have
> been created");
> 876: assert(_inline_layout_info_array->adr_at(field_index)->klass()
> != nullptr, "Klass must have been set");
> 877: _has_inlined_fields = true;
This seems wrong, even if it was taken from the old code. We're saying that if
we've got a buffered LayoutKind, we've got inlined fields, but to my knowledge,
a buffered value object is basically a regular object.
src/hotspot/share/classfile/fieldLayoutBuilder.cpp line 965:
> 963: assert(_inline_layout_info_array != nullptr, "Array must have
> been created");
> 964: assert(_inline_layout_info_array->adr_at(field_index)->klass()
> != nullptr, "Klass must have been set");
> 965: _has_inlined_fields = true;
Nit: can we have some sort of `set/mark_inlined_fields_checked` which does
this? I saw the exact same code block above.
-------------
PR Review:
https://git.openjdk.org/valhalla/pull/1966#pullrequestreview-3716391238
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1966#discussion_r2736360051
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1966#discussion_r2736393824
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1966#discussion_r2736420392
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1966#discussion_r2736440070
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1966#discussion_r2736445565
