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

[Roman Kennke]

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


-------------- 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/2afda9af/signature.asc>