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

Wed Feb 21 13:49:44 UTC 2018

Hi Roman,

Thanks for the review.

/Erik

On 2018-02-21 14:28, Roman Kennke wrote:
> 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
>>>>>