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

[Bengt Rutisson]

Hi John,

Some comments inline...

On 3/22/13 7:19 PM, John Cuthbertson wrote:
> 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.

Sound reasonable. Thanks for the extra explanation.

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

Great!

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

Right. I would prefer 1. but I'm fine with 2. Maybe 2. is more 
reasonable change to do for this fix. Perhaps 1. should be its own change.

Either way is fine with me.

Thanks,
Bengt


>
> Cheers,
>
> JohnC

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