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

[Roman Kennke]

Indeed, I just checked our code base, and the only non-GC-internal use
of that API is inside referenceProcessor.cpp, and we can probably live
without that.
Thanks, Roman

On Tue, Feb 20, 2018 at 11:00 PM, Per Liden <per.liden at oracle.com> wrote:
> Hi Roman,
>
>
> On 02/20/2018 06:25 PM, Erik Osterlund wrote:
>>
>> Hi Roman,
>>
>>> On 20 Feb 2018, at 17:44, Roman Kennke <rkennke at redhat.com> wrote:
>>>
>>> Hi Eric,
>>>
>>> On Tue, Feb 20, 2018 at 5:41 PM, Erik Österlund
>>> <erik.osterlund at oracle.com> 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.
>>>
>>>
>>> I will review it ASAP.
>>>
>>> Just a quick note: In Shenandoah we have introduced an API in
>>> BarrierSet to check bool BarrierSet::is_safe(oop) which checks exactly
>>> the to-space invariant, i.e. if it's safe to access that oop for
>>> writing. I'd like that to be upstream if possible, and maybe this
>>> would be a good opportunity to add it? Maybe rename is_stable() or
>>> assert_stable() (and maybe keep it under #ifdef ASSERT)?
>>
>>
>> I see. That sounds like a useful tool to have. I’m not sure where I should
>> be calling it from though. In the _addr calls that go through resolve(),
>> is_stable() trivially returns true. That leaves only a few gc callsites that
>> use *_addr_raw() - oop iterate and reference processing to put the assert
>> in. As for oop iterate, I don’t think we should check is_stable. Imagine,
>> e.g. a generational concurrently compacting GC that uses oop iterate to scan
>> cards, and expects to see only addresses in that card boundary, whether they
>> are to-space invariant or not (been there, done that). The addresses in that
>> card boundary may or may not be stable. As for reference processing, I can
>> see that it could be nice to know the addresses are stable. You probably
>> know better than me where to put these asserts as you have already done it.
>
>
> I think I agree with Erik. It sounds like something that is useful to
> Shenandoah, but it also sounds strange that someone outside of Shenandoah
> code would be calling it? I.e. I'm not sure this should be part of the
> public facing BarrierSet, as it more sounds like something that should only
> be used internally in Shenandoah?
>
> cheers,
> Per
>
>
>>
>> Thanks,
>> /Erik
>>
>>> Thanks for doing this!
>>>
>>> Roman
>>
>>
>