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

Tue Mar 13 10:02:38 UTC 2018

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

-------------- 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/20180313/bd305950/signature.asc>