8331734: Atomic MemorySegment VarHandle operations fails for element layouts

[mcimadamore]

This PR fixes an issue that has crept into the FFM API implementation.

From very early stages, the FFM API used to disable the alignment check on nested layout elements, in favor of an alignment check against the memory segment base address. The rationale was that the JIT had issue with eliminating redundant alignment checks, and accessing nested elements could never result in alignment issues, since the alignment of the container is provably bigger than that of the contained element. This means that, when creating a var handle for a nested layout element, we set the nested layout alignment to 1 (unaligned), derive its var handle, and then decorate the var handle with an alignment check for the container.

At some point in 22, we tweaked the API to throw UnsupportedOperationException when using an access mode incompatible with the alignment constraint of the accessed layout. That is, a volatile read on an int is only possible if the access occurs at an address that is at least 4-byte aligned. Otherwise an UOE is thrown.

Unfortunately this change broke the aforementioned optimization: creating a var handle for an unaligned layout works, but the resulting layout will not support any of the atomic access modes.

Since this optimization is not really required anymore (proper C2 support to hoist/eliminate alignment checks has been added since then), it is better to disable this implementation quirk, and leave optimizations to C2.

(If we really really wanted to optimize things a bit, we could remove the container alignment check in the case the accessed value is the first layout element nested in the container, but this PR doesn't go that far).

I've run relevant benchmarks before/after and found no differences. In part this is because arrayElementVarHandle is unaffected. But, even after tweaking the LoopOverNonConstant benchmark to add explicit tests for the code path affected, no significant difference was found, sign that C2 is indeed able to spot (and remove) the redundant alignment check. Note: if we know that aligned_to_N(base) holds, then it's easy to prove that aligned_to_M(base + offset + index * scale) holds, when N >= M and with offset and scale known (the latter a power of two).

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8331734: Atomic MemorySegment VarHandle operations fails for element layouts (Bug - P2)

Reviewers

• Per Minborg (@minborg - Reviewer) ⚠️ Review applies to aa5ed595
• Paul Sandoz (@PaulSandoz - Reviewer) ⚠️ Review applies to aa5ed595

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19124

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19124.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

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

[openjdk]

@mcimadamore 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:

8331734: Atomic MemorySegment VarHandle operations fails for element layouts

Reviewed-by: pminborg, psandoz

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 37 new commits pushed to the master branch:

• d47a4e9: 8332008: Enable issuestitle check
• 0bf7282: 8331231: containers/docker/TestContainerInfo.java fails
• ffbdfff: 8331999: BasicDirectoryModel/LoaderThreadCount.java frequently fails on Windows in CI
• c7d98df: 8329691: Support nonlikelyScript parent locale inheritance
• aa4cddd: 8331950: Remove MemoryPoolMBean/isCollectionUsageThresholdExceeded from ZGC ProblemLists
• c4ff58b: 8330146: assert(!_thread->is_in_any_VTMS_transition()) failed
• aaa90b3: 8327476: Upgrade JLine to 3.26.1
• 0a4eeea: 8331033: EA fails with "EA unexpected CallLeaf unsafe_setmemory" after JDK-8329331
• ad0b54d: 8314225: SIGSEGV in JavaThread::is_lock_owned
• ac86f59: 8330532: Improve line-oriented text parsing in HotSpot
• ... and 27 more: https://git.openjdk.org/jdk/compare/23a72a1f2f651d5e8e9a0eb1e75e2b44572a13da...master

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 master branch, type /integrate in a new comment.

[openjdk]

@mcimadamore The following label will be automatically applied to this pull request:

• core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 02: Full - Incremental (40ba693c)
• 01: Full - Incremental (829c5e28)
• 00: Full (aa5ed595)

[minborg]

LGTM. As mentioned in the PR notes, additional optimizations could be made and this could be the objective of a future PR.

[mcimadamore]

Overall, I'm not too sure about the alignment check on the root layout performed by a var handle pointing to a group layout member. We've got there honestly - e.g. make sure the accessed segment/offset is really compatible with root layout. But, if so, then why also not checking the segment size against the root layout size? E.g. say I have a layout for:

var POINT_LAYOUT = structLayout(JAVA_DOUBLE.withName("x"), JAVA_DOUBLE.withName("y"))

And I derive a var handle for x, like so:

VarHandle x_handle = POINT_LAYOUT.varHandle(PathElement.groupLayout("x"));

Say that I have a 8-byte segment, and I use that with the x_handle above:

MemorySegment halfPoint = Arena.ofAuto().allocate(8);
double x = (double)x_handle.get(halfPoint, 0);

Should this work? Currently it does, and one might claim that it does so "by accident" - e.g. it just so happen that the provided segment/offset combo was big enough to access x. But of course accessing y would fail.

There are of course two possible interpretations here, and both are valid:

• x_handle is just a regular memory access var handle for JAVA_DOUBLE - it just contains some extra logic to compute the correct offset from the start segment/offset combo, but that's it.
• x_handle is really meant to provide access to a memory segment modelling (at least) one struct with layout POINT_LAYOUT. As such, the initial segment/offset combo should (a) be adequately aligned (according to POINT_LAYOUT.byteAlignment()) and have sufficient size (according to POINT_LAYOUT.byteSize())

The former favors performance, the latter favors safety, as it ensures that the initial segment/offset combo do indeed describe a segment whose characteristics are compatible with those of the selected layout.

This is also related to MemoryLayout::arrayElementVarHandle. Currently this method is specified as:

MethodHandles.collectCoordinates(varHandle(elements), 1, scaleHandle())

This means that the root layout checks performed by the obtained method handle will refer to the alignment (and size) of the current layout. For instance, if I write:

VarHandle xs_handle = POINT_LAYOUT.arrayElementVarHandle(PathElement.groupLayout("x"));

Assuming we picked the safer semantics described above, the resulting var handle will check that the initial segment/offset combo will point to a segment whose alignment (and size) are compatible with those of POINT_LAYOUT. I believe this is acceptable: this var handle is meant to capture unbounded sequence access - so the only thing we can meaningfully check is whether the segment is at least big enough to store one element (if not, then we know that all accesses are going to fail). But, on the other hand, an argument could be made that, given we don't know the size of the array statically, adding any kind of size check here feels too much and/or doesn't buy much.

[mcimadamore]

• x_handle is really meant to provide access to a memory segment modelling (at least) one struct with layout POINT_LAYOUT. As such, the initial segment/offset combo should (a) be adequately aligned (according to POINT_LAYOUT.byteAlignment()) and have sufficient size (according to POINT_LAYOUT.byteSize())

An example of this approach is attempted here:
https://github.com/openjdk/jdk/compare/master...mcimadamore:jdk:bad_align_segment_var_handle+size_checks?expand=1

Again, no significant performance regression is observed - the root layout checks dominate the value layout checks, and it seems like C2 is able to spot that.

[mcimadamore]

I've decided to integrate this for now, as the fix in this PR restores correctness of the implementation.
I will investigate follow up improvements and enhancements separately:
https://bugs.openjdk.org/browse/JDK-8331865

[mcimadamore]

/integrate

[openjdk]

Going to push as commit 1c5f150.
Since your change was applied there have been 56 commits pushed to the master branch:

• 65abf24: 8331866: Add warnings for locale data dependence
• d215bc4: 8332066: AArch64: Math test failures since JDK-8331558
• d11e70a: 8331646: Add specific regression leap year tests
• f95c937: 8331577: RISC-V: C2 CountLeadingZerosV
• 675fbe6: 8331993: Add counting leading/trailing zero tests for Integer
• 242446b: 8331931: JFR: Avoid loading regex classes during startup
• 45792c5: 8331352: error: template-id not allowed for constructor/destructor in C++20
• 1547a69: 8327696: [TESTBUG] "javax/swing/JTable/KeyBoardNavigation/KeyBoardNavigation.java" test instruction needs to be corrected
• 784b8fc: 8331744: java.lang.classfile.TypeKind improvements
• dea8076: 8332006: Test com/sun/net/httpserver/TcpNoDelayNotRequired.java run timeout with -Xcomp
• ... and 46 more: https://git.openjdk.org/jdk/compare/23a72a1f2f651d5e8e9a0eb1e75e2b44572a13da...master

Your commit was automatically rebased without conflicts.

[openjdk]

@mcimadamore Pushed as commit 1c5f150.

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