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

[Thomas Schatzl]

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