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

[Roman Kennke]

Sorry, I replied that to the wrong thread. Discard it please ;-)

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