RFR(s): 8029255: G1: Reference processing should not enqueue references on the shared SATB queue

[Per Liden]

Thanks Thomas!

cheers,
/Per

On 2014-01-07 15:38, Thomas Schatzl wrote:
> Hi,
>
> On Mon, 2013-12-02 at 14:44 +0100, Per Liden wrote:
>> Hi Thomas, thanks for looking at this.
>>
>> Comments inline below.
>>> Background (for others too, it took me a while to understand the
>>> reference processing code):
>>> The problem why the code cannot use the normal barrier code in all these
>>> cases is that collection uses the next and discovered fields of
>>> java.lang.Reference for its own purpose, putting references from the
>>> cset into the queues.
>>>
>>> In this process we loose the previous values too, that's why the code
>>> cannot just apply the normal barrier - before activating the shared
>>> reference queue in 8029162 it was not a problem because the "old"
>>> references were dropped immediately when trying to add.
>>>
>>> It seems that this is not an issue in regards to SATB as these fields
>>> are handled somewhat specially always anyway.
>>>
>>> I would prefer if setting the "next" field of the reference were
>>> encapsulated into a method like ReferenceProcessor::set_discovered (e.g.
>>> ReferenceProcessor::set_next()), and used instead of inlining
>>> set_raw/apply-post-barrier code all the time.
>> That would be nice, but as far as I can see all three cases where some
>> sort of set_next is done all have different barrier needs, so it I'm not
>> sure it would be a simplification in the end. Same is true for
>> ReferenceProcessor::set_discovered, it's only used in two places but we
>> do set the discovered field in many other paths, but those paths have
>> different needs (set by oopDesc::atomic_exchange_oop, with/without
>> barriers, etc). I'd almost suggest that we remove
>> ReferenceProcessor::set_discovered to avoid any confusion. But actually,
>> I'd rather touch as little as possible of this code at this stage, as I
>> have the feeling I might not understand of all aspects of it and
>> cleaning it up is a fairly substantial project in itself. The changes
>> I've done should only affect G1 as it's only a matter of removing some
>> pre-barriers.
> Okay, let's leave the cleanup out in this CR. My concern is that all the
> (existing) inlining makes the code a lot harder to read than need be.
>
>>> There is already a missing call to the post-barrier in
>>> referenceProcessor.cpp:366  when storing the self-loop value (That is
>>> benign since it does not add a new object to the object graph, but if
>>> that is kept, the reason should be documented).
>> Yes, will add a comment to clarifying that. The post-barrier was left
>> out intentionally as it would just end up being a noop.
> Thanks.
>
>>> Other issues:
>>> - (this is more a question than an issue)
>>> DiscoveredListIterator::make_active() has some special handling for
>>> setting the next field of java.lang.Reference. In particular it does not
>>> call the post-barrier because this will fail CT verification in G1.
>>> What's the difference between that field and the discovered field in
>>> that respect? Also the original code executed the post-barrier on the
>>> next field (e.g. referenceProcessor.cpp:366), so if there were a
>>> problem, it should already occur, shouldn't it?
>> Good question. Looking at InstanceRefKlass::update_nonstatic_oop_maps()
>> where the oop map is set to ignore all fields except "queue", so I'm not
>> sure I understand why any post-barriers would be needed as the dirty
>> card would basically be ignored anyway, no?
> Did not know about the code in
> InstanceRefKlass::update_nonstatic_oop_maps(). So it seems fine :)
>
>>> - there are places where the code for
>>> ReferenceProcessor::set_discovered() is inlined for no apparent reason
>>> (e.g. at least referenceProcessor.cpp:374, and
>>> referenceProcessor.cpp:1256). Could this change (or the one proposed
>>> below) also clean this up?
>> Note that ReferenceProcessor::set_discovered() has slightly different
>> semantics as it only does a barrier if _discovered_list_needs_barrier is
>> enabled, which it is only for G1's CM reference processor.
>>> - with the change in 8029162 I do not think the code that is enabled by
>>> setting _pending_list_uses_discovered_field to false works any more (if
>>> it ever worked with G1, see the very first thought in this list). It
>>> will trigger the same assertion that this fix wants to fix.
>>> If the "old behavior" is not needed any more (I think so), please remove
>>> that code first in another CR. The alternative is to fix it I guess.
>> Yes, I concluded that this code is never used anymore, so I ignored it
>> in this change and added it to the list of "things to cleanup/remove". I
>> can register a separate CR for cleaning that out.
> Please do.
>
> Thomas
>
>