RFR: JDK-8198285: More consistent Access API for arraycopy

[Erik Österlund]

Hi Roman,

So the options for this API were:

1) Resolve the addresses as we do today, but you think you get better 
Shenandoah performance if we either
2) Have 3 different access calls, (from heap to native, from native to 
heap, from heap to heap) for copying, or
3) Have 1 access call that can encode the above 3 variants, but looks 
ugly at the call sites.

You clearly went for 3, which leaves the callsites looking rather hard 
to read. It is for example not obvious for me what is going on here 
(javaClasses.cpp line 313):

HeapAccess<>::arraycopy<jbyte>(NULL, 0, reinterpret_cast<const 
jbyte*>(utf8_str), value(h_obj()), 
typeArrayOopDesc::element_offset<jbyte>(0), NULL, length);

...without looking very carefully at the long list of arguments encoding 
what is actually going on (copy from native to the heap). What is worse 
is that this will probably not compile without adding the template 
keyword to the call (since you have a qualified template member function 
behind a template class), like this:
HeapAccess<>::template arraycopy<jbyte>(NULL, 0, reinterpret_cast<const 
jbyte*>(utf8_str), value(h_obj()), 
typeArrayOopDesc::element_offset<jbyte>(0), NULL, length);

...which as a public API leaves me feeling a bit like this:     :C

May I suggest adding an array access helper. The idea is to keep a 
single call through Access, and a single backend point for array copy, 
but let the helper provide the three different types of copying as 
different functions, so that the call sites still look pretty and easy 
to read and follow.

This helper could additionally have load_at, and store_at use array 
indices as opposed to offsets, and hence hide the offset calculations we 
perform today (typically involving checking if we are using compressed 
oops or not).

I am thinking something along the lines of 
ArrayAccess<>::arraycopy_to_native(readable_arguments), 
ArrayAccess<>::arraycopy_from_native(readable_arguments), 
ArrayAccess<>::arraycopy(readable_arguments), which translates to some 
form of Access<>::arraycopy(unreadable_arguments). And for example 
ArrayAccess<>::load_at(obj, index) would translate to some kind of 
HeapAccess<IN_HEAP_ARRAY>::load_at(obj, offset_for_index(index)) as a 
bonus, making everyone using the API jump with happiness.

What do you think about this idea? Good or bad? I guess the question is 
whether this helper should be in access.hpp, or somewhere else (like in 
arrayOop). Thoughts are welcome.

Thanks,
/Erik

On 2018-04-11 19:54, Roman Kennke wrote:
>   Currently, the arraycopy API in access.hpp gets the src and dst oops,
> plus the src and dst addresses. In order to be most useful to garbage
> collectors, it should receive the src and dst oops together with the src
> and dst offsets instead, and let the Access API / GC calculate the src
> and dst addresses.
>
> For example, Shenandoah needs to resolve the src and dst objects for
> arraycopy, and then apply the corresponding offsets. With the current
> API (obj+ptr) it would calculate the ptr-diff from obj to ptr, then
> resolve obj, then re-add the calculate ptr-diff. This is fragile because
> we also may resolve obj in the runtime before calculating ptr (e.g. via
> arrayOop::base()). If we then pass in the original obj and a ptr
> calculated from another copy of the same obj, the above resolution logic
> would not work. This is currently the case for obj-arraycopy.
>
> I propose to change the API to accept obj+offset, in addition to ptr for
> both src and dst. Only one or the other should be used. Heap accesses
> should use obj+offset and pass NULL for raw-ptr, off-heap accesses (or
> heap accesses that are already resolved.. use with care) should pass
> NULL+0 for obj+offset and the raw-ptr. Notice that this also allows the
> API to be used for Java<->native array bulk transfers.
>
> An alternative would be to break the API up into 4 variants:
>
> Java->Java transfer:
> arraycopy(oop src, size_t src_offs, oop dst, size_t dst_offs, size_t len)
>
> Java->Native transfer:
> arraycopy(oop src, size_t src_offs, D* raw_dst, size_t len)
>
> Native->Java transfer:
> arraycopy(S* src_raw, oop dst, size_t dst_offs, size_t len)
>
> 'Unsafe' transfer:
> arraycopy(S* src_raw, D* dst_raw, size_t len)
>
>
> But that seemed to be too much boilerplate copy+pasting for my taste.
> (See how having this overly complicated template layer hurts us?)
>
> Plus, I had a better idea: instead of accepting oop+offset OR T* for
> almost every Access API, we may want to abstract that and take an
> Address type argument, which would be either HeapAddress(obj, offset) or
> RawAddress(T* ptr). GCs may then just call addr->address() to get the
> actual address, or specialize for HeapAddress variants and resolve the
> objs and then resolve the address. This would also allow us to get rid
> of almost half of the API (all the *_at variants would go) and some
> other simplifications. However, this seemed to explode the scope of this
> RFE, and would be better handled in another RFE.
>
> This changes makes both typeArrayKlass and objArrayKlass use the changed
> API, plus I identified all (hopefully) places where we do bulk
> Java<->native array transfers and make them use the API too. Gets us rid
> of a bunch of memcpy calls :-)
>
> Please review the change:
> http://cr.openjdk.java.net/~rkennke/JDK-8198285/webrev.00/
>
> Thanks, Roman
>