[foreign-memaccess+abi] RFR: 8278151: Heap segments should handle alignment constraints in a deterministic fashion [v4]

[Paul Sandoz]

On Thu, 2 Dec 2021 16:14:27 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> This patch fixes the issue with alignment checks on heap segment leading to failures which depend on VM implementation details and layout choices.
>> 
>> This is discussed in more details here:
>> https://mail.openjdk.java.net/pipermail/panama-dev/2021-November/015852.html 
>> 
>> The fix consists in adding a *max alignment* mask to all segments. Native segments have mask set at 0, whereas heap segment have the mask set at the element size of their backing array (e.g. 1 for `byte[]`, 2 for `short[]`/`char[]` and so forth).
>> 
>> The alignment check is the computed as follows:
>> 
>> 
>> if (((address | maxAlignMask) & alignmentMask) != 0) {
>>                 throw ...
>> }
>> 
>> 
>> Where `address` is the long address being dereferenced, `maxAlignMask` is the max alignment associated with the segment being dereferenced, and `alignmentMask` is the alignment associated with the layout used during a dereference operation.
>> This patch also applies alignment checks to bulk copy operations as well, and it consolidates the way in which alignment checks are carried out (by adding a couple of helper methods, in `Utils` and `AbstractMemorySegment`, respectively).
>> 
>> This patch also changes alignment errors to uniformly throw IllegalArgumentException, in all cases.
>> 
>> Finally, this patch clarifies that during bulk copy and spliterator calls, the element layout alignment cannot be bigger than its size (as the size is used as a stride).
>> 
>> A new test `TestHeapAlignment` has been added to check that alignment of heap segment is correctly enforced.
>> 
>> Performance-wise, this patch doesn't change anything; that's because the value layout constants in `ValueLayout` are all unaligned. For code using aligned layout, some slight regression when using heap segment is possible (whereas native segment should have same performance as before). Eventually, these performance issues should disappear once [1] is fixed.
>> 
>> [1] - https://bugs.openjdk.java.net/browse/JDK-8277850
>
> Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Strengthen test

Excellent analysis of the problem, and a very pleasing result.

Having consistent alignment check behavior with sequenced layout construction and get/setAtIndex seems like a good thing, re: your comment.

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java line 170:

> 168:  *     <li>if {@code AT = long[]}, then {@code A = 8}
> 169:  *     <li>if {@code AT = double[]}, then {@code A = 8}
> 170:  * </ul>

This might be clearer as a table then there is less need to appeal to `AT` and `A` e.g.:
(copied from a table in `String`):


     * <blockquote><table class="plain">
     * <caption style="display:none">Array type of an array backing a segment and its address aligment</caption>
     * <thead>
     * <tr>
     *     <th scope="col">Array type</th>
     *     <th scope="col">Alignment</th>
     * </tr>
     * </thead>
     * <tbody>
     * <tr><th scope="row" style="text-weight:normal">{@code boolean[]}</th>
     *     <td>{@code 1}</td></tr>
     * <tr><th scope="row" style="text-weight:normal">{@code byte[]}</th>
     *     <td>{@code 1}</td></tr>
     * ...
     * </tbody>
     * </table></blockquote>

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java line 265:

> 263:      * {@code elementLayout} size is greater than zero, if this segment is
> 264:      * <a href="MemorySegment.html#segment-alignment">incompatible with the alignment constraints</a> in the provided layout,
> 265:      * or if the {@code elementLayout} alignment is greater than its size.

I believe its possible to have multiple declarations of `@throws IllegalArgumentException`, so if you wish for clarity there could be a declaration for each separate case.

test/jdk/java/foreign/TestHeapAlignment.java line 57:

> 55:         if (arr != null) {
> 56:             assertAligned(align, layout, () -> MemorySegment.copy(arr, 0, segment, layout, 0, 1));
> 57:             assertAligned(align, layout, () -> MemorySegment.copy(arr, 0, segment, layout, 0, 1));

Swap arguments for second invocation?
Suggestion:

            assertAligned(align, layout, () -> MemorySegment.copy(segment, layout, 0, arr, 0, 1));

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

Marked as reviewed by psandoz (Committer).

PR: https://git.openjdk.java.net/panama-foreign/pull/622