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

[Jorn Vernee]

On Mon, 21 Jun 2021 15:38:35 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> This patch includes some of the changes from Lee to support a set of static functions to allow bulk copy from Java arrays to segments (and viceversa) in a more succint fashion (so that the user doesn't have to create a temporary heap segment to do the copy).
>> 
>> I've added a new public method to `MemorySegment` which performs an *element-wise* bulk copy; it takes a source segment and a couple of element layouts: the source element layout and the destination element layout. The two layouts must have same size, but can have different alignments (which will be checked against the corresponding segments) and byte orders. If the byte order differs, a bulk copy with swap will be performed. As such, this method generalizes the previous `copyFrom` - as follows:
>> 
>> 
>> copyFrom(srcSegment) -> copyFrom(JAVA_BYTE, srcSegment, JAVA_BYTE)
>> 
>> I've added support for argument type profiling for MemoryCopy static methods to avoid type pollution in cases where same metod is called with different memory segment types.
>> 
>> I've done a pass over the javadoc, and make it more consistent with the rest of the API. I've also reworked the test a bit to use the data provider functionality of TestNG, since all the test cases were similar, except for the carrier type.
>> 
>> There are other cosmetic changes as well, compared to original code from Lee, such as naming of static fields which is now capitalized. Everything else is the same.
>
> Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Remove alignment constraints from MemoryCopy layouts

I guess this is still ongoing, but for now some comments inline.

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

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?

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryCopy.java line 137:

> 135:         MemorySegment srcSegmentSlice =
> 136:                 MemorySegment.ofArray(srcArray).asSlice(srcIndexChars * 2L, srcCopyLengthChars * 2L);
> 137:         MemorySegment dstSegmentSlice = dstSegment.asSlice(dstOffsetBytes, srcCopyLengthChars * 2L);

Little weird to see `srcCopyLengthChars` being used for the destination slice. But the length is the same for both of course. Maybe just drop the `src` prefix? (it's a nit though).

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

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.

test/jdk/java/foreign/TestMemoryCopy.java line 387:

> 385: 
> 386:     @DataProvider
> 387:     Object[][] copyModes() {

Seems unused.

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

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