On Mon, 26 Jan 2026 08:43:40 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
I'm leaving the proper review to Valhalla-devs, but I added a few comments that
could be considered.
src/hotspot/share/classfile/classFileParser.cpp line 6298:
> 6296: if (klass != nullptr) {
> 6297: if (klass->is_inline_klass()) {
> 6298: set_inline_layout_info_klass(fieldinfo.index(), klass,
> CHECK);
I see that this can throw a Metaspace OOME (hence the CHECK) and that we later
have this comment:
// Loads triggered by the LoadableDescriptors attribute are
speculative, failures must not impact loading of current class
if (HAS_PENDING_EXCEPTION) {
CLEAR_PENDING_EXCEPTION;
}
Can you verify that using CHECK here is really appropriate? The
`resolve_super_or_fail` uses THREAD, and I think that's what the comment refer
to. But with the current patch there's a "returning" call in-between those two
parts of the function.
src/hotspot/share/classfile/classFileParser.cpp line 6341:
> 6339: _must_be_atomic, _layout_info, _inline_layout_info_array);
> 6340: lb.build_layout();
> 6341: _has_inlined_fields = _layout_info->_has_inlined_fields;
More of a questions to the long-time Valhalla devs: I'm a little curious why
the parser has booth of these fields containing the same value?
src/hotspot/share/classfile/classFileParser.cpp line 6394:
> 6392:
> 6393: void ClassFileParser::set_inline_layout_info_klass(int field_index,
> Klass* klass, TRAPS) {
> 6394: assert(field_index >= 0 && field_index < java_fields_count(), "IOOB");
I would propose that you split && asserts into two asserts, or add an error
message so that we can see what part failed if this ever asserts.
src/hotspot/share/classfile/fieldLayoutBuilder.cpp line 856:
> 854: group->add_oop_field(idx);
> 855: } else {
> 856: assert(_inline_layout_info_array != nullptr &&
> _inline_layout_info_array->adr_at(field_index)->klass() != nullptr, "Array
> must have been set up");
&& assert - see previous comment
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?
-------------
PR Review:
https://git.openjdk.org/valhalla/pull/1966#pullrequestreview-3705382128
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1966#discussion_r2727022060
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1966#discussion_r2727061299
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1966#discussion_r2727067158
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1966#discussion_r2727080548
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1966#discussion_r2727092250
