On Thu, 10 Sep 2026 10:26:57 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 one additional commit
> since the last revision:
>
> fixed whitespaces
src/hotspot/share/cds/aotMapLogger.cpp line 886:
> 884: }
> 885: } else {
> 886: // Was this a bug before this change?
I added this as a marker to show that there was a bug here. It should be
removed before integration:
Suggestion:
src/hotspot/share/cds/heapShared.hpp line 368:
> 366:
> 367: class OopFieldPusher;
> 368: class FlatFieldKlassFinder;
Shouldn't this be removed?
Suggestion:
src/hotspot/share/oops/flatArrayKlass.cpp line 436:
> 434: } else {
> 435: ValuePayloadContext vpc{vk, fa->value_offset_as_int(index,
> layout_helper())};
> 436: FieldPrinter print_field(st, fa, /*indent*/0, &vpc);
I guess this is a preference, but maybe let everything breath a little ...
Suggestion:
FieldPrinter print_field(st, fa, /* indent */ 0, &vpc);
src/hotspot/share/runtime/fieldDescriptor.cpp line 174:
> 172:
> 173: // Print information (such as type, name, offset) of this field.
> 174: void fieldDescriptor::print_on(outputStream* st, const
> ValuePayloadContext* vpc) const {
FWIW, I had a version of this that skipped passing down a vpc. Instead it sent
in an optional "offset_override", which brought down the
`this->field_offset_in_obj(vpc);` calculation from `print_on_for`. This allows
you to make `print_on` vpc agnostic. Just food for thought.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/32565#discussion_r3978442479
PR Review Comment: https://git.openjdk.org/jdk/pull/32565#discussion_r3978472286
PR Review Comment: https://git.openjdk.org/jdk/pull/32565#discussion_r3978488219
PR Review Comment: https://git.openjdk.org/jdk/pull/32565#discussion_r3978559435
