On Mon, 2 Mar 2026 14:46:34 GMT, Axel Boldt-Christmas <[email protected]>
wrote:
> Right now some of the call sites assets that ResolvedFieldEntry corresponds
> to the correct fieldDescriptor, and that these fields are in fact flattened.
> It was suggested that this would be more robust if this was folded into the
> construction of the FlatFieldPayload construction.
>
> I moved these asserts up the construction hierarchy so that we also verify
> the unsafe flat field access. Because these can access nested flat values,
> some extra utility was added to find nested flat fields from an offset.
>
> After this change the `FlatFieldPayload` check that the `fieldDescriptor*`
> constructor and the `ResolvedFieldEntry*` is a valid non-nested flat field in
> the containing object.
>
> And the `FlatValuePayload::construct_from_parts` verifies that there is an
> field at that offset of of the correct InlineKlass. (Either a direct field or
> array element, or a nested flat field inside an flat field or array element)
>
> _Also the unsafe access logging was missing a ResourceMark, and changed it so
> it does not assert on bad offset, as I moved the assertions into the
> FlatValuePayload construction._
>
> Testing:
> * Running tier 1 - 4 with `--enable-preview`
> * Running all Valhalla tests
src/hotspot/share/oops/instanceKlass.cpp line 2212:
> 2210: }
> 2211:
> 2212: bool InstanceKlass::find_local_flat_field_from_offset(int offset,
> fieldDescriptor* fd) const {
The name of this method is confusing. The legacy
`InstanceKlass::find_local_field_from_offset()` returns the information of the
field at a given offset, but the new
`InstanceKlass::find_local_flat_field_from_offset()` returns either the flat
field at a given offset or the flat field declared in the current class that
encloses the specified offset. This dual behavior would deserve a comment to
make it more explicit.
The name of the method could also be improved, to make it more distinct than
the legacy `find_local_field_from_offset()`. May be
`InstanceKlass::find_local_flat_field_containing_offset()` ?
Other than that, the method's implementation looks fine, and it's usage in the
recursive method ValuePayload::assert_is_flat_field() looks fine too.
-------------
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2186#discussion_r2880563190
