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

[Per Liden]

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
>