On Thu, 10 Sep 2026 17:12:27 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:
>
> @stefank and @matias9927 comments
I've left a couple of nits and a few comments, but overall I think this looks
good, so consider this reviewed from my side.
src/hotspot/share/oops/flatArrayKlass.cpp line 433:
> 431: st->print_cr(" - Index %3d offset %3d:", index, off);
> 432: if (!fa->is_null_free_array() && fa->obj_at_is_null(index)) {
> 433: st->print_cr(" - (null)");
Does this also have to be indented now that you indent the output in the else
branch?
src/hotspot/share/runtime/fieldDescriptor.cpp line 323:
> 321: void FieldPrinter::do_field(fieldDescriptor* fd) {
> 322: for (int i = 0; i < _indent; i++) _st->print(" ");
> 323: _st->print(" - ");
This used to use `BULLET` instead of ` - ` are we transitioning away from using
`BULLET`. Should a follow-up RFE remove its usage?
-------------
Marked as reviewed by stefank (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/32565#pullrequestreview-5171300957
PR Review Comment: https://git.openjdk.org/jdk/pull/32565#discussion_r3982648739
PR Review Comment: https://git.openjdk.org/jdk/pull/32565#discussion_r3982670388
