On Wed, 9 Sep 2026 19:09:14 GMT, Ioi Lam <[email protected]> wrote:
>> `FieldClosure` is used for iterating fields in an object. For Valhalla, it
>> has been enhanced to handle fields that are inside inlined fields. However,
>> the current implementation has two problems:
>>
>> [1] It type casts the address of an inlined field into an `oop` pointer.
>> This is unsafe as many operations, such as getting the header of an `oop`,
>> will not work with such an `oop` pointer:
>>
>> https://github.com/openjdk/jdk/blob/b1f975efa481bd5e20c1b7d58a87d0866df205e6/src/hotspot/share/runtime/fieldDescriptor.cpp#L216
>>
>> [2] The parameter `base_offset` is used in many functions. Its meaning is
>> unclear and inconsistent.
>>
>> https://github.com/openjdk/jdk/blob/b1f975efa481bd5e20c1b7d58a87d0866df205e6/src/hotspot/share/runtime/fieldDescriptor.cpp#L159
>>
>> https://github.com/openjdk/jdk/blob/b1f975efa481bd5e20c1b7d58a87d0866df205e6/src/hotspot/share/oops/instanceKlass.hpp#L99-L101
>>
>> This RFE refactors `FieldClosure` to avoid the above problems.
>> `FieldClosure` now carries information about inlined fields. This
>> information can be used by various field iteration code to simplify their
>> operations. See `FieldClosure::inline_klass()` and
>> `FieldClosure::inline_offset()`.
>>
>> As a result, users of `FieldClosure` and `FieldDescriptor()` no longer need
>> to perform obscure arithmetics with `InlineKlass::payload_offset()`.
>>
>> This RFE also moves a few common operations into utility functions to avoid
>> code duplication.
>>
>> Also:
>> - Fixed a bug in `FlatArrayKlass::oop_print_elements_on()` in the handling
>> of nullable elements.
>>
>> ---------
>> - [x] I confirm that I make this contribution in accordance with the
>> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai).
>
> Ioi Lam has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - clean up: ValuePayloadContext::_klass is used only in asserts
> - Alternative
Looks good! Leaving some notes from my first pass.
src/hotspot/share/cds/aotMapLogger.cpp line 1050:
> 1048: st->cr();
> 1049: ValuePayloadContext vpc{elem_k, elem_offset};
> 1050: ArchivedFieldPrinter print_field(fake_flat_array, st, 0, &vpc);
Indentation is not preserved from before your change. Previously indentation
started at 1 so fields were indented one level from the holder.
src/hotspot/share/runtime/fieldDescriptor.cpp line 314:
> 312: if (obj == nullptr) {
> 313: assert(vpc == nullptr, "flattening not supported for static fields");
> 314: } else {
This can be `else if`
-------------
Changes requested by matsaave (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/32565#pullrequestreview-5159775213
PR Review Comment: https://git.openjdk.org/jdk/pull/32565#discussion_r3973035569
PR Review Comment: https://git.openjdk.org/jdk/pull/32565#discussion_r3972944342
