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

[John Cuthbertson]

Hi Bengt,

Thanks for looking over the changes. Replies inline....

On 3/21/2013 11:48 PM, Bengt Rutisson wrote:
>
> 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.

I think they we're deliberately added - probably a very long time ago. 
And Thomas' speculation that it's just to narrow the window in case of 
an error sounds plausible. But since parallel scavenge skipped the 
generations until the the first true GC - not sure how useful it would 
be for PS. Likewise for G1 - in the default case we skipped these extra 
verifications and in the first verification we skipped part of the heap.

>
> 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?

Yeah. Good catch. It should be with VerifyBeforeGC. Must have had 
marking on the brain.

As for the test. I think we can check the output for "VerifyBefore" for 
all the collectors. I'll change the test.

>
> 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).

I think this was a case of cut-n-paste from the GC code. I agree 
overriding the flag is strange - especially given that we have a flag 
for VerifyOnExit (or something like that). But it a pattern that we, in 
the GC team, recognize. :) I agree that checking against 
total_collections() is bogus. It will be 0 and so we'll skip the 
verification if VerifyGCStartAt is anything other than 0. I guess two 
choices:

1. Add new flag (or rename existing VerifyOnExit to VerifyOnInitAndExit), or
2. Use (VerifyBeforeGC && VerifyGCStartAt == 0)

Cheers,

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