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

[Maurizio Cimadamore]

On Thu, 2 Dec 2021 14:27:17 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:
> 
>   Add benchmarks for aligned access

I've uploaded a new revision which slightly improves existing support for skipping alignment check when the var handle has an offset that is already "provably aligned" (in which case only the base address needs to be checked). Here are some numbers:


Benchmark                                                Mode  Cnt  Score   Error  Units
LoopOverNonConstant.segment_loop                         avgt   30  0.225 ? 0.003  ms/op
LoopOverNonConstant.segment_loop_aligned                 avgt   30  0.228 ? 0.003  ms/op
LoopOverNonConstant.segment_loop_instance                avgt   30  0.226 ? 0.003  ms/op
LoopOverNonConstant.segment_loop_instance_aligned        avgt   30  0.330 ? 0.005  ms/op


Benchmark                                              (polluteProfile)  Mode  Cnt  Score    Error  Units
LoopOverNonConstantHeap.segment_loop                              false  avgt   30  0.231 ?  0.001  ms/op
LoopOverNonConstantHeap.segment_loop                               true  avgt   30  0.227 ?  0.003  ms/op
LoopOverNonConstantHeap.segment_loop_aligned                      false  avgt   30  0.228 ?  0.002  ms/op
LoopOverNonConstantHeap.segment_loop_aligned                       true  avgt   30  0.229 ?  0.003  ms/op
LoopOverNonConstantHeap.segment_loop_instance                     false  avgt   30  0.225 ?  0.003  ms/op
LoopOverNonConstantHeap.segment_loop_instance                      true  avgt   30  0.225 ?  0.003  ms/op
LoopOverNonConstantHeap.segment_loop_instance_aligned             false  avgt   30  0.329 ?  0.003  ms/op
LoopOverNonConstantHeap.segment_loop_instance_aligned              true  avgt   30  0.321 ?  0.004  ms/op


In other words, performance is fast for both unaligned and aligned - but only when using var handles derived from layout paths. When using instance methods, it gets slower (but this is true even before this patch). While this can be addressed by a convoluted workaround such as the one originally attempted in [1], I think it's better to wait for a fuller VM fix.

[1] - https://git.openjdk.java.net/jdk/pull/6589

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

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