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

Wed Feb 21 13:28:01 UTC 2018

Looks good to me too. Thanks!!

Roman

On Wed, Feb 21, 2018 at 2:18 PM, Per Liden <per.liden at oracle.com> wrote:
> 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
>>>>
>>>>
>>
>