[lworld] RFR: 8377451: [lworld] Add ValuePayload abstraction

Mon Feb 9 22:05:13 UTC 2026

On Mon, 9 Feb 2026 11:29:57 GMT, Stefan Karlsson <stefank at openjdk.org> 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