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

[Roman Kennke]

Alright, I figured it out. The humongous handling is correct, but the
accounting of seqnum_at_gc_start is not. We currently set it in
ShenandoahGCPauseMark, which updates the numbers at the beginning and
end of each *pause*, not GC cycle. However, doing it in
ShenandoahGCSession, which would cover the whole cycle, is not safe
because it happens from the scheduler thread while Java is running
(and updating the counter). It needs to be done during the init- and
final- pauses of the cycle in order to be safe and correct. I've done
it in traversal using this patch:

https://paste.fedoraproject.org/paste/SZ2tqntg4l5T3TAIdtQ2uw

The only other user of that counter seems to be the generational and
LRU heuristics, and I'm wondering if they work by accident, or not
really at all, but a complete fix would do what I did to traversal
also to partial GC. Something for tomorrow.

Cheers, Roman

On Tue, Feb 20, 2018 at 11:06 PM, Roman Kennke <rkennke at redhat.com> wrote:
> 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
>>>
>>>
>>