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

[Erik Österlund]

Hi Per,

(looping in hotspot-dev as this seems to touch more than runtime)

On 2018-02-20 17:03, Per Liden wrote:
> 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.

Sure.

Here is a full webrev with my proposal for this RFE, now that we agree 
on the direction:
http://cr.openjdk.java.net/~eosterlund/8198286/webrev.00/

Incremental from the prototype I sent out for early turnaround yesterday:
http://cr.openjdk.java.net/~eosterlund/8198286/webrev.00_inc/

It is now enforced that *_addr_raw() functions are to be used by the GC 
only, when the GC knows addresses are stable. All other address 
resolution goes through non-raw address resolution functions that at a 
lower level end up calling the resolve barrier on Access, which can be 
overridden by Shenandoah. There are in total two callers of 
Access<>::resolve: on oopDesc::field_addr and arrayOop::base. The rest 
is derived from that.

@Roman: Hope this works well for Shenandoah.
@Per: Hope you like the new shorter name.

Thanks,
/Erik

> 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