On Mon, 9 Feb 2026 13:51:58 GMT, Axel Boldt-Christmas <[email protected]>
wrote:
>> 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.
Which I know, the offset is not an offset it is an address there. Maybe we
should rename it. The holder + offset, should probably just be a tagged union.
And have the is_raw property just be the tag.
I will look at doing this.
>> 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.
And I am not convinced that we have the correct synch primitives everywhere for
the immutable invariant to hold. But I think that the the immutable invariant
is the intended design.
If it was not and we are allowed to observe the pre-copy / pre-construction
contents of a buffered object I think both this and the previous implementation
is broken w.r.t. concurrent reads.
-------------
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782775162
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782847142
