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

[Per Liden]

Looks good to me.

(Erik and I discussed removing Runtime1::arraycopy() completely, and let 
those few platforms that doesn't implement this in C1 fall back to the 
VM implementation instead, but let's take that as a separate RFE).

/Per

On 03/13/2018 11:02 AM, Roman Kennke wrote:
> Hi Erik,
> 
> thanks for doing this! It looks good to me!
> 
> I'm putting this up here again, with commit msg fixed:
> http://cr.openjdk.java.net/~rkennke/8198445/webrev.03/
> 
> Can I get (final?) reviews? And then I'd put it through the new submit
> repo and push it?
> 
> Thanks, Roman
> 
> 
>> Hi Roman,
>>
>> I would love to avoid using the switch to enumerate all different Java
>> types you could possibly have.
>> How about this? (was easier to cook something together than explain in
>> detail what I had in mind)
>>
>> Webrev:
>> http://cr.openjdk.java.net/~eosterlund/8198445/webrev.00/
>>
>> The idea is to let Access recognize arraycopy with element type "void",
>> and then perform a conservative byte arraycopy, that when
>> ARRAYCOPY_ATOMIC uses the natural atomicity of the element alignment by
>> using Copy::conjoint_memory_atomic().
>>
>> Thanks,
>> /Erik
>>
>> On 2018-03-08 21:12, Roman Kennke wrote:
>>> Am 08.03.2018 um 14:13 schrieb Erik Österlund:
>>>> Hi Roman,
>>>>
>>>> On 2018-03-08 11:25, Roman Kennke wrote:
>>>>> Wouldn't that lead to mis-aligned accesses when you try to copy an
>>>>> array
>>>>> of, e.g., bytes, but shoehorn it to pointer-size? Or am I
>>>>> misunderstanding?
>>>> No it would not. The element type is void, not void*. Because the
>>>> element type is type erased. And that would map to
>>>> Copy::conjoint_memory_atomic, which is precisely what typeArrayKlass.cpp
>>>> maps such an arraycopy to today. Assuming elements are aligned to their
>>>> natural size (which they are), Copy::conjoint_memory_atomic is
>>>> guaranteed to be at least as atomic as you need your elements to be, up
>>>> to an element size of 8.
>>> Ah, ok. Alright, so here comes rev.02:
>>>
>>> Differential:
>>> http://cr.openjdk.java.net/~rkennke/8198445/webrev.02.diff/
>>> Full:
>>> http://cr.openjdk.java.net/~rkennke/8198445/webrev.02/
>>>
>>> Still passes tier1 tests.
>>>
>>> Ok now?
>>>
>>> Cheers, Roman
>>>
>>>
>>>>> An idea would be to copy equal-sized elements using the corresponding
>>>>> Copy::conjoint_XXX_atomic() calls, e.g. char and short would use the
>>>>> short version, boolean and byte would use the byte version (or is
>>>>> boolean sometimes int?!), float and int would use the int version and
>>>>> long and double the long version. WDYT?
>>>> I'm not sure I see what the advantage of that would be.
>>>>
>>>> Thanks,
>>>> /Erik
>>>>
>>>>> Roman
>>>>>
>>>>>
>>>>>> My spontaneous idea was to have an overload for type erased void* that
>>>>>> when called with ARRAYCOPY_ATOMIC maps to Copy::conjoint_memory_atomic
>>>>>> which AFAIK is conservatively atomic regardless of what the precise
>>>>>> element type is.
>>>>>>
>>>>>> But if you have better ideas, I am open for suggestions.
>>>>>>
>>>>>> Thanks,
>>>>>> /Erik
>>>>>>
>>>>>> On 2018-03-06 17:03, Roman Kennke wrote:
>>>>>>> Am 06.03.2018 um 14:35 schrieb Roman Kennke:
>>>>>>>> Makes me wonder: why attempt to be smart in c1_Runtime1.cpp, when
>>>>>>>> we can
>>>>>>>> just as well call typeArrayOop::copy_array() and have it do the
>>>>>>>> right
>>>>>>>> thing? Or go even further and also do it for oop-arraycopy?
>>>>>>> Something like:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~rkennke/8198445/8198445-1.patch
>>>>>>>
>>>>>>> This wouldn't compile because of bunch of missing
>>>>>>> arraycopy_conjoint_atomic defintions for extra types like jfloat,
>>>>>>> jdouble, jboolean, etc, which in turn would be missing the same
>>>>>>> Copy::conjoint_jFluffs_atomic() which drags in a bunch of platform
>>>>>>> specific stuff... and my question before I go there is: do we want
>>>>>>> all
>>>>>>> that? Or can you think of a better way to solve it?
>>>>>>>
>>>>>>> Roman
>>>>>>>
>>>>>>>> Roman
>>>>>>>>
>>>>>>>>> The ARRAYCOPY_ATOMIC decorator makes the arraycopy atomic over the
>>>>>>>>> size
>>>>>>>>> of the passed in elements.
>>>>>>>>> In this case, it looks like the address has been type erased to
>>>>>>>>> void*,
>>>>>>>>> and hence lost what the element size was. There is currently no
>>>>>>>>> overload
>>>>>>>>> accepted for type erased element - only accurate elements {jbyte,
>>>>>>>>> jshort, jint, jlong}.
>>>>>>>>>
>>>>>>>>> So it looks like an overload must be added to accept type erased
>>>>>>>>> void*
>>>>>>>>> elements and make that call conjoint_memory_atomic when the
>>>>>>>>> ARRAYCOPY_ATOMIC decorator is passed in.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> /Erik
>>>>>>>>>
>>>>>>>>> On 2018-03-06 13:56, David Holmes wrote:
>>>>>>>>>> On 6/03/2018 10:54 PM, Erik Österlund wrote:
>>>>>>>>>>> Hi David,
>>>>>>>>>>>
>>>>>>>>>>> It is atomic with the ARRAYCOPY_ATOMIC decorator. If that
>>>>>>>>>>> comment is
>>>>>>>>>>> correct (I do not know if it is), then the ARRAYCOPY_ATOMIC
>>>>>>>>>>> decorator
>>>>>>>>>>> should probably be used here.
>>>>>>>>>> If that code implements a Java array copy then yes it is
>>>>>>>>>> required to
>>>>>>>>>> be 32-bit atomic. Do you need the decorator to get 32-bit
>>>>>>>>>> atomicity?
>>>>>>>>>>
>>>>>>>>>> David
>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> /Erik
>>>>>>>>>>>
>>>>>>>>>>> On 2018-03-06 13:48, David Holmes wrote:
>>>>>>>>>>>> Hi Roman,
>>>>>>>>>>>>
>>>>>>>>>>>> Not a review as I'm not familiar enough with the Access API,
>>>>>>>>>>>> but in
>>>>>>>>>>>> src/hotspot/share/c1/c1_Runtime1.cpp the comments above the
>>>>>>>>>>>> changed
>>>>>>>>>>>> code need updating - probably deleting. I assume the Access API
>>>>>>>>>>>> arraycopy is atomic?
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>> David
>>>>>>>>>>>>
>>>>>>>>>>>> On 6/03/2018 9: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/
>>>>>>>>>>>>>
>>>>>>>>>>>>> Tests: tier1 ok
>>>>>>>>>>>>>
>>>>>>>>>>>>> Please review!
>>>>>>>>>>>>> Thanks, Roman
>>>>>>>>>>>>>
>>>
>>
> 
>