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

Tue Mar 13 11:02:28 UTC 2018

Hi Erik & Per,

yes, I agree with getting rid of Runtime1::arraycopy(). Let's take it to
another RFE.

Let's try the newfangled submit repo and push (mostly) my first patch :-)

Thanks for reviewing!
Cheer, Roman

> Hi Roman,
> 
> Ideally I'd like to get rid of Runtime1::arraycopy. It seems like a
> medium slow path of C1 never used by any platforms other than S390 and
> PPC, which look like the only platforms that lack a generic arraycopy
> stub from the StubRoutines. I think that S390 and PPC could easily just
> jump to the C1 arraycopy stub instead and take the slowpath to the
> normal native arraycopy that enters through the JNI interface instead.
> 
> If the performance of this C1 arraycopy path on S390 and PPC are
> crucial, then they should probably implement stubs for it like the other
> platforms. If the performance of this is not crucial (I suspect this is
> the case), then it should not harm to take the normal entry through JNI
> for generic arraycopy using C1.
> 
> But I think trying to get rid of that path could be a follow-up RFE
> instead.
> 
> Looks good.
> 
> Thanks,
> /Erik
> 
> On 2018-03-13 11:02, 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
>>>>>>>>>>>>>>
>>
> 

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