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

[Per Liden]

Hi Erik,

As we discussed, coming up with a good name for the new Access call is 
really hard. All good/descriptive alternatives I can come up with tend 
to be way to long. So, next strategy is to pick something that fits into 
the reset of the API. With this in mind I'd like to suggest we just name 
it: oop Access<>::resolve(oop obj)

The justification would that this this matches the one-verb style we 
have for the other functions (load/store/clone) and it seems that you 
anyway named the internal parts just "resolve", such as BARRIER_RESOLVE, 
and resolve_func_t.

What do you think?

cheers,
Per

On 02/19/2018 06:08 PM, Erik Osterlund wrote:
> Hi Roman,
> 
> I see there is a need to resolve a stable address for some objects to bulk access primitives. The code base is full of assumptions that no barriers are needed for such address resolution. It looks like the proposed approach is to one by one hunt down all such callsites. I could find some places where such barriers are missing.
> 
> To make the code as maintainable as possible, I would like to propose a slightly different take on this, and would love to hear if this works for Shenandoah or not. The main idea is to annotate places where we do *not* want GC address resolution for internal pointers to objects, instead of where we want it, as it seems to be the common case that we do want to resolve the address.
> 
> In some more detail:
> 
> 1) Rip out the *_addr fascilities not used (a whole bunch on oopDesc).
> 2) Ignore the difference between read/write resolution (write resolution handles both reads and writes). Instead introduce an oop resolve_stable_addr(oop) function in Access. This makes it easier to use.
> 3) Identify as few callsites as possible for this function. I'm thinking arrayOop::base() and a few strange exceptions.
> 4) Identify the few places where we explicitly do *not* want address resolution, like calls from GC, and replace them with *_addr_raw variants.
> 5) Have a switch in barrierSetConfig.hpp that determines whether the build needs to support not to-space invariant GCs or not.
> 
> With these changes, the number of callsites have been kept down to what I believe to be a minimum. And yet it covers some callsites that you accidentally missed (e.g. jvmciCodeInstaller.cpp). Existing uses of the various *_addr fascilities can in most cases continue to do what they have done in the past. And new uses will not be surprised that they accidentally missed some barriers. It will be solved automagically.
> 
> Webrev:
> http://cr.openjdk.java.net/~eosterlund/typearray_resolve/webrev.00/
> 
> Please let me know what you think about this style and whether that works for you or not. I have not done proper testing yet, but presented this patch for quicker turn-around so we can synchronize the direction first.
> 
> Thanks,
> /Erik
> 
>> On 16 Feb 2018, at 17:18, 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