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

[Roman Kennke]

The following changeset addresses all the comments that have been
brought up so far in review. It also makes the arraycopy word-atomic. It
casts pointers to elements of same size to their respective counterparts
in Copy::conjoint_XXX_atomic(), i.e. jboolean* -> jbyte*, jchar* ->
jshort*, jfloat* -> jint* and jdouble* -> jlong*.

Differential:
http://cr.openjdk.java.net/~rkennke/8198445/webrev.01.diff/
Full:
http://cr.openjdk.java.net/~rkennke/8198445/webrev.01/

Still passes tier1 tests.

OK to go?

Roman


> Hi 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
>>>>>>>>
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20180308/48c79b95/signature.asc>