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

[Thomas Schatzl]

Hi,

On Fri, 2013-03-22 at 07:48 +0100, 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.

These verifications seem to make the intervals between verification
smaller to be able to pinpoint any problem location quicker.
Threads::create_vm() does a lot after all. Removal is fine with me as
well.

Comparing to the comments on the CR (great analysis btw!), you opted for
the approach to just skip verification of roots, heap regions and remset
until a safepoint. From your analysis, the other option would have been
to allow a NULL VM thread in some places. 

Allowing a NULL VMThread would allow checking heap consistency right
after Universe::genesis(). It is probably not expected that there are
already issues about the heap at this stage, but since we're already
verifying, I'd tend to prefer the option that verifies as much as
possible.
(Did not really check if it would really make a difference)

Maybe you could add a guarantee() or such in Threads::oops_do() and
Threads::possibly_parallel_oops_do() that only allowed a NULL VM Thread
during initialization?


> 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

You mean because of the name of the flag? That's a point. Otoh at VM
start you're technically before the next gc (whenever this happens ;)

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

Some values of some command line options change VerifyGCStartAt to
eventually disable verification at VM startup (beginning
arguments.cpp:2004).

>  seems unnecessary. We should either check VerifyGCStartAt == 0 or not

I would not mind either way; but you still need the VerifyBeforeGC (or
another flag) because otherwise the verification would practically be
unconditional as the default value of VerifyGCStartAt is zero.

>  include that flag at all (best choice in my opinion).

  Thomas