RFR: 8198286: Direct memory accessors in typeArrayOop.hpp should use Access API

[Roman Kennke]

Kim,
those are good suggestions about using const to prevent writing to
memory that's supposed to be read from. I will give them a try.

Folding this into arraycopy seems tricky:
- While it is often used for bulk memory transfer, it is not always
the case. For example, it's also used for stuff like JNI
GetXXXArrayElements and similar methods.
- Arraycopy copies between Java objects, while this here usually
copies from a Java object to native memory or vice versa. With a
little bit of thought we might be able to come up with a consistent
API for arraycopy and Java<->native memory transfers. But we'd need
something for the JNI stuff above anyway. I need to re-work the
arraycopy Access API anyway, maybe I can make it work for the direct
memory transfers too, and make those places in code use that new
arraycopy API too.

Thanks for looking at it!
Roman

On Sat, Feb 17, 2018 at 8:54 PM, Kim Barrett <kim.barrett at oracle.com> wrote:
>> On Feb 16, 2018, at 11:18 AM, Roman Kennke <rkennke at redhat.com> wrote:
>>
>> The direct memory accessors in typeArrayOop.hpp, which are usually
>> used for bulk memory access operations, should use the Access API, in
>> order to give the garbage collector a chance to intercept the access
>> (for example, employ read- or write-barriers on the target array).
>> This also means it's necessary to distinguish between write-accesses
>> and read-accesses (for example, GCs might want to use a
>> copy-on-write-barrier for write-accesses only).
>>
>> This changeset introduces two new APIs in access.hpp: load_at_addr()
>> and store_at_addr(), and links it up to the corresponding X_get_addr()
>> and X_put_addr() in typeArrayOop.hpp. All uses of the previous
>> X_addr() accessors have been renamed to match their use (load or store
>> of primitive array elements).
>>
>> The changeset is based on the previously proposed:
>> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2018-February/026426.html
>>
>> Webrev:
>> http://cr.openjdk.java.net/~rkennke/8198286/webrev.00/
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8198286
>>
>> Please review!
>>
>> Thanks,
>> Roman
>
> Not a review, but suggestions and questions.
>
> In order to reduce opportunities for bugs, I suggest a change of
> signatures:
>
>    T* T_at_get_addr(int which) const
>    T* T_at_put_addr(int which) const
> =>
>    const T* T_at_get_addr(int which) const
>    T* T_at_put_addr(int which)
>
> This prevents (a) writing to the result of a get_addr, (b) writing to
> a constant object.  The latter is probably matters less, given our
> level of const-cleanliness.
>
> Obviously there will be fanout, particularly from the get_addr
> change, as there are some missing const qualifiers.  Addressing those
> is probably a good thing though.
>
> But I'm also wondering whether the overall approach is right.  If this
> is about bulk transfers, as suggested in the CR and RFR, then shouldn't
> this be involving Access::arraycopy?  Why isn't changing these copies
> to use that operation, along with an appropriate implementation  of
> that operation by the barrier set, a better solution?
>