On Mon, 26 Jan 2026 10:30:57 GMT, Stefan Karlsson <[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
>
> src/hotspot/share/classfile/fieldLayoutBuilder.cpp line 930:
>
>> 928: bool use_atomic_flat = _must_be_atomic; // flatten atomic fields
>> only if the container is itself atomic
>> 929: LayoutKind lk = field_layout_selection(fieldinfo,
>> _inline_layout_info_array, use_atomic_flat);
>> 930: const int field_index = (int)fieldinfo.index();
>
> Are `idx` and `field_index` different here?
I don't think they ever are. I added an assert and ran tier1-3, which never
hit. Maybe @fparain can shed some light on this? Otherwise I think we should
use `field_info.index()` in favor of `idx` and maybe replace the
GrowableArrayIterator with a range-based for loop, like this:
for (FieldInfo fi : *_field_info) {
...
}
-------------
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1966#discussion_r2727311083
