[foreign] RFR: 8226555: Provide java.foreign.memory.Array a copy function similar to System.arraycopy

[Maurizio Cimadamore]

Hi Henry,
I'm unsure about the approach.

We have this routine sitting in BoundedPointer.java:

public static <Z> BoundedPointer<Z> fromArray(LayoutType<Z> type, Object array) {
  254         int size = java.lang.reflect.Array.getLength(array);
  255         long base = Util.unsafeArrayBase(array.getClass());
  256         long scale = Util.unsafeArrayScale(array.getClass());
  257         return new BoundedPointer<>(type, ScopeImpl.UNCHECKED, AccessMode.READ_WRITE,
  258                         MemoryBoundInfo.ofHeap(array, base, size * scale));
  259     }


If this became a new static factory on Pointer, then surely you could 
convert the Java array into a pointer and then use Pointer.copy for all 
the things you want to do?

The copyTo/From methods and Array::assign where really born not to copy 
stuff, but to initialize a native array (hence the strict length 
requirement), or to assign an array to a struct field (of same type).

Note that the two public Array::assign methods (for Java to native, 
native to Java) were added later, in response to some (valid!) 
performance concern, so they are more an example of what the impl can 
do, rather than a case of a perfectly designed API.

I believe we should:

1) keep Array::assign to mimic C assign between array in struct fields

2) drop the Array::assign between Java vs. native and viceversa

3) add a supported way to create a (heap) Pointer from a Java array - 
after all we can create Pointer from bytebuffer so I don't see why we 
shouldn't allow the same for arrays

This way you can use the existing Pointer.copy API instead of the 
'hybrid' Array::assign methods.

As for copying when layout is not the same, pointer.copy will fail - and 
I think it should continue to do so - user can cast his/her way around 
that. Don't be fooled by the fact that BoundedArray::copyFrom/To has a 
slow path - again, those routines are for (i) initializing the contents 
of a native array from a Java array or (ii) dumping the contents of a 
native array onto a Java array - so the slow path was originally added 
to handle cases where the LayoutType was 'complex' and required manual 
adaptation of the contents.

P.S.
as for your overload question, I think all the new methods are ambiguous 
- because when you call them, a numeric constant such as '0' is 
interpreted as 'int' and is compatible with both 'int' and 'long'. So 
it's a bit like doing:

m(int i, long l)
m(long l, int i)
m(long l1, long l2)

m(0, 0)

which is ambiguous. In your case is a bit more complex, as you have 
another ambiguity going on, between Object and Array, but that one could 
resolved correctly, if it weren't for the long vs. int mismatches, which 
make the three methods fundamentally incompatible with each other, with 
no 'best' one (if you pass ints). What you'd really need to resolve the 
ambiguity would be a method with signature:

(Array, int, Array, int, int)

Which would be more specific than both - but of course in this case, 
this signature doesn't make sense.

Maurizio


On 21/06/2019 07:59, Henry Jen wrote:
> Hi,
>
> Please review a webrev[1] to provide more flexible way to copy between native and native/java arrays.
>
> This webrev changes Array.assign behavior to make it more consistent:
>
> 1. It add a slow-path copy for assign between native arrays. Given the carrier type is the same, in case the native layout doesn’t match, it can fallback to sequential copy, which have been available for assign between native and java arrays.
>
> 2. It allows copy source array in full to a larger destination array. Before the patch, only slow path between native and java array can do that. Otherwise, it will have IllegalArgumentException caused by different layout.
>
> This webrev make assign work consistently between all three modes so that, it will copy full source array into destination that is large enough, or throw IndexOutOfBoundException if the destination is too small.
>
> Do check out the test case, that would help to clarify the specs if it’s not clear enough.
>
> There is a minor irritation for the API in that, it can be ambiguous between Array and Object, as you can see following example.
>
> /Users/hjen/ws/panama.dev/test/jdk/java/foreign/memory/ArrayTest.java:360: error: reference to copy is ambiguous
>              Array.copy(EMPTY_INT_ARRAY, 0, naInt, 0, 0);
>                   ^
>    both method <Z#1>copy(Array<Z#1>,long,Object,int,int) in Array and method <Z#2>copy(Object,int,Array<Z#2>,long,int) in Array match
>    where Z#1,Z#2 are type-variables:
>      Z#1 extends Object declared in method <Z#1>copy(Array<Z#1>,long,Object,int,int)
>      Z#2 extends Object declared in method <Z#2>copy(Object,int,Array<Z#2>,long,int)
>
> I am not sure why javac pick those two, while I actually want the third one,
>
> copy(Array<Z>, long, Array<Z>, long, long);
>
> A simple fix is simply to cast the last argument.
>              Array.copy(EMPTY_INT_ARRAY, 0, naInt, 0, 0L);
>
> Perhaps we can simply rename cop between Array to copyNative or something. Suggestions?
>
> Cheers,
> Henry
>
> [1] http://cr.openjdk.java.net/~henryjen/panama/8226555/0/webrev/