[foreign-memaccess] RFR: JDK-8242011: Add support for memory address combinator

[Maurizio Cimadamore]

On Thu, 2 Apr 2020 10:02:47 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>> Following recent changes (see https://github.com/openjdk/panama-foreign/pull/77)which backport some of the goodies in
>> foreign-abi into foreign-memaccess, this patch brings support for a VarHandle combinator which turns a regular memory
>> access var handle into a var handle which gets/sets a MemoryAddress (e.g. instead of a long).
>> This patch also addresses the general problem of the co-existence between combinators in MemoryHandles and the general
>> var handle combinators in MethodHandles.
>> This coexistence has always been tricky, because the combinators in MemoryHandles like to create a new 'flattened'
>> memory access var handle, which provides best possible performances, and also performs certain alignment checks
>> upfront.  However, if a memory access handle is adapted (e.g. the carrier type is changed) this simplistic approach no
>> longer works, as, by reconstructing the memory access var handle from scratch we will also lose all the adaptations.
>> The solution is to detect as to wheter the target handle is a "direct" memory access var handle or not (special
>> provisions are made for the ubiquitous adaptation added on all memory access var handle creation as a workaround for
>> JDK-8237349). If that's the case, and, if the stride or the offset matches the alignment constraint, then we go the
>> fast/flattened path and we can adapt by reconstructing the handle from scratch. If any of these two conditions are not
>> met (there's complex adaption that would be dropped on the floor, or the offset/stride parameter doesn't match with
>> alignment constraint), then the slow path is taken - the target var handle is kept around and is adapted using the
>> standard combinator API. This leads to a less performant VarHandle (because, unfortunately, calling addOffset()
>> currently breaks C2 optimizations), but also guarantees that alignment constraints will be checked in full (this is
>> because the memory access var handle implementation always checks the alignment of the base address passed to it - only
>> for the offset part is the alignment check skipped - on the basis that this has already been verified by
>> construction).  The result is that we can lift a lot of the restrictions surrounding the combinators in MemoryHandles;
>> such combinators can now work on _any_ var handle (provided the var handle has a first coordinate type of type
>> MemoryAddress). Also, I've lifted the alignment restrictions, since these will either be enforced dynamically (by
>> taking the slow path), or they won't be enforced because we have already statically proven that the constraints are
>> satisfied (fast path).  I've also took the chance to rename some of the classes surrounding memory access var handles
>> to use the "memory access var handle" terminology, which is the one that stuck (currently, some classes use the word
>> "address var hande" which is ambiguous).
>
> src/java.base/share/classes/jdk/internal/access/JavaLangInvokeAccess.java line 122:
> 
>> 121:      * Used by {@code jdk.incubator.foreign.MemoryHandles}.
>> 122:      */
>> 123:     boolean isMemoryAccessVarHandle(VarHandle handle);
> 
> Does this need `@param` and `@return` tags?

I went with the style of the other comments surrounding this. This is internal API, seems like the main need (as arised
during RFR) is to indicate where a routine is used :-)

> src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryHandles.java line 253:
> 
>> 252:      * The returned var handle will feature the same type as the target var handle; an additional access coordinate
>> 253:      * of type {@code long} will be <em>prepended</em> to the access coordinate types of the target var handle.
>> 254:      *
> 
> Doesn't the MemoryAddress also count as an access coordinate? So the new coordinate is put right after MemoryAddress.

good point, will fix

> src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryHandles.java line 262:
> 
>> 261:     public static VarHandle withStride(VarHandle target, long bytesStride) {
>> 262:         if (bytesStride == 0) {
>> 263:             throw new IllegalArgumentException("Stride must be positive: " + bytesStride);
> 
> Pre-existing, but it seems that this should check for `byteStride <= 0` instead?

also good point, will fix and add a test

> src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryHandles.java line 342:
> 
>> 341:     private static long addressToLong(MemoryAddress value) {
>> 342:         return ((MemoryAddressImpl)value).unsafeGetOffset();
>> 343:     }
> 
> Maybe this should use MemoryAddressImpl::addressof, since that also checks for heap addresses. I don't think we want to
> allow serializing heap addresses?

yeah - that's what it should do

> test/jdk/java/foreign/TestAddressHandle.java line 80:
> 
>> 79:         MemoryHandles.asAddressVarHandle(doubleHandle);
>> 80:     }
>> 81:
> 
> The adapter code also checks for `!carrier.isPrimitive()` that's also a case that could be tested.

I'll add something

> test/jdk/java/foreign/TestAddressHandle.java line 108:
> 
>> 107:                 { MemoryHandles.asAddressVarHandle(MemoryHandles.withOffset(MemoryHandles.varHandle(byte.class,
>> ByteOrder.nativeOrder()), 0)) }, 108:                 {
>> MemoryHandles.asAddressVarHandle(MemoryLayouts.JAVA_BYTE.varHandle(byte.class)) } 109:         };
> 
> `boolean` would also be an interesting case to test. explicitCastArguments does have handling to convert between
> boolean and long as well. Maybe that's a carrier you also want to explicitly disallow in the adapter?

Good point, I'll go for disallow, since there's only two pointers you can express with that :-)

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

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