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
Looks good. Just one comment on using a more precis type signature.
A pre-existing issue which I wonder if we can do better. And a general question
of how the failure paths are suppose to work for these new pre-loaded classes.
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 think this is a property already precent in the code but this change made me
reflect on this.
What are the expected interactions for pre-loaded classes if we fail to load
parse the class file which contains them. For example here if we have just
resolved the pre-loaded class, and then get metaspace out of memory and stop
loading this class file.
Is this the correct (specification wise) interaction for partially failed class
file parsing?
src/hotspot/share/classfile/classFileParser.cpp line 6393:
> 6391: }
> 6392:
> 6393: void ClassFileParser::set_inline_layout_info_klass(int field_index,
> Klass* klass, TRAPS) {
This should be typed as `InstanceKlass* klass`.
2 out 3 call sites have that type, and the remaining can be trivially changed
to `InstanceKlass*`
https://github.com/jsikstro/valhalla/blob/ae50201a1b19eaa7a53bce222dfc93ab61dd31f9/src/hotspot/share/classfile/classFileParser.cpp#L6293
src/hotspot/share/classfile/fieldLayoutBuilder.cpp line 848:
> 846: || (!fieldinfo.field_flags().is_injected()
> 847: && _inline_layout_info_array != nullptr &&
> _inline_layout_info_array->adr_at(field_index)->klass() != nullptr
> 848: &&
> !_inline_layout_info_array->adr_at(field_index)->klass()->is_identity_class()))
> {
Pre-existing:
I wonder if this should be a helper function. (Maybe the whole predicate should
be)
But we are really asking is the klass of field type at `field_index` not an
identity class. The null checks seems like implementation details.
-------------
PR Review:
https://git.openjdk.org/valhalla/pull/1966#pullrequestreview-3705431560
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1966#discussion_r2727103461
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1966#discussion_r2727067959
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1966#discussion_r2727087888
