RFR: 8331734: Atomic MemorySegment VarHandle operations fails for element layouts

[Maurizio Cimadamore]

On Tue, 7 May 2024 15:42:23 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

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

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.

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

PR Comment: https://git.openjdk.org/jdk/pull/19124#issuecomment-2100853634