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

[Per Liden]

Hi Erik,

Looks good, just one small-ish request. Can we please push the default 
implementation down to the raw layer, so that the two instances of 
"return obj;" becomes "return Raw::resolve(obj);". From my point of 
view, that makes this symmetric with the other functions and helps me 
think/reason about this.

cheers,
Per

On 02/20/2018 05:41 PM, Erik Österlund wrote:
> 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
>