RFR: JDK-8198445: Access API for primitive/native arraycopy (original) (raw)

Per Liden per.liden at oracle.com
Tue Mar 13 10:48:04 UTC 2018

Looks good to me.

(Erik and I discussed removing Runtime1::arraycopy() completely, and let those few platforms that doesn't implement this in C1 fall back to the VM implementation instead, but let's take that as a separate RFE).

/Per

On 03/13/2018 11:02 AM, 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 ARRAYCOPYATOMIC uses the natural atomicity of the element alignment by using Copy::conjointmemoryatomic(). 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::conjointmemoryatomic, which is precisely what typeArrayKlass.cpp maps such an arraycopy to today. Assuming elements are aligned to their natural size (which they are), Copy::conjointmemoryatomic 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::conjointXXXatomic() 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 ARRAYCOPYATOMIC maps to Copy::conjointmemoryatomic 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 c1Runtime1.cpp, when we can just as well call typeArrayOop::copyarray() 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 arraycopyconjointatomic defintions for extra types like jfloat, jdouble, jboolean, etc, which in turn would be missing the same Copy::conjointjFluffsatomic() 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 ARRAYCOPYATOMIC 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 conjointmemoryatomic when the ARRAYCOPYATOMIC 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 ARRAYCOPYATOMIC decorator. If that comment is correct (I do not know if it is), then the ARRAYCOPYATOMIC 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/c1Runtime1.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::copyarray(), 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::XYZaddr() + 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

More information about the hotspot-gc-dev mailing list