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

Roman Kennke rkennke at redhat.com
Tue Mar 13 15:40:24 UTC 2018 

The test came back with the below. Doesn't strike me as being related to
this change.

What to do now? Push it anyway? Re-submit the test? (If so, how?)

Roman

Build Details: 2018-03-13-1330406.roman.source
1 Failed Test
Test	Tier	Platform	Keywords	Description	Task
java/lang/invoke/condy/CondyInterfaceWithOverpassMethods.java 	tier1
windows-x64 	bug8186046 othervm testng 	Error: failed to clean up files
after test 	task
Mach5 Tasks Results Summary

    NA: 0
    FAILED: 0
    PASSED: 74
    UNABLE_TO_RUN: 0
    KILLED: 0
    EXECUTED_WITH_FAILURE: 1
    Test

        1 Executed with failure
            jdk_open_test_jdk_tier1-windows-x64-66 Results: total: 1775,
passed: 1774; failed: 1



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

More information about the hotspot-gc-dev mailing list