On Tue, 8 Sep 2026 23:21:13 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:
>
> @fparain comment: fixed null marker printing, added tests
This PR changes FieldClosure from being a pure C++ "interface" to also be a
carrier of data for iterators / visitors of fields. Messing with interfaces
like this is something I think we should refrain from doing, unless there's a
very strong reason to do so.
For example, when you look at this function:
void fieldDescriptor::print_on(outputStream* st, FieldClosure* fc) const {
you start to scratch your head wondering why a print function needs a field
closure. The same goes for other functions in fieldDescriptor.
It also fudges with all implementers that only wanted to implement the clean
FieldClosure interface, and not have the extra value klass adaption (E.g.
CompactStringsFixup).
I think that there's a relative easy change that can be made to keep
FieldClosure "clean", and I have a version of that here:
Diff:
https://github.com/stefank/jdk/compare/pull/32565...stefank:jdk:alt_32565
Branch:
https://github.com/openjdk/jdk/compare/master...stefank:jdk:alt_32565
The patch moves the fields added to FieldClosure out to its own struct, that I
call `ValuePayloadContext`. Whenever the code is descending into a flat
field/element, it sets up a new `ValuePayloadContext` to describe the current
value payload that is being iterated.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/32565#issuecomment-5604389366
