RFR(M): 7004681: G1: Extend marking verification to marking phase of Full GCs

[Y. S. Ramakrishna]

Looks good, except for one comment for your consideration/thought.
It appeared to me that since the verification of mark sweep
is now called after the stringtable, symbol table
and system dictionary, code cache and other weak roots
structures have been visited and cleaned up, the
process_strong_roots code might in fact want to verify
those structures with the rootsCL as well, via enabling
the relevant bits in the scanning option:

i.e. instead of your current:-

2835     // We apply the relevant closures to all the oops in the
2836     // system dictionary, the string table and the code cache.
2837     int so = SharedHeap::SO_AllClasses | SharedHeap::SO_Strings | SharedHeap::SO_CodeCache;
2838
2839     if (vo == VerifyOption_G1UseMarkWord) {
2840       // We need to match the parameters used in G1MarkSweep::mark_sweep_phase1
2841       // to match the set of marked roots. Otherwise we could see spurious
2842       // verification failures where a (non-marked or external) root referencing
2843       // a dead oop. For example we could have an entry in the dictionary
2844       // pointing to an unmarked class, or we could have an unmarked klassOop
2845       // in Perm referencing its class loader. This is not a marking failure
2846       // as the dead objects will be cleaned up during the current full GC.
2847       so = SharedHeap::SO_SystemClasses;
2848     }

simply keep:

    const int so = SharedHeap::SO_AllClasses | SharedHeap::SO_Strings | SharedHeap::SO_CodeCache;


I read yr comment as saying these weak root structures may
still be left pointing to dead objects, but it appears from
the mark sweep phase 1 code that at the end of phase 1
we have in fact cleaned up all these weak roots left pointing
to dead objects by now. Anyway, worth checking this suggestion,
just in case it allows a slightly tighter verification than
what you might otherwise get.

Otherwise looks good.

-- ramki


On 04/28/11 10:59, John Cuthbertson wrote:
> Hi All,
> 
> Another new webrev for this CR can be found at: 
> http://cr.openjdk.java.net/~johnc/MarkSweep-VerifyMark/webrev.5/
> 
> The changes since the last one are just a reconciliation, in 
> g1CollectedHeap.cpp, between the previous changes and Ramki's changes 
> for 7039089.
> 
> Testing: I ran the GC test suite overnight with VerifyDuringGC and a low 
> initial occupancy threadhold (5%)
> 
> Thanks,
> 
> JohnC
> 
> On 04/26/11 12:01, John Cuthbertson wrote:
>> Hi All,
>>
>> A new webrev is here: 
>> http://cr.openjdk.java.net/~johnc/MarkSweep-VerifyMark/webrev.4/
>>
>> The changes made since the last webrev include a suggestion from Tony 
>> to fold the check from G1CollectedHeap::checkConcurrentMark into the 
>> VerifyObjsInRegionClosure and remove the checkConcurrentMark routine 
>> and associated closure.
>>
>> Thanks,
>>
>> JohnC
>>
>> On 04/22/11 17:33, John Cuthbertson wrote:
>>> Hi Everyone,
>>>
>>> A new webrev for this CR can be found at: 
>>> http://cr.openjdk.java.net/~johnc/MarkSweep-VerifyMark/webrev.3.
>>>
>>> I'd like to get at least another person look over these changes (Tony 
>>> has already looked at an earlier version). The latest webrev includes 
>>> skipping the region set verification if the verification was called 
>>> from a full GC (in G1 the region sets are torn down at the start of 
>>> the full GC and so the verification will give a false failure).
>>>
>>> Testing: GC test suite with +VerifyDuringGC with and without G1.
>>>
>>> Thanks,
>>>
>>> JohnC
>>
>