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

Wed Oct 23 12:16:28 UTC 2024

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

General question - does the Valhalla CDS support need to be updated to handle the Klass pointers in InlineLayoutInfo when this is merged?

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

Suggestion:

    array->value_copy_to_index(value, index, LayoutKind::PAYLOAD); // Non atomic is currently the only layout supported by flat arrays

src/hotspot/share/classfile/classFileParser.cpp line 1566:

> 1564:     _temp_field_info->adr_at(idx)->set_index(idx);
> 1565:     _static_oop_count++;
> 1566:     // Inject static ".reset" field

I think we should rename this field to be something like `null_examplar` or `null_reset`.  
Suggestion:

    // Inject static ".null_examplar" field.  This is the same as the `.default field but will never have its null-channel set to non-zero.

src/hotspot/share/classfile/fieldLayoutBuilder.cpp line 723:

> 721:   _field_info(field_info),
> 722:   _info(info),
> 723:   // _inline_type_field_klasses(inline_type_field_klasses),

Remove?

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?

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)

src/hotspot/share/classfile/fieldLayoutBuilder.cpp line 1197:

> 1195:     // to shift all fields in the raw layout, but this operation is possible only if the class
> 1196:     // doesn't have inherited fields (offsets of inherited fields cannot be changed). If a
> 1197:     // field shift is needed but not possible, atomic layouts are disabled.

Suggestion:

    // field shift is needed but not possible, all atomic layouts are disabled and only reference and loosely consistent are supported.

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.

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

> 288:   GrowableArray<FieldInfo>* _field_info;
> 289:   FieldLayoutInfo* _info;
> 290:   // Array<InlineKlass*>* _inline_type_field_klasses;

Remove?

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

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?

src/hotspot/share/oops/inlineKlass.hpp line 200:

> 198:     return static_cast<const InlineKlass*>(k);
> 199:   }
> 200:   // static InlineKlass* cast(Klass* k);

Suggestion:

src/hotspot/share/oops/instanceKlass.cpp line 183:

> 181:   if (_loadable_descriptors == nullptr) return false;
> 182:   for (int i = 0; i < _loadable_descriptors->length(); i++) {
> 183:         // Symbol* class_name = _constants->klass_at_noresolve(_loadable_descriptors->at(i));

Suggestion:

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?

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

PR Review: https://git.openjdk.org/valhalla/pull/1275#pullrequestreview-2372924439
PR Review Comment: https://git.openjdk.org/valhalla/pull/1275#discussion_r1803376234
PR Review Comment: https://git.openjdk.org/valhalla/pull/1275#discussion_r1808893029
PR Review Comment: https://git.openjdk.org/valhalla/pull/1275#discussion_r1808915598
PR Review Comment: https://git.openjdk.org/valhalla/pull/1275#discussion_r1808965782
PR Review Comment: https://git.openjdk.org/valhalla/pull/1275#discussion_r1808975301
PR Review Comment: https://git.openjdk.org/valhalla/pull/1275#discussion_r1808982551
PR Review Comment: https://git.openjdk.org/valhalla/pull/1275#discussion_r1808904155
PR Review Comment: https://git.openjdk.org/valhalla/pull/1275#discussion_r1808905176
PR Review Comment: https://git.openjdk.org/valhalla/pull/1275#discussion_r1808990396
PR Review Comment: https://git.openjdk.org/valhalla/pull/1275#discussion_r1808996534
PR Review Comment: https://git.openjdk.org/valhalla/pull/1275#discussion_r1809006518
PR Review Comment: https://git.openjdk.org/valhalla/pull/1275#discussion_r1809008647
PR Review Comment: https://git.openjdk.org/valhalla/pull/1275#discussion_r1810852592