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

[Uwe Schindler]

On Tue, 22 Jun 2021 12:37:17 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:
> 
>   Fix wrong polarity of readOnly in copyTo methods

Hi,
to me this looks good now.

I like the new code much more, because the previous version of this pull request was a bit strange:
- In MemoryCopy the code checked if native order equals parameter order and then used ValueLayout (one instance for each valuecsize) to call MemorySegment.copyFrom()
- MemorySegment.copyFrom() took the ValueLayout and then compared it again to determine if it is different from native byte order; just to convert it back to a boolean at end: swap yes/no.

All this was so much code and I was always afraid that there is some case where the ValueLayouts were used wrongly.

So much better! I love it!

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

> 56:     private static final Unsafe unsafe = Unsafe.getUnsafe();
> 57: 
> 58:     private static final int BYTE_BASE = unsafe.arrayBaseOffset(byte[].class);

Are there not constants for this already in `jdk.internal.misc.Unsafe`?

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

> 151:         AbstractMemorySegmentImpl destImpl = (AbstractMemorySegmentImpl)dstSegment;
> 152:         destImpl.checkAccess(dstOffsetBytes, srcCopyLengthChars << 1, false);
> 153:         if (order == ByteOrder.nativeOrder()) {

I like this code much more than the previous one, as it makes clear: swap tru or false, just a boolean! The Usage of MemorySegment.copyFrom() was very complicated as it used ValueLayouts, one for each size.

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

> 117:             arr[i] = (byte)i;
> 118:         }
> 119:         return MemorySegment.ofArray(arr);

This segment should be made readonly, to assert all checkAccess() calls.

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

Marked as reviewed by uschindler (no project role).

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