[lworld] RFR: 8376221: [lworld] Do not store array of InlineLayoutInfo for all InstanceKlasses [v4]

[Paul Hübner]

On Tue, 27 Jan 2026 09:58:49 GMT, Joel Sikström <jsikstro at openjdk.org> 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