RFR(S/M): 7132678: G1: verify that the marking bitmaps have no marks for objects over TAMS

[Bengt Rutisson]

Hi John,

A couple of general questions:

What is the performance impact of these extra verifications? I realize 
that the performance impact is almost zero when G1VerifyBitmaps and 
G1VerifyPrevBitmaps are disabled, but when they are enabled there will 
be many places where we do the extra checks.

Stefan recently tried to run G1 with verification enabled and it turned 
out to be almost impossible. GC times were somewhere around 50 seconds 
if I remember correctly. Having this many extra verification may add 
even more to that. Would it be possible to have a "light" mode where we 
for example only do the verification after a marking cycle has finished?

Also, it seems inconsistent to use guarantees in the checks when the 
flags are develop flags. I would prefer to keep the guarantees and 
change to product (or diagnostic) flags.

Some implementation comments:

ConcurrentMark::abort()

I think we have to clear the previous bitmap here too if 
G1VerifyPrevBitmap is false, right? Since in 
G1CollectedHeap::do_collection() we do:

     if (G1VerifyPrevBitmap) {
       ((CMBitMap*) concurrent_mark()->prevMarkBitMap())->clearAll();
     }

Also, why do we need to clear part of the previous bitmap in 
G1CollectedHeap::free_region()?


In G1CollectedHeap::verify_bitmap() there are these two lines:

   HeapWord* result = bitmap->getNextMarkedWordAddress(tams, end);
   guarantee(result <= end,
             err_msg("result: "PTR_FORMAT" end: "PTR_FORMAT, result, end));

This guarantee pretty much just checks that getNextMarkedWordAddress() 
works as expected. I think that is a bit odd to check here. Personally I 
find it more confusing than helping to have this check here. I would 
prefer to remove this guarantee.



g1CollectedHeap.cpp

I don't understand the division between the verify* and check* methods. 
Why do the verify* methods return true/false when all the check methods 
do is guarantee that they get true returned? Would be simpler to have 
the verify* methods be void and just inline the guarantee inside of them.

Having the guarantees inside the verify methods would also make sure 
that the message ends up in the hs_err file, which is more likely to 
propagate to us than something we print on stdout or a log file.

Personally I also find the "####" prefix kind of ugly. Since we exit the 
VM just after we print this there is really no need to make this print 
statement stand out. It will be very easy to spot anyway. Especially if 
we change to make the messages error messages for the guarantees.

Thanks,
Bengt


On 1/15/13 9:10 PM, John Cuthbertson wrote:
> Hi Everyone,
>
> Can I have a couple of volunteers review the changes for this CR? The 
> webrev can be found at: 
> http://cr.openjdk.java.net/~johnc/7132678/webrev.0/
>
> Most of the changes come from a patch that Tony gave me before he left 
> and I had to tweak them slightly to remove a spurious failure. The 
> changes verify that the heap regions don't have any marks between 
> [TAMS, top) at strategic places: start and end of each GC, start and 
> end of remark and cleanup, and when allocating a region. Tony deserves 
> the bulk of the credit so, if possible and there are no objections, I 
> intend to list him as author of the change and include myself as a 
> reviewer.
>
> Testing:
> GC test suite with the both the new flags (separately and together) 
> and a low IHOP value.
> jprt with the new flags (+IgnoreUnrecognizedVMOptions so that product 
> test runs did not fail).
>
> Thanks,
>
> JohnC