8331006: [lworld] Support of null markers for nullable flat fields #1078

fparain · 2024-04-18T20:40:22Z

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.

Progress

Change must not contain extraneous whitespace

Issue

JDK-8331006: [lworld] Support of null markers for nullable flat fields (Enhancement - P3)

Reviewers

David Simms (@MrSimms - Committer) ⚠️ Review applies to 34180e09
Dan Heidinga (@DanHeidinga - no project role)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/1078/head:pull/1078
$ git checkout pull/1078

Update a local copy of the PR:
$ git checkout pull/1078
$ git pull https://git.openjdk.org/valhalla.git pull/1078/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1078

View PR using the GUI difftool:
$ git pr show -t 1078

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1078.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-04-18T20:40:56Z

👋 Welcome back fparain! A progress list of the required criteria for merging this PR into lworld will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-04-18T20:41:45Z

@fparain This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: dsimms, heidinga

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 302 new commits pushed to the lworld branch:

380e8f6: 8326401: [lworld] compiler/valhalla/inlinetypes/TestLWorld.java fails after merge of jdk-22+25
d67b732: 8331362: [lworld] JDK-8331217 breaks the build
9ecd7ce: 8331217: [lworld] sun.misc.Unsafe should fail to get the offset of a field of value type
f40784d: Merge jdk
26de9e2: 8321616: Retire binary test vectors in test/jdk/java/util/zip/ZipFile
b530c02: 8317804: com/sun/jdi/JdwpAllowTest.java fails on Alpine 3.17 / 3.18
e70cb4e: 8322565: (zipfs) Files.setPosixPermissions should preserve 'external file attributes' bits
d89602a: 8322982: CTW fails to build after 8308753
3bd9042: 8320788: The system properties page is missing some properties
525063b: 8322878: Including sealing information Class.toGenericString()
... and 292 more: https://git.openjdk.org/valhalla/compare/2a85be0465b0dafa878818741427dc3c5c12f165...lworld

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the lworld branch, type /integrate in a new comment.

mlbridge · 2024-04-23T17:51:02Z

Webrevs

test/hotspot/jtreg/runtime/valhalla/inlinetypes/NullableFlatFieldTest.java

test/hotspot/jtreg/runtime/valhalla/inlinetypes/field_layout/NullMarkersTest.java

test/hotspot/jtreg/runtime/valhalla/inlinetypes/NullableFlatFieldTest.java

MrSimms

Good initial runtime implementation to hang the JIT implementation off of, further work for other PRs...good to go !

src/hotspot/cpu/aarch64/templateTable_aarch64.cpp

src/hotspot/share/classfile/fieldLayoutBuilder.cpp

DanHeidinga · 2024-04-30T13:57:33Z

src/hotspot/share/classfile/fieldLayoutBuilder.cpp

+      bool field_is_known_value_class =  !fieldinfo.field_flags().is_injected() && _inline_type_field_klasses != nullptr && _inline_type_field_klasses->at(fieldinfo.index()) != nullptr;
+      bool value_has_oops = field_is_known_value_class ? _inline_type_field_klasses->at(fieldinfo.index())->nonstatic_oop_count() > 0 : true;
+      bool is_candidate_for_flattening = fieldinfo.field_flags().is_null_free_inline_type() || (EnableNullableFieldFlattening && field_is_known_value_class && !value_has_oops);
+      // 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...

I don't like the duplication either, and having a helper method would make the code easier to read. However, this is a section of code under big transformations, with nullable flat field being added, then next the atomic fields, then the nullable atomic fields. Before refactoring, I'd prefer to wait until all the cases are covered, so we know exactly all the parameters the helper method will require, and if the two cases are still identical or if some divergences appeared.

DanHeidinga · 2024-04-30T14:08:33Z

src/hotspot/share/interpreter/interpreterRuntime.cpp

+  InstanceKlass* ik = InstanceKlass::cast(obj_h()->klass());
+  int nm_offset = ik->null_marker_offsets_array()->at(entry->field_index());
+  if (val_h() == nullptr) {
+    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.

In this case, there is a single write from the current thread, so there's no visibility ordering issue like with the non-null case. A concurrently reading thread will check (or has already read) the null marker, and this would be no different than reading a reference field being set to null. So I don't think we need a barrier here, but if you really think there's a potential issue, I can add one.

I think you're right here and we don't need the release_byte_field_put here. Just as we're happy for reads/writes of null references to slowly make their way to other threads, a slow discovery of the null bit is fine given the other value fields are stable before the bit flips.

The barrier you have in place is sufficient for that constraint.

DanHeidinga · 2024-04-30T14:12:04Z

src/hotspot/share/interpreter/interpreterRuntime.cpp

+  if (entry->has_internal_null_marker()) {
+    // The interpreter copies values with a bulk operation
+    // To avoid accidently setting the null marker to "null" during
+    // 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

How does that work for our memory barriers then? Do we not need a release here as well given the acquire used to read the null marker?

This particular case cannot currently happen with JEP 401 model implementation (nullable flat field are limited to single field values until we have atomic updates supported). But it involves the tearing issue the interpreter has in some cases. So, even adding a barrier would not make the code correct. I'd prefer to address the issue of this particular code when fixing the wider interpreter tearing issue.

DanHeidinga

Thanks for the clarifications. This looks good to me.

fparain · 2024-05-01T15:32:39Z

Mr Simms, Dan,
Thank you for your reviews.
Fred

fparain · 2024-05-01T15:32:46Z

/integrate

openjdk · 2024-05-01T15:34:24Z

Going to push as commit eb1df16.
Since your change was applied there have been 302 commits pushed to the lworld branch:

380e8f6: 8326401: [lworld] compiler/valhalla/inlinetypes/TestLWorld.java fails after merge of jdk-22+25
d67b732: 8331362: [lworld] JDK-8331217 breaks the build
9ecd7ce: 8331217: [lworld] sun.misc.Unsafe should fail to get the offset of a field of value type
f40784d: Merge jdk
26de9e2: 8321616: Retire binary test vectors in test/jdk/java/util/zip/ZipFile
b530c02: 8317804: com/sun/jdi/JdwpAllowTest.java fails on Alpine 3.17 / 3.18
e70cb4e: 8322565: (zipfs) Files.setPosixPermissions should preserve 'external file attributes' bits
d89602a: 8322982: CTW fails to build after 8308753
3bd9042: 8320788: The system properties page is missing some properties
525063b: 8322878: Including sealing information Class.toGenericString()
... and 292 more: https://git.openjdk.org/valhalla/compare/2a85be0465b0dafa878818741427dc3c5c12f165...lworld

Your commit was automatically rebased without conflicts.

openjdk · 2024-05-01T15:34:30Z

@fparain Pushed as commit eb1df16.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

fparain added 10 commits March 6, 2024 16:01

Nullable flat field step 1

17eda5b

Merge remote-tracking branch 'upstream/lworld' into null_flat

186adfd

Merge remote-tracking branch 'upstream/lworld' into null_flat

b5390a5

Merge remote-tracking branch 'upstream/lworld' into null_flat

0457e89

Fix metadata, FieldInfoStream and printing

76e8666

Add layout test framework plus bug fixes

31d9a68

Add support for internal null markers in layouts and tests

51a150f

Add internal null marker support, aarch64 support, unsafe support

4eb3be1

Document nullable flat field access protocols

8396897

Merge remote-tracking branch 'upstream/lworld' into null_flat

8b987b5

fparain added 3 commits April 19, 2024 13:29

Fixes and cleanup

15e2f41

Merge remote-tracking branch 'upstream/lworld' into null_flat

f2917ff

More cleanup

684da16

fparain changed the title ~~[lworld draft] Implementation of nullable flat fields~~ 8331006: [lworld] Support of null markers for nullable flat fields Apr 23, 2024

fparain marked this pull request as ready for review April 23, 2024 17:46

openjdk bot added ready rfr labels Apr 23, 2024

MrSimms reviewed Apr 25, 2024

View reviewed changes

test/hotspot/jtreg/runtime/valhalla/inlinetypes/NullableFlatFieldTest.java Outdated Show resolved Hide resolved

MrSimms reviewed Apr 25, 2024

View reviewed changes

test/hotspot/jtreg/runtime/valhalla/inlinetypes/field_layout/NullMarkersTest.java Show resolved Hide resolved

MrSimms reviewed Apr 25, 2024

View reviewed changes

test/hotspot/jtreg/runtime/valhalla/inlinetypes/NullableFlatFieldTest.java Outdated Show resolved Hide resolved

Fixing issues spotted during reviews

34180e0

MrSimms approved these changes Apr 30, 2024

View reviewed changes

DanHeidinga reviewed Apr 30, 2024

View reviewed changes

More fixes from reviews

d8190b7

DanHeidinga approved these changes May 1, 2024

View reviewed changes

openjdk bot added the integrated label May 1, 2024

openjdk bot closed this May 1, 2024

openjdk bot removed ready rfr labels May 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8331006: [lworld] Support of null markers for nullable flat fields #1078

8331006: [lworld] Support of null markers for nullable flat fields #1078

fparain commented Apr 18, 2024 •

edited by openjdk bot

bridgekeeper bot commented Apr 18, 2024

openjdk bot commented Apr 18, 2024 •

edited

mlbridge bot commented Apr 23, 2024 •

edited

MrSimms left a comment

DanHeidinga Apr 30, 2024

fparain May 1, 2024

DanHeidinga Apr 30, 2024

fparain May 1, 2024

DanHeidinga May 1, 2024

DanHeidinga Apr 30, 2024

DanHeidinga Apr 30, 2024

fparain May 1, 2024

DanHeidinga left a comment

fparain commented May 1, 2024

fparain commented May 1, 2024

openjdk bot commented May 1, 2024

openjdk bot commented May 1, 2024

8331006: [lworld] Support of null markers for nullable flat fields #1078

8331006: [lworld] Support of null markers for nullable flat fields #1078

Conversation

fparain commented Apr 18, 2024 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Apr 18, 2024

openjdk bot commented Apr 18, 2024 • edited

mlbridge bot commented Apr 23, 2024 • edited

Webrevs

MrSimms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanHeidinga left a comment

Choose a reason for hiding this comment

fparain commented May 1, 2024

fparain commented May 1, 2024

openjdk bot commented May 1, 2024

openjdk bot commented May 1, 2024

fparain commented Apr 18, 2024 •

edited by openjdk bot

openjdk bot commented Apr 18, 2024 •

edited

mlbridge bot commented Apr 23, 2024 •

edited