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

Thomas Schatzl thomas.schatzl at oracle.com
Tue Jan 7 14:38:46 UTC 2014

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::setdiscovered (e.g. > ReferenceProcessor::setnext()), and used instead of inlining > setraw/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 setnext 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::setdiscovered, 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::atomicexchangeoop, with/without barriers, etc). I'd almost suggest that we remove ReferenceProcessor::setdiscovered 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::makeactive() 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::updatenonstaticoopmaps() 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::setdiscovered() 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::setdiscovered() has slightly different semantics as it only does a barrier if discoveredlistneedsbarrier 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 pendinglistusesdiscoveredfield 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

More information about the hotspot-gc-dev mailing list