[foreign-memaccess+abi] RFR: Add support for high-level functions to copy to and from Java arrays [v2]

[Maurizio Cimadamore]

On Mon, 21 Jun 2021 17:39:47 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>> Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Remove alignment constraints from MemoryCopy layouts
>
> src/hotspot/share/oops/methodData.cpp line 1598:
> 
>> 1596:         return true;
>> 1597:       }
>> 1598:     }
> 
> Tbh, I'm a little skeptical about (the need for) adding argument type profiling at this point:
> - Only the first 2 arguments to a method are profiled by default (controlled by `TypeProfileArgsLimit`) and if we care about the MemorySegment args, there will be no profiling of those for the `copyFromArray` methods.
> - AFAICS, `Unsafe::copyMemory` is far less susceptible to changes in the type of the base pointers, which IIRC was the main reason why we added type profiling in the past for the MemoryAccess.* methods. The C2 intrinsic for copyMemory just always generates an out of line call with memory barriers [1].
> 
> [1] : https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/library_call.cpp#L4071-L4118

Ok, I can revert that - IMHO, the fact that a memory barrier is always added is a peformance issue that should be addressed, as that would impact things like liveness checks - but we can get there when we get there.

> src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryCopy.java line 40:
> 
>> 38:  * <p>
>> 39:  * Copy operations defined in this class accept a <em>byte order</em> parameter. If the specified byte order is different
>> 40:  * from the <em>native</em> byte order, a byte swap operation is performed on each array elements
> 
> This seems somewhat roundabout. Why not accept a `boolean swap` instead?

That seems consistent with MemoryAccess, to be honest. And with every other access operation which touches segments in the API.

> src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java line 405:
> 
>> 403:      * @param src the source segment.
>> 404:      * @param srcElementLayout the element layout associated with the source segment.
>> 405:      * @throws IndexOutOfBoundsException if {@code src.byteSize() > this.byteSize()}.
> 
> Maybe this should be strengthened to require both segments to be the same size? (Otherwise I guess there's quickly a question of 'why can't I specify a destination offset here').

For now, that is consistent with what the other `MemorySegment::copyFrom` does.

> test/jdk/java/foreign/TestMemoryCopy.java line 49:
> 
>> 47:  * These tests exercise the MemoryCopy copyFromArray(...) and copyToArray(...).
>> 48:  * To make these tests more challenging the segment is a view of the given array,
>> 49:  * which makes the copy operations overlapping self-copies.  Thus, this checks the claim:
> 
> I'm not sure I understand this claim; it seems to me that the test sets up an array view segment, but then copies the contents into a new array (using toArray) as src/dst to do the actual copying? i.e. the source and destination seem disjoint.

This comment was in the original patch - I'll fix or drop it.

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

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