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

Wed Oct 23 12:16:28 UTC 2024

On Tue, 22 Oct 2024 14:10:05 GMT, Frederic Parain <fparain at openjdk.org> wrote:

> > General question - does the Valhalla CDS support need to be updated to handle the Klass pointers in InlineLayoutInfo when this is merged?
> 
> The InstanceKlass' metaspace_pointers_do() method has been updated to process the _inline_layout_info_array, and the InlineLayoutInfo class has its own metaspace_pointers_do() method to process its _klass pointer. Did I miss something?

No, I missed `it->push(&_inline_layout_info_array, MetaspaceClosure::_writable);` in the instanceKlass.  Thanks for confirming

>> src/hotspot/share/classfile/fieldLayoutBuilder.cpp line 933:
>> 
>>> 931:         // Presence of a must_be_atomic field must revert the _must_be_atomic flag of the holder to true
>>> 932:         if (vk->must_be_atomic()) {
>>> 933:           _must_be_atomic = true;
>> 
>> Is this something we want to log so it's clear why a `@LooselyConsistent` class is forced to be atomic?
>
> So far, we have avoided logging individual layout decisions. Only the final result is available as a diagnostic feature. If we start with this one, we could have many other decisions to add to the logging.

Fair enough.  This comment can be resolved.

>> src/hotspot/share/classfile/fieldLayoutBuilder.cpp line 1119:
>> 
>>> 1117: 
>>> 1118:   // Determining if the value class is naturally atomic:
>>> 1119:   if ((!_layout->super_has_fields() && _declared_non_static_fields_count <= 1 && !_has_non_naturally_atomic_fields)
>> 
>> Suggestion:
>> 
>>   if ((!_layout->super_has_fields() && _declared_non_static_fields_count == 1 && !_has_non_naturally_atomic_fields)
>> 
>> 
>> Should the check be `==1` rather than `<=1`?  We don't support empty values (we inject a field)
>
> Empty abstract values have no instance fields. The ".empty" field is only added to concrete value classes.

Thanks for confirming.  This can be resolved,

>> src/hotspot/share/classfile/fieldLayoutBuilder.hpp line 67:
>> 
>>> 65:     FLAT,                  // flat field
>>> 66:     INHERITED,             // field(s) inherited from super classes
>>> 67:     NULL_MARKER            // stores the null marker for a flat field
>> 
>> Does this mean we only allocate a `NULL_MARKER` for non-abstract value classes?  I think that makes sense but confirmation would be good.  This may affect future opts for sealed hierarchies as the null channel may be in different places for each class.
>
> Correct. The null-marker is only added to concrete value classes. In fact, all the flat layouts are computed only for non-abstract value classes. If this is going to be an issue in the future, we should discuss another design before pushing those changes.

We have hypothesized that sealed hierarchies may be a target for future flattening optimizations as users will target the sealed type (typically an interface) for use as fields in their classes with a limited number (sometimes one!) of implementations.

These kinds of optimizations will require additional book-keeping for tracking the null marker, determining the klass of the value, and maybe additional info.  I don't think we want to delay this work for those future optimizations.... we can figure them out under a future RFE

>> src/hotspot/share/classfile/javaClasses.cpp line 3785:
>> 
>>> 3783: 
>>> 3784: #define BOXING_FIELDS_DO(macro) \
>>> 3785:   macro(_value_offset,      byteKlass, "value", byte_signature, false); \
>> 
>> Is this a Valhalla change?  I'm not sure I understand this change
>
> JavaClasses::check_offsets() verifies the offset of the boxing classes. The code used to handle two offsets, one taken rom j.l.Integer and used for Byte, Char, Short, Int and Float, and another one taken from j.l.Long for Long and Double. If you think of an object with compress class pointer, the first group has its field aligned on a 32bits boundary, and the second group has a field aligned on a 64bits boundary. The addition of ATOMIC_NULLABLE_FLAT layout changes of the layout of j.l.Integer and j.l.Float which now must have their field aligned on a 64bits boundary to be copiable atomically with their null marker. The new code now uses the j.l.Byte class as a reference for Byte, Char and Short, and has special cases to handle j.l.Integer and j.l.Float.

Sorry, I may be slow today but I would expect j.l.Byte and j.l.Short/Char to have different field layouts - given a 12byte header, the `value` field of j.l.Byte can start at the first free byte.  Using hand-hacked versions of the print fields output:

 @0 RESERVED 12/-
 @12 REGULAR 1/1 "value" B

and the `value` field of a j.l.Short needs additional alignment:

 @0 RESERVED 12/-
 @12 PADDING 4/1
 @16 REGULAR 2/2 "value" S

What am I missing?

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

PR Comment: https://git.openjdk.org/valhalla/pull/1275#issuecomment-2429471876
PR Review Comment: https://git.openjdk.org/valhalla/pull/1275#discussion_r1809271732
PR Review Comment: https://git.openjdk.org/valhalla/pull/1275#discussion_r1809292409
PR Review Comment: https://git.openjdk.org/valhalla/pull/1275#discussion_r1809270848
PR Review Comment: https://git.openjdk.org/valhalla/pull/1275#discussion_r1809324644