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

[Frederic Parain]

On Fri, 18 Oct 2024 19:48:26 GMT, Coleen Phillimore <coleenp 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
>
> 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.

This is temporary. The original value_copy() method is still there because of arrays. Once arrays have been updated to support the new flat layouts, value_copy() will be removed, and value_copy2() will be renamed value_copy().

> 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.

Not always. For instance fields, the class of those fields is usually known while parsing the class file (LoadableDescriptors attribute, @NullRestricted annotation), but the layout kind is only known after the field layout computation (post-processing of the parsed stream).

> 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.

Initialization of various fields for layout support in the InlineKlassFixedBlock. In the next patch, they are rewritten using setter methods.

> 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?

The InlineLayoutInfo array is a side data structure associated with InstanceKlass instances (any class with value fields needs this array, and this includes identity classes and abstract classes). This is why it is declared here, like the OopMapBlock. It could be declared in its own header file, but this would add a new file for just 30 lines of code.

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

PR Review Comment: https://git.openjdk.org/valhalla/pull/1275#discussion_r1809286465
PR Review Comment: https://git.openjdk.org/valhalla/pull/1275#discussion_r1809358529
PR Review Comment: https://git.openjdk.org/valhalla/pull/1275#discussion_r1809468512
PR Review Comment: https://git.openjdk.org/valhalla/pull/1275#discussion_r1809481070