[lworld] RFR: 8331006: [lworld] Support of null markers for nullable flat fields [v2]

[Dan Heidinga]

On Mon, 29 Apr 2024 14:41:45 GMT, Frederic Parain <fparain at openjdk.org> wrote:

>> This is the first step in supporting nullable flat fields in JEP 401.
>> Those changes give value fields the optional capability to be associated with a null marker that indicates if the value if the field is null or not. Null markers are integrated in the object layout, and in the field metadata (in both compresses and uncompressed forms).
>> Field accesses in the x86 and the arch64 interpreter have been extended to check the presence of a null marker when reading a field. If present, the null marker is checked in reads and updated on writes.
>> 
>> The field layout logic is becoming more complex (and the complexity will continue to increase in the future with the addition of atomic flat fields and atomic nullable flat fields). So the changeset includes a test framework able to verify the consistency of fields layout using the output of -XX:+PrintFieldLayout. The format of data printed by PrintFieldLayout has been extended and modified to be easier to parse.
>
> Frederic Parain has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fixing issues spotted during reviews

src/hotspot/cpu/aarch64/templateTable_aarch64.cpp line 3355:

> 3353:   // access field
> 3354:   switch (bytecode()) {
> 3355:   case Bytecodes::_fast_vputfield: //fall through

Suggestion:

  case Bytecodes::_fast_vputfield:

src/hotspot/cpu/aarch64/templateTable_aarch64.cpp line 3359:

> 3357:       Label is_flat, has_null_marker, done;
> 3358:       __ test_field_has_null_marker(r3, noreg /* temp */, has_null_marker);
> 3359:       __ null_check(r0);

Does `__ null_check(r0);` need to occur first to make sure we consistently null check the receiver object?  Or is the null receiver check handled in the InterpreterRuntime::write_nullale_flat_field helper?

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

> 352:       has_instance_fields = true;
> 353:       LayoutRawBlock* block;
> 354:       // if (fs.field_flags().is_null_free_inline_type()) {

Left over commented code or breadcrumb for future update?

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

> 798:       bool value_has_oops = field_is_known_value_class ? _inline_type_field_klasses->at(fieldinfo.index())->nonstatic_oop_count() > 0 : true;
> 799:       bool is_candidate_for_flattening = fieldinfo.field_flags().is_null_free_inline_type() || (EnableNullableFieldFlattening && field_is_known_value_class && !value_has_oops);
> 800:       // if (!fieldinfo.field_flags().is_null_free_inline_type()) {

Is it worth pulling this into a `array_candidate_for_flattening` helper method?  It's a complex set of conditions and this is the second occurrence of it...

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

> 353:   int nm_offset = ik->null_marker_offsets_array()->at(entry->field_index());
> 354:   if (val_h() == nullptr) {
> 355:     obj_h()->byte_field_put(nm_offset, (jbyte)0);

Do we need a barrier here?  When we write the null marker, it is as if we wrote the null marker and then nulled the fields out (which we don't actually do) which makes me think we always need the barrier after writing the null marker to be consistent with the non-null case.

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

> 360:     // The interpreter copies values with a bulk operation
> 361:     // To avoid accidently setting the null marker to "null" during
> 362:     // the copying, the null marker is set to non zero in the source object

Is this saying if the null marker is embedded in the value field layout rather than placed separately, we set it to non-null before writing it into the container?  This tripped me up a little bit reading the code but I think it makes sense

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

PR Review Comment: https://git.openjdk.org/valhalla/pull/1078#discussion_r1581317255
PR Review Comment: https://git.openjdk.org/valhalla/pull/1078#discussion_r1581320878
PR Review Comment: https://git.openjdk.org/valhalla/pull/1078#discussion_r1584872488
PR Review Comment: https://git.openjdk.org/valhalla/pull/1078#discussion_r1584879794
PR Review Comment: https://git.openjdk.org/valhalla/pull/1078#discussion_r1584897107
PR Review Comment: https://git.openjdk.org/valhalla/pull/1078#discussion_r1584902626