RFR(S): 8010463: G1: Crashes with -UseTLAB and heap verification

[Bengt Rutisson]

Hi John,

Your changes look good to me.

I think your motivation for removing the verification from 
universe2_init() and init_globals() is fine. Actually I wonder why they 
were there in the first place, but they do seem intentionally put in 
there. However, I'm fine with removing them.

About the test. Great that you write a regression test for this! :)

The @summary says that the test uses -XX:+VerifyDuringGC but the command 
line is actually using -XX:+VerifyBeforeGC (which is correct, I think). 
Also, would it make sense to have a separate test that specifies 
-XX:+UseG1GC and checks the output that we expect to see?

One question that is not strictly related to your change:

The code to do the verification in Threads::create_vm() is:

3449   if (VerifyBeforeGC &&
3450       Universe::heap()->total_collections() >= VerifyGCStartAt) {
3451     Universe::heap()->prepare_for_verify();
3452     Universe::verify();   // make sure we're starting with a clean 
slate
3453   }

This is what it looked like before your change as well. But to me this 
looks kind of odd. First, we re-use the flag VerifyBeforeGC even though 
we are not about to do a GC. I can live with that, but it is kind of 
strange. Then we have the check against VerifyGCStartAt. By default this 
is 0 so we will do the verification. But why do we do this check? There 
is no chance that we have been able to do any GC yet, right? So, 
checking against Universe::heap()->total_collections() seems 
unnecessary. We should either check VerifyGCStartAt == 0 or not include 
that flag at all (best choice in my opinion).

Thanks,
Bengt



On 3/21/13 11:28 PM, John Cuthbertson wrote:
> Hi Everyone,
>
> I'm looking for a couple of reviews for the fix for these crashes. The 
> webrev can be found at: 
> http://cr.openjdk.java.net/~johnc/8010463/webrev.0/
>
> Summary:
> During JVM start up, with TLABs disabled, the JVM performs three 
> separate verifications. The first is in universe2_init(), the second 
> is in init_globals(), and the final one is in Threads::create_vm(). 
> With TLABs enabled only one verification is performed during start up 
> -  the one in Threads::create_vm(). These verifications are invoked by 
> the main thread.
>
> The problem here was that the G1 verification code was expecting to be 
> invoked by the VMThread, at a safepoint. When TLABs are disabled the 
> verification code was executed by main thread, triggering the assert. 
> Relaxing the assert (to allow for execution during VM start up) is, 
> unfortunately, not a good solution. There are parts of the root 
> scanning code which assume the JVM is at a safepoint or has completed 
> initialization. For example Threads::oops_do() assumes that the 
> VMThread exists; CodeCache::oops_do() assumes that the CodeCache_lock 
> is being held (or the JVM is at a safepoint); verifying G1's region 
> sets assumes that the Heap_lock is being held (or the JVM is at a 
> safepoint); etc.
>
> When TLABs are enabled the verification from Threads::create_vm() 
> skips verifying parts of the heap. The solution is to skip those parts 
> of the verification even if TLABs are disabled. With just the changes 
> in g1CollectedHeap.cpp, we would see the following:
>
> With -UseTLABS:
>
>> ----> universe2_init
>> [Verifying threads (SKIPPING roots, heapRegionSets, heapRegions, 
>> remset) syms strs zone dict cldg metaspace chunks hand C-heap code 
>> cache ]
>> <---- universe2_init
>> ---->init::verify
>> [Verifying threads (SKIPPING roots, heapRegionSets, heapRegions, 
>> remset) syms strs zone dict cldg metaspace chunks hand C-heap code 
>> cache ]
>> <----init::verify
>> --->create_vm:verify
>> [Verifying threads (SKIPPING roots, heapRegionSets, heapRegions, 
>> remset) syms strs zone dict cldg metaspace chunks hand C-heap code 
>> cache ]
>> <---create_vm:verify
>
> With +UseTLABS:
>
>> ----> universe2_init
>> <---- universe2_init
>> ---->init::verify
>> <----init::verify
>> --->create_vm:verify
>> [Verifying threads (SKIPPING roots, heapRegionSets, heapRegions, 
>> remset) syms strs zone dict cldg metaspace chunks hand C-heap code 
>> cache ]
>> <---create_vm:verify
>
> Why do we perform  two additional verifications when TLABs are 
> disabled? I've removed these in this fix. If someone can provide a 
> reasonable justification, I'll add them back.
>
> Additionally I've moved the verification code in Threads::create_vm() 
> to after the VMThread is created. That way, as a future enhancement, 
> the verification could be wrapped inside a VMOperation.
>
> I've also included a regression test.
>
> Testing:
> The failing test case with G1 with and without TLABs enabled.
> The regression test with all the collectors.
> A jprt run (with -UseTLABS -XX:+VerifyBeforeGC) is in the queue.
>
> Thanks,
>
> JohnC
>
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20130322/5b7a64fe/attachment.htm>