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

[Erik Österlund]

Hi Per,

While I think any potential callsite to RawAccess<>::resolve() would 
always be rather nonsense, I do not mind moving it to Raw.

Incremental webrev:
http://cr.openjdk.java.net/~eosterlund/8198286/webrev.00_01/

Full webrev:
http://cr.openjdk.java.net/~eosterlund/8198286/webrev.01/

Thanks,
/Erik

On 2018-02-21 12:31, Per Liden wrote:
> 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
>>