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

[Henry Jen]

OK, in that case, I’ll push updated web rev[1] with javadoc.

[1] http://cr.openjdk.java.net/~henryjen/panama/8226555/2/webrev/

Cheers,
Henry

> On Jun 25, 2019, at 6:12 PM, Maurizio Cimadamore <maurizio.cimadamore at oracle.com> wrote:
> 
> Sorry - I missed that the check was referring to array element type; in that case yes, not much need to check for void.
> 
> Maurizio
> 
> On 25/06/2019 19:31, Henry Jen wrote:
>> Not clear if you meant we should allow void or should prevent it.
>> 
>> I am not sure we need special treatment for that because,
>> 
>> 1. Is there even a way to get a void[]?
>> 2. Util.sizeof() will throw IAE with void
>> 3. Boxed type is not considered primitive
>> 
>> So I think we are covered?
>> 
>> Cheers,
>> Henry
>> 
>>> On Jun 24, 2019, at 4:44 AM, Maurizio Cimadamore <maurizio.cimadamore at oracle.com> wrote:
>>> 
>>> Looks good - you forgot to check for 'void' in ofPrimitiveArray, that will surely lead to issues.
>>> 
>>> Maurizio
>>> 
>>> 
>>> On 22/06/2019 08:14, Henry Jen wrote:
>>>> Please have a look at updated webrev[1].
>>>> 
>>>> I like the idea to have separate API for bulk and element-wise copy, in this webrev, we simply add the needed capability for bulk copy.
>>>> 
>>>> I don’t see a good reason to have non-primitive type support, as I don’t think we should encourage move native aggregate data into Java, developer is likely only deal with native Array for those.
>>>> 
>>>> The only reason to have element-wise copy, is probably like to composite with a mapping function that would convert a Java Object into a native Layout carrier type. I had done something like that before, we can save that for next.
>>>> 
>>>> Javadoc is needed, I’ll add that later, first let’s see if those make sense.
>>>> 
>>>> Array.ptr() is moved from BoundedArray into Array as default method.
>>>> Array.ofPrimitiveArray() can get you a Array from primitive Java array
>>>> Array.slice() to get you a slice of the Array
>>>> 
>>>> Array.copy() is a just equivalent and easier to use than calling Pointer.copy(src.slice(), dst.slice()).
>>>> 
>>>> Let me know what you think.
>>>> 
>>>> [1] http://cr.openjdk.java.net/~henryjen/panama/8226555/1/webrev/
>>>> 
>>>> Cheers,
>>>> Henry
>>>> 
>>>>> On Jun 21, 2019, at 12:46 AM, Maurizio Cimadamore <maurizio.cimadamore at oracle.com> wrote:
>>>>> 
>>>>> 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/