On Mon, 9 Feb 2026 09:35:48 GMT, Axel Boldt-Christmas <[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.
Here's a first set of comments. I've marked some of the comments with
`Follow-up`. Those are things that probably shouldn't be fixed with this PR,
but something I'd like to revisit as soon as this has been integrated.
Next set of comments:
src/hotspot/share/gc/shared/cardTableBarrierSet.inline.hpp line 169:
> 167: template <DecoratorSet decorators, typename BarrierSetT>
> 168: inline void CardTableBarrierSet::AccessBarrier<decorators, BarrierSetT>::
> 169: value_copy_in_heap(const ValuePayload& src, const ValuePayload& dst) {
After this change I don't think this comment makes that much sense anymore and
should probably be updated. This comment occurs at multiple places in the code.
// src/dst aren't oops, need offset to adjust oop map offset
src/hotspot/share/gc/shared/cardTableBarrierSet.inline.hpp line 178:
> 176: BarrierSetT* bs =
> barrier_set_cast<BarrierSetT>(BarrierSet::barrier_set());
> 177: // src/dst aren't oops, need offset to adjust oop map offset
> 178: const address dst_oop_addr_offset = (dst.get_address()) -
> md->payload_offset();
Suggestion:
const address dst_oop_addr_offset = dst.get_address() -
md->payload_offset();
src/hotspot/share/gc/shared/cardTableBarrierSet.inline.hpp line 223:
> 221: OopMapBlock* map = md->start_of_nonstatic_oop_maps();
> 222: OopMapBlock* const end = map + md->nonstatic_oop_map_count();
> 223: bool is_uninitialized = HasDecorator<decorators,
> IS_DEST_UNINITIALIZED>::value;
Follow-up: If destination is uninitialized we don't need to execute the
pre-barrier.
src/hotspot/share/gc/shared/cardTableBarrierSet.inline.hpp line 225:
> 223: bool is_uninitialized = HasDecorator<decorators,
> IS_DEST_UNINITIALIZED>::value;
> 224: while (map != end) {
> 225: address doop_address = dst_oop_addr_offset + map->offset();
Follow-up: `dst_oop_addr_offset` isn't an offset but an address and should
probably be renamed.
src/hotspot/share/gc/shared/cardTableBarrierSet.inline.hpp line 233:
> 231: }
> 232:
> 233: Raw::value_store_null(dst);
Follow-up: I don't think a post-barrier is needed when storing null.
src/hotspot/share/gc/z/zBarrierSet.inline.hpp line 356:
> 354: store_barrier_heap_without_healing(dst);
> 355:
> 356: AtomicAccess::store(dst, color_null());
The comment looks like it refers to the next line. I'd suggest:
Suggestion:
// Store barrier
store_barrier_heap_without_healing(dst);
// Store colored null
AtomicAccess::store(dst, color_null());
src/hotspot/share/gc/z/zBarrierSet.inline.hpp line 480:
> 478: }
> 479:
> 480: void* dst_payload = (void*)(address(dst) + copied_bytes);
Follow-up: constify locals and consider not having the side-effect of updating
`copied_bytes` in this function.
src/hotspot/share/gc/z/zBarrierSet.inline.hpp line 487:
> 485: template <DecoratorSet decorators, typename BarrierSetT>
> 486: inline void ZBarrierSet::AccessBarrier<decorators,
> BarrierSetT>::value_copy_in_heap(const ValuePayload& src, const ValuePayload&
> dst) {
> 487: precond(src.get_klass() == dst.get_klass());
Suggestion:
precond(src.get_klass() == dst.get_klass());
src/hotspot/share/gc/z/zBarrierSet.inline.hpp line 490:
> 488: const LayoutKind lk = LayoutKindHelper::get_copy_layout(
> 489: src.get_layout_kind(), dst.get_layout_kind());
> 490: const InlineKlass* md = src.get_klass();
Suggestion:
const LayoutKind lk =
LayoutKindHelper::get_copy_layout(src.get_layout_kind(), dst.get_layout_kind());
const InlineKlass* md = src.get_klass();
src/hotspot/share/gc/z/zBarrierSet.inline.hpp line 493:
> 491: if (md->contains_oops()) {
> 492: assert(!LayoutKindHelper::is_atomic_flat(lk),
> 493: "ZGC cannot handle atomic flat values");
Suggestion:
assert(!LayoutKindHelper::is_atomic_flat(lk), "ZGC cannot handle atomic
flat values");
src/hotspot/share/gc/z/zBarrierSet.inline.hpp line 537:
> 535: assert(!LayoutKindHelper::is_null_free_flat(lk),
> 536: "ZGC cannot handle atomic flat values");
> 537: const InlineKlass* md = dst.get_klass();
Suggestion:
const LayoutKind lk = dst.get_layout_kind();
assert(!LayoutKindHelper::is_null_free_flat(lk), "ZGC cannot handle atomic
flat values");
const InlineKlass* md = dst.get_klass();
src/hotspot/share/gc/z/zBarrierSet.inline.hpp line 540:
> 538: if (md->contains_oops()) {
> 539: assert(!LayoutKindHelper::is_atomic_flat(lk),
> 540: "ZGC cannot handle atomic flat values");
Suggestion:
assert(!LayoutKindHelper::is_atomic_flat(lk), "ZGC cannot handle atomic
flat values");
src/hotspot/share/gc/z/zBarrierSet.inline.hpp line 546:
> 544: // 3) possibly raw clear for any primitive payload trailer
> 545:
> 546: // dst may not be oops, need offset to adjust oop map offset
This comment needs some tweaking
src/hotspot/share/gc/z/zBarrierSet.inline.hpp line 548:
> 546: // dst may not be oops, need offset to adjust oop map offset
> 547: const address dst_addr = dst.get_address();
> 548: const address dst_oop_addr_offset = (dst_addr) -
> md->payload_offset();
Suggestion:
const address dst_oop_addr_offset = dst_addr - md->payload_offset();
src/hotspot/share/gc/z/zBarrierSet.inline.hpp line 554:
> 552: size_t copied_bytes = 0;
> 553: while (map != end) {
> 554: zpointer *dst_p = (zpointer*)(dst_oop_addr_offset + map->offset());
Suggestion:
zpointer* dst_p = (zpointer*)(dst_oop_addr_offset + map->offset());
src/hotspot/share/interpreter/interpreterRuntime.cpp line 50:
> 48: #include "oops/flatArrayKlass.hpp"
> 49: #include "oops/flatArrayOop.inline.hpp"
> 50: #include "oops/inlineKlass.hpp"
Suggestion:
src/hotspot/share/interpreter/interpreterRuntime.cpp line 238:
> 236: JRT_BLOCK_ENTRY(void, InterpreterRuntime::read_flat_field(JavaThread*
> current, oopDesc* obj, ResolvedFieldEntry* entry))
> 237: assert(oopDesc::is_oop(obj), "Sanity check");
> 238: #ifdef ASSERT
Follow-up: I wonder if this assert could be folded into FlatFieldPayload.
src/hotspot/share/interpreter/interpreterRuntime.cpp line 266:
> 264:
> 265: FlatFieldPayload payload(instanceOop(obj), entry);
> 266: payload.write(inlineOop(value), CHECK);
`obj_h` and `val_h` seems unused now.
src/hotspot/share/oops/access.inline.hpp line 32:
> 30: #include "gc/shared/barrierSet.inline.hpp"
> 31: #include "gc/shared/barrierSetConfig.inline.hpp"
> 32: #include "oops/accessBackend.hpp"
Suggestion:
src/hotspot/share/oops/flatArrayKlass.cpp line 272:
> 270: // Because source and destination have the same layout, we do
> not have
> 271: // to worry about null checks and atomicity problems and can
> call the
> 272: // AccessAPI directly.
Suggestion:
// Access API directly.
src/hotspot/share/oops/flatArrayKlass.cpp line 291:
> 289: src_payload.get_handle(THREAD);
> 290: FlatArrayPayload::Handle dst_payload_handle =
> 291: dst_payload.get_handle(THREAD);
Suggestion:
FlatArrayPayload::Handle src_payload_handle =
src_payload.get_handle(THREAD);
FlatArrayPayload::Handle dst_payload_handle =
dst_payload.get_handle(THREAD);
src/hotspot/share/oops/flatArrayKlass.cpp line 297:
> 295:
> 296: src_payload = src_payload_handle();
> 297: dst_payload = dst_payload_handle();
Maybe add comment why this is done.
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.
src/hotspot/share/oops/flatArrayOop.inline.hpp line 42:
> 40: inline size_t flatArrayOopDesc::base_offset_in_bytes() {
> 41: return
> static_cast<size_t>(arrayOopDesc::base_offset_in_bytes(T_FLAT_ELEMENT));
> 42: }
Follow-up: This adds some inconsistency between the array klasses when this
class now returns size_t while the others return int.
src/hotspot/share/oops/inlineKlass.hpp line 319:
> 317: void oop_verify_on(oop obj, outputStream* st) override;
> 318:
> 319: void print_on(outputStream* st) const override;
`print_on` and `oop_print_on` tend to be placed before the Verification section.
src/hotspot/share/oops/inlineKlass.inline.hpp line 31:
> 29: #include "oops/inlineKlassPayload.inline.hpp"
> 30: #include "oops/layoutKind.hpp"
> 31: #include "oops/oopsHierarchy.hpp"
These are brought in via the InlineKlass interface and therefore doesn't have
to be included here.
Suggestion:
src/hotspot/share/oops/inlineKlass.inline.hpp line 37:
> 35:
> 36: inline bool InlineKlass::layout_has_null_marker(LayoutKind lk) const {
> 37: assert(is_layout_supported(lk), "Must do");
Suggestion:
assert(is_layout_supported(lk), "Must be");
src/hotspot/share/oops/inlineKlassPayload.hpp line 25:
> 23: */
> 24:
> 25: #ifndef SHARE_VM_OOPS_INLINEKLASSPAYLOAD_HPP
Follow-up: Figure out if this should be valuePayload.hpp. Might lead to the
discussion about renaming `InlineKlass` to `ValueKlass`, so it might be good to
take after integration of this PR, outside of this PR.
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`?
src/hotspot/share/oops/inlineKlassPayload.hpp line 68:
> 66: inline ValuePayload(oop holder, InlineKlass* klass, ptrdiff_t offset,
> 67: LayoutKind layout_kind
> 68: DEBUG_ONLY(COMMA bool is_raw = false));
To make it easier to read:
Suggestion:
inline ValuePayload(oop holder,
InlineKlass* klass,
ptrdiff_t offset,
LayoutKind layout_kind
DEBUG_ONLY(COMMA bool is_raw = false));
src/hotspot/share/oops/inlineKlassPayload.hpp line 93:
> 91: inline LayoutKind get_layout_kind() const;
> 92:
> 93: inline address get_address() const;
HotSpot tend to not use `get_` prefix. Maybe `address()` is problematic, but I
think `addr` could be used instead?
src/hotspot/share/oops/inlineKlassPayload.hpp line 117:
> 115:
> 116: protected:
> 117: using ValuePayload::ValuePayload;
Why is this needed?
src/hotspot/share/oops/inlineKlassPayload.inline.hpp line 24:
> 22: *
> 23: */
> 24: #ifndef SHARE_VM_OOPS_INLINEKLASSPAYLOAD_INLINE_HPP
Suggestion:
*/
#ifndef SHARE_VM_OOPS_INLINEKLASSPAYLOAD_INLINE_HPP
src/hotspot/share/oops/inlineKlassPayload.inline.hpp line 98:
> 96: }
> 97:
> 98: #ifdef ASSERT
The ASSERT belongs to multiple functions, so a blank line makes it more obvious.
Suggestion:
#ifdef ASSERT
src/hotspot/share/oops/inlineKlassPayload.inline.hpp line 138:
> 136:
> 137: inline void ValuePayload::assert_post_construction_invariants() const {
> 138: OnVMError on_assertion_failuire([&](outputStream* st) {
Suggestion:
OnVMError on_assertion_failure([&](outputStream* st) {
src/hotspot/share/oops/inlineKlassPayload.inline.hpp line 148:
> 146: postcond(get_klass()->is_layout_supported(get_layout_kind()));
> 147: postcond(get_layout_kind() != LayoutKind::REFERENCE &&
> 148: get_layout_kind() != LayoutKind::UNKNOWN);
Suggestion:
postcond(get_layout_kind() != LayoutKind::REFERENCE);
postcond(get_layout_kind() != LayoutKind::UNKNOWN);
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?
src/hotspot/share/oops/inlineKlassPayload.inline.hpp line 205:
> 203: copy_layout_kind);
> 204: }
> 205: #endif // ASSERT
Suggestion:
#endif // ASSERT
src/hotspot/share/oops/inlineKlassPayload.inline.hpp line 207:
> 205: #endif // ASSERT
> 206:
> 207: inline oop ValuePayload::get_holder() const { return _storage._holder; }
No need to optimize for space in the inline hpp file:
Suggestion:
inline oop ValuePayload::get_holder() const {
return _storage._holder;
}
src/hotspot/share/oops/inlineKlassPayload.inline.hpp line 209:
> 207: inline oop ValuePayload::get_holder() const { return _storage._holder; }
> 208:
> 209: inline InlineKlass* ValuePayload::get_klass() const { return
> _storage._klass; }
Suggestion:
inline InlineKlass* ValuePayload::get_klass() const {
return _storage._klass;
}
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;
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()`?
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
src/hotspot/share/oops/inlineKlassPayload.inline.hpp line 406:
> 404:
> 405: inline void FlatArrayPayload::previous_element() { advance_index(-1); }
> 406:
Suggestion:
inline void FlatArrayPayload::next_element() {
advance_index(1);
}
inline void FlatArrayPayload::previous_element() {
advance_index(-1);
}
src/hotspot/share/oops/inlineKlassPayload.inline.hpp line 420:
> 418:
> 419: const ptrdiff_t min_offset = base_offset - (length == 0 ? 0 :
> element_size);
> 420: const ptrdiff_t max_offset = base_offset + length * element_size;
`ptrdiff_t` could be messy on 32-bits. It could be worth adding an assert to
catch if the porters misses this.
src/hotspot/share/oops/oop.hpp line 133:
> 131: // type test operations (inlined in oop.inline.hpp)
> 132: inline bool is_instance() const;
> 133: inline bool is_inline() const;
Follow-up: This just becomes weird.
src/hotspot/share/oops/oopsHierarchy.hpp line 46:
> 44: typedef class instanceOopDesc* instanceOop;
> 45: typedef class inlineOopDesc* inlineOop;
> 46: typedef class stackChunkOopDesc* stackChunkOop;
Suggestion:
typedef class inlineOopDesc* inlineOop;
typedef class stackChunkOopDesc* stackChunkOop;
src/hotspot/share/prims/jni.cpp line 55:
> 53: #include "oops/arrayOop.hpp"
> 54: #include "oops/flatArrayOop.inline.hpp"
> 55: #include "oops/inlineKlass.hpp"
Suggestion:
src/hotspot/share/prims/jvm.cpp line 459:
> 457: }
> 458: if (org->is_flatArray()) {
> 459: assert(to >= 0 && from >= 0, "Assume this for now");
Suggestion:
assert(from >= 0, "Assume this for now");
assert(to >= 0, "Assume this for now");
Or add logging of values
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?
-------------
PR Review:
https://git.openjdk.org/valhalla/pull/2068#pullrequestreview-3772607617
PR Review:
https://git.openjdk.org/valhalla/pull/2068#pullrequestreview-3772998094
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2781966514
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2781959320
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2781976590
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2781979732
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2781982935
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2781999582
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782021827
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782022613
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782027770
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782028709
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782038237
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782039429
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782042328
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782043493
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782045057
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782047585
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782051291
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782055376
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782065132
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782080827
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782085945
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782092683
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782121225
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782130043
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782150575
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782156355
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782158091
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782172522
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782401283
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782178179
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782186475
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782196095
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782342586
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782416800
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782364968
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782371882
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782412026
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782417430
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782419937
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782421253
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782425673
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782446628
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782456182
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782474479
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782486615
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782216381
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782219171
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782222828
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782228149
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2782242744
