RFR(M/L): 6484982: G1: process references during evacuation pauses

[Tony Printezis]

Hi all,

First, thanks to Bengt and Stefan for looking at this webrev! A few 
comments below....

On 09/12/2011 07:42 PM, John Cuthbertson wrote:
>>
>> referenceProcessor.cpp
>> - Not strictly related to your change, but it would be nice to change 
>> the name of _discoveredSoftRefs to _discoveredRefs and introduce a 
>> separate _discoveredSoftRefs. It is quite confusing that 
>> _discoveredSoftRefs is being used for all reference types.
>>
>
>
> I like this suggestion but I would rather do the renaming as a 
> separate RFE because the change is already ~1600 lines.

Totally agreed. This is an orthogonal cleanup change that should be done 
on a separate CR.

>
>> g1CollectedHeap.cpp
>>
>> - What does this comment mean? 
>> instanceRefKlass::process_discovered_references does not exist.
>>     // We want to discover references, but not process them yet.
>>     // This mode is disabled in
>>     // instanceRefKlass::process_discovered_references if the
>>     // generation does some collection work, or
>>     // instanceRefKlass::enqueue_discovered_references if the
>>     // generation returns without doing any work.
>>
>
> I think the comment is out of date. It has been in the G1 sources 
> since I as long as I can remember. Removed.

Thanks. :-)

>> - The setup of the STW reference processor uses an is_alive_closure. 
>> This seems to be correct and efficient, but why do the other full and 
>> STW reference processors in HotSpot skip this?
>
> The is_alive_non_header is an optimization for collectors that use an 
> external data structure (bitmap) rather than the object's header to 
> record liveness information. Only G1 and CMS have this external data 
> structure. 

...as well as Parallel Old.

> So when we try to 'discover' a reference we can see if its referent is 
> already alive and, if so, skip the 'discovery' and treat the reference 
> as a regular oop.What this buys us is less reference objects to 
> process. The other collectors use an object's mark word to record 
> liveness info during the GC. With these other GCs you can't normally 
> tell if the referent is alive while walking the heap marking (and 
> doing discovery) until the reachable object graph has been walked.

>> - The G1_DEBUG define makes the code really difficult to read. Could 
>> this be removed?
>
> OK. It was really helpful while debugging but I suppose it's served 
> it's purpose.

Let me also point out that I removed the current instances of G1_DEBUG 
as part of the ergo policy verbose change.

>
>> - Are these asserts really necessary?
>>   void G1ParScanThreadState::trim_queue() {
>>     assert(_evac_cl != NULL, "not set");
>>     assert(_evac_failure_cl != NULL, "not set");
>>     assert(_partial_scan_cl != NULL, "not set");
>
> Ensuring that the PSS was fully initialized before calling these 
> closures was very helpful. Are they necessary? No. Are they defensive? 
> Yes. I vote to leave them in.

It also depends on the definition of "necessary". Maybe we can prove 
that they cannot happen by quickly looking at the code, which might make 
then "unnecessary". But it's good to have some pre-conditions in such 
places in case we re-organize the code in the future and some of our 
assumptions do not hold any more... In that case they are "necessary". :-)

>>
>> heapRegion.hpp
>> - I guess #if WHASSUP should be removed...
>>
>>
>
> Are you referring to line 593? I thought I had removed it.I can't see 
> it in my workspace.

I can see it in mine.... But please nuke it. :-)


Tony