On Mon, 9 Feb 2026 11:29:57 GMT, Stefan Karlsson <[email protected]> wrote:
>> Create a wrapper class which represents the payload of an InlineKlass object.
>>
>> The current convention is to use a `void*` representing where the payload
>> starts, the `InlineKlass*` (as we do not always have a header when
>> flattened) and a `LayoutKind` (describing the payloads layout).
>>
>> I suggest we introduce something like a `ValuePayload` which encapsulate
>> these properties. As well as a hierarchy built upon these, with the proper
>> interfaces implemented.
>> * ValuePayload (Any payload)
>> * RawValuePayload (Payload with holder erased)
>> * BufferedValuePayload (Payload of normal heap object)
>> * FlatValuePayload (Payload of flattened value)
>> * FlatFieldPayload (Payload of flattened field)
>> * FlatArrayPayload (Payload of flattened array element)
>>
>> The goal is to both make interfaces clearer, and easier to understand. As
>> well as consolidating the implementation in one place rather than spread
>> across different subsystems.
>>
>> Each type (except RawValuePayload) also allows for the creation of a Handle,
>> (thread local, or in an OopStorage) for keeping the payload as a thread or
>> global root.
>>
>> The ValuePayload class is also the interface for interacting with the Access
>> API for InlineKlass objects.
>>
>> * Testing
>> * Running tier 1-4 with preview enabled
>> * Running app tests with preview enabled
>> * Running normal tier 1-5
>>
>> #### _Extra Notes:_
>> * The `OopHandle` type is there so that we can migrate the JVMTI payload
>> abstraction implementation to using this instead. (Future RFE)
>> * Some interfaces got cleaned up. Some are unused. Like the `null_payload`
>> which was superseded by the `Access::value_store_null`. C1 still uses the
>> `.null_reset` but if that dependency is removed we should be able to remove
>> that weird object all together.
>> * Simply adding the Java to VM transition deep inside the payload code
>> created a circular include dependency here. So rather than fixing that, I
>> implemented the relevant bytecodes in the BytecodeInterpreter.
>
> src/hotspot/share/oops/flatArrayKlass.cpp line 311:
>
>> 309: THROW(vmSymbols::java_lang_NullPointerException());
>> 310: }
>> 311: dst_payload.copy_from_non_null(buf_payload);
>
> Follow-up: It is unclear to me why we don't need to adhere to the
> `needs_backwards_copy` in this branch.
Added a comment
> src/hotspot/share/oops/inlineKlassPayload.hpp line 47:
>
>> 45: private:
>> 46: template <typename Holder> struct StorageImpl {
>> 47: mutable Holder _holder;
>
> I'm a little bit unsure about using `holder` as a name for the container
> object. In the runtime code `holder` often means the class that keeps
> metadata alive. Could this be `_object`, `_base`, `_container`, or something
> like that?
>
> Maybe `is_raw` could also get a name that makes the connection between this
> property and the `holder`?
Hmm. I agree. I see pros and cons. Let us see if we can discuss this one
offline as well.
> src/hotspot/share/oops/inlineKlassPayload.hpp line 117:
>
>> 115:
>> 116: protected:
>> 117: using ValuePayload::ValuePayload;
>
> Why is this needed?
To get the `construct_from_parts` constructor. I decided to add
`construct_from_parts` to all types as part of its interface.
But it is really only there for the `Unsafe_[Get|Put]FlatValue`. So might only
be something we want on the `FlatValuePayload`.
The idea is that the `construct_from_parts` in the JVMTI code can be removed in
a follow-up RFE.
> src/hotspot/share/oops/inlineKlassPayload.inline.hpp line 184:
>
>> 182: dst.get_layout_kind() == LayoutKind::BUFFERED;
>> 183: const bool src_and_dst_same_layout_kind =
>> 184: src.get_layout_kind() == dst.get_layout_kind();
>
> Is your editor set to line breaks at 80 characters? There seems to be a lot
> of line breaks in the patch?
Yeah, did to much copy pasting and renaming (especially with in the start when
everything was templates). So just used the following `.clang-format` file. I
do not mind the 80 char line breaks. But we can take a pass offline and cleanup
whatever style you think is appropriate.
BasedOnStyle: LLVM
PointerAlignment: Left
ReferenceAlignment: Pointer
> src/hotspot/share/oops/inlineKlassPayload.inline.hpp line 222:
>
>> 220: inline address ValuePayload::get_address() const {
>> 221: return
>> reinterpret_cast<address>(cast_from_oop<intptr_t>(get_holder()) +
>> 222: _storage._offset);
>
> Could this be just:
> Suggestion:
>
> return cast_from_oop<address>(get_holder()) + _storage._offset;
In the `RawValuePayload` case the holder is `nullptr`, we would get `nullptr` +
non-zero offset UB.
Probably worth a comment.
> src/hotspot/share/oops/inlineKlassPayload.inline.hpp line 280:
>
>> 278:
>> 279: // We may have copied a null payload.
>> 280: return !dst.is_payload_null();
>
> Could you explain why this isn't depending on the value of
> `has_null_marker()`?
`is_payload_null()` checks `has_null_marker()`. Something with
`!has_null_marker()` can never be null. So if `has_null_marker() == false`,
`is_payload_null() == false`.
Maybe there is a more consistent naming scheme / way of doing this which causes
less confusion.
> src/hotspot/share/oops/inlineKlassPayload.inline.hpp line 289:
>
>> 287: // non null. It is the callers responsibility to ensure that this
>> is a
>> 288: // valid non null value.
>> 289: src.mark_as_non_null();
>
> Is it guaranteed that only one thread is accessing the `src` here? Maybe more
> of a question to @fparain
Probably good to double check. I do not think we assume the invariant that this
is not a concurrently accessed object.
My assumption is that `BufferedValuePayload` have an immutable payload, only
the null marker may be indeterminate. A buffered payload cannot be transformed
from a valid object -> null.
With the removal of the private buffer, (which now instead uses a FlatArray of
length 1) we should not have mutable buffered objects out in Java. Only mutable
flat fields and flat array elements. But they do not have the immutability
assumption.
> src/hotspot/share/prims/jvm.cpp line 461:
>
>> 459: assert(to >= 0 && from >= 0, "Assume this for now");
>> 460: int org_length = org->length();
>> 461: int copy_len = MIN2(to, org_length) - MIN2(from, org_length);
>
> This code is non-obvious. Could you add a comment or write it so that it is
> clearer what this does?
It is good that you remained me. This was one piece of code I wanted others
input on. I am not sure about this code at all. I could not really understand
how the previous implementation was correct. Nor exactly what we expect the
outcome to be. It seems like the input indices are allowed to be out of bounds?
And `from` > `end`. I really hoped they were not expected to also sometimes be
negative.
But hopefully someone in the hotspot / lang tools knows what is the expected
behaviour of `CopyOfSpecialArray` and maybe we would rewrite the whole function
to be more clear.
-------------
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782615904
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782635805
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782604865
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782655527
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782762345
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782804886
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782829681
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782599661
