[lworld] RFR: 8341757: Field layout computation allowing atomic and nullable flattening

[Coleen Phillimore]

On Fri, 11 Oct 2024 13:45:46 GMT, Frederic Parain <fparain at openjdk.org> wrote:

> Initial implementation of fields layouts in order to support heap flattening with JEP 401 model.
> The patch includes the generation of two new flat fields layouts: atomic and nullable.
> The patch also includes the support in the interpreter (through the access API) to access the new layouts while respecting the new semantic (on x86_64 and aarch64).
> Some areas do not support the new layouts yet, like JITs, Unsafe and arrays.
> 
> There's no automatic tests yet, but considering the impact those new layouts will have on the JVM, the priority was to make this code available to all developers as soon as possible. Automatic tests will be added in a following CR.
> 
> Fred

This is sort of a surface review of all but the actual field layout part (the meat of the change) until I ran out of steam.

src/hotspot/cpu/aarch64/gc/shared/barrierSetAssembler_aarch64.cpp line 155:

> 153:   // "short-cuts" to be made from asm. What there is, appears to have the same
> 154:   // cost in C++, so just "call_VM_leaf" for now rather than maintain hundreds
> 155:   // of hand-rolled instructions...

Never apologize for not writing assembly!

src/hotspot/share/c1/c1_Runtime1.cpp line 521:

> 519:   } else {
> 520:     assert(array->klass()->is_flatArray_klass(), "should not be called");
> 521:     array->value_copy_to_index(value, index, LayoutKind::PAYLOAD); // Non atomic is currently the only layout supported by flt arrays

s/flt/flat/

src/hotspot/share/classfile/fieldLayoutBuilder.hpp line 55:

> 53: //
> 54: 
> 55: #define MAX_ATOMIC_OP_SIZE sizeof(jlong)

We've been avoiding Java types where possible (not consistently).  Can you change to uint64_t?

src/hotspot/share/classfile/fieldLayoutBuilder.hpp line 348:

> 346: 
> 347:   int null_marker_offset() const { return _null_marker_offset; }
> 348:   bool is_empty_inline_class() const { return _is_empty_inline_class; }

Nit, it would look nice if you lined up {'s here, as we do in other places that are functions like this.

src/hotspot/share/gc/shared/barrierSetRuntime.cpp line 38:

> 36: 
> 37: JRT_LEAF(void, BarrierSetRuntime::value_copy2(void* src, void* dst, InlineLayoutInfo* li))
> 38:   HeapAccess<>::value_copy(src, dst, li->klass(), li->kind());

This can't be an overload because it's called from macro assembler?  maybe value_copy_with_layout ? That's not too long...
Or are you going to remove the first when the compiler and unsafe support is implemented. This is fine then.

src/hotspot/share/interpreter/interpreterRuntime.cpp line 283:

> 281:       InlineLayoutInfo* li = klass->inline_layout_info_adr(index);
> 282:       li->set_klass(InlineKlass::cast(field_k));
> 283:       li->set_kind(LayoutKind::REFERENCE);

Do you always set these together?  Might be good to have one function like fill_in() that fills both values in.

src/hotspot/share/oops/inlineKlass.cpp line 85:

> 83:   *((int*)adr_atomic_size_in_bytes()) = -1;
> 84:   *((int*)adr_nullable_size_in_bytes()) = -1;
> 85:   *((int*)adr_null_marker_offset()) = -1;

hm don't know what this is.

src/hotspot/share/oops/instanceKlass.hpp line 191:

> 189:   static ByteSize klass_offset() { return in_ByteSize(offset_of(InlineLayoutInfo, _klass)); }
> 190:   static ByteSize null_marker_offset_offset() { return in_ByteSize(offset_of(InlineLayoutInfo, _null_marker_offset)); }
> 191: 

Should this be in inlineKlass.hpp ?  Is that included here?  It seems better there and then #include that.
Edit: it is complicated though isn't it?  Maybe it is better here but I don't like extra classes in header files, especially this one since it's sort of key.  Maybe it's better in its own header file?

-------------

PR Review: https://git.openjdk.org/valhalla/pull/1275#pullrequestreview-2378835003
PR Review Comment: https://git.openjdk.org/valhalla/pull/1275#discussion_r1806936155
PR Review Comment: https://git.openjdk.org/valhalla/pull/1275#discussion_r1806937058
PR Review Comment: https://git.openjdk.org/valhalla/pull/1275#discussion_r1806940480
PR Review Comment: https://git.openjdk.org/valhalla/pull/1275#discussion_r1806942549
PR Review Comment: https://git.openjdk.org/valhalla/pull/1275#discussion_r1806954345
PR Review Comment: https://git.openjdk.org/valhalla/pull/1275#discussion_r1806965794
PR Review Comment: https://git.openjdk.org/valhalla/pull/1275#discussion_r1806970340
PR Review Comment: https://git.openjdk.org/valhalla/pull/1275#discussion_r1806974674