RFR: JDK-8198445: Access API for primitive/native arraycopy

[Per Liden]

On 03/06/2018 01:47 PM, Per Liden wrote:
> Hi Roman,
> 
> On 03/06/2018 12:56 PM, Roman Kennke wrote:
>> Currently, the Access API is only used for oop-arraycopy, but not for
>> primitive arraycopy. GCs might want to intercept this too, e.g. resolve
>> src and dst arrays.
>>
>> There *is* an implementation of primitive arraycopy in the Access API,
>> but it doesn't even compile, because Raw::arraycopy() does not take src
>> and dst oop operands, but it's called like that. The only reason why
>> this does not blow up (I think) is that because nobody calls it, the
>> compiler doesn't even get there.
>>
>> This change fixes the Access API/impl and adds the relevant calls into
>> it (in C1 and runtime land). C2 uses arraycopy stubs (which cannot be
>> handled here) or calls out to the ArrayKlass::copy_array(), which should
>> be covered with this change.
>>
>> It should be possible to use the same Access API for Java-array <->
>> native-array bulk transfers, which currently use the rather ugly
>> typeArrayOop::XYZ_addr() + memcpy() pattern. I'll address that in a
>> separate change though.
>>
>> http://cr.openjdk.java.net/~rkennke/8198445/webrev.00/
> 
> src/hotspot/share/c1/c1_Runtime1.cpp
> ------------------------------------
> 
> -    memmove(dst_addr, src_addr, length << l2es);
> -    return ac_ok;
> +    return HeapAccess<>::arraycopy(arrayOop(src), arrayOop(dst), 
> src_addr, dst_addr, length << l2es) ? ac_ok : ac_failed;
> 
> I think we should always return ac_ok here, and change 
> Heap<>::arraycopy() to return void, since it will always return true for 
> primitive types.
> 
> 
> src/hotspot/share/gc/shared/barrierSet.hpp
> ------------------------------------------
> -      return Raw::arraycopy(src_obj, dst_obj, src, dst, length);
> +      return Raw::arraycopy(src, dst, length);
> 
> I'd suggest that we adjust the Raw::arraycopy() signature instead to 
> accept these arguments (even if they are later unused), to keep this 
> symmetric with oop_arraycopy() and avoid doing these types of 
> "decisions" in this layer.
> 
> 
> src/hotspot/share/oops/accessBackend.cpp
> ----------------------------------------
> 
> +  void arraycopy_conjoint<char>(char* src, char* dst, size_t length) {
> +    Copy::conjoint_jbytes(src, dst, length);
> +  }
> 
> This looks like an unnecessary addition. I suggest you adjust the code 
> in src/hotspot/share/c1/c1_Runtime1.cpp to use jbyte* instead char* in 
> the address calculations. I.e. these lines:
> 
>       char* src_addr = (char*) ((oopDesc**)src + ihs) + (src_pos << l2es);
>       char* dst_addr = (char*) ((oopDesc**)dst + ihs) + (dst_pos << l2es);
> 

I missed this one...

src/hotspot/share/oops/typeArrayKlass.cpp
-----------------------------------------

-  Copy::conjoint_memory_atomic(src, dst, (size_t)length << l2es);
+  HeapAccess<>::arraycopy(s, d, src, dst, (size_t)length << l2es);

You're loosing the atomic part here, which should translate to an 
ARRAYCOPY_ATOMIC decorator.

Erik will send out a proposal for how to deal with the atomic stuff when 
we've erased the type information, like in C1's stub.

/Per


> cheers,
> Per
> 
>>
>> Tests: tier1 ok
>>
>> Please review!
>> Thanks, Roman
>>