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

[Per Liden]

Thanks Erik!

Looks good!

/Per

On 02/21/2018 12:51 PM, Erik Österlund wrote:
> 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
>>>
>