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

[Frederic Parain]

On Mon, 21 Oct 2024 15:11:50 GMT, Dan Heidinga <heidinga 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?

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

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

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

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

> src/hotspot/share/interpreter/interpreterRuntime.cpp line 317:
> 
>> 315: 
>> 316:   InlineLayoutInfo* layout_info = holder->inline_layout_info_adr(entry->field_index());
>> 317:   InlineKlass* field_vklass = layout_info->klass();
> 
> Why `vklass`?  Valhalla Klass?

Value Klass. InstanceKlass pointers are often named ik, but now those two letters can be interpreted as either InstanceKlass or InlineKlass. So, I often use vk or vklass to designate a pointer to an InlineKlass.

> src/hotspot/share/oops/instanceKlass.cpp line 1374:
> 
>> 1372:                 CLEAR_PENDING_EXCEPTION;
>> 1373:             }
>> 1374:             THROW_OOP(e());
> 
> If I understand this code correctly, an OutOfMemoryError here will make the class unusable, which is a bit surprising.  Is it possible to instead disable all layouts except the REFERENCE if an OOM is thrown here?

Only the NULLABLE_ATOMIC_FLAT layout would need to be disabled. But if an OOM is thrown at this stage (class initialization), does it really worth the effort to try to recover the class? It is unlikely the application will be able to allocate an instance of this class anyway.

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

PR Comment: https://git.openjdk.org/valhalla/pull/1275#issuecomment-2429399611
PR Review Comment: https://git.openjdk.org/valhalla/pull/1275#discussion_r1809251379
PR Review Comment: https://git.openjdk.org/valhalla/pull/1275#discussion_r1809253832
PR Review Comment: https://git.openjdk.org/valhalla/pull/1275#discussion_r1809262373
PR Review Comment: https://git.openjdk.org/valhalla/pull/1275#discussion_r1809282536
PR Review Comment: https://git.openjdk.org/valhalla/pull/1275#discussion_r1809363690
PR Review Comment: https://git.openjdk.org/valhalla/pull/1275#discussion_r1810874464