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

[Jon Masamitsu]

There are two new flags introduced.

>   334   develop(bool, G1VerifyBitmaps, false,                                     \
>   335           "Verifies the consistency of the marking bitmaps")                \
>   336                                                                             \
>   337   develop(bool, G1VerifyPrevBitmap, false,                                  \
>   338           "Verifies the consistency of the prev marking bitmap")

When would we want to verify the next bitmap but not the prev bitmap?

I see that G1VerifyPrevBitmap guards

> 1515       // Clear the previous marking bitmap, if needed for bitmap verification.
> 1516       // Note we cannot do this when we clear the next marking bitmap in
> 1517       // ConcurrentMark::abort() above since VerifyDuringGC verifies the
> 1518       // objects marked during a full GC against the previous bitmap.
> 1519       // But we need to clear it before calling check_bitmaps below since
> 1520       // the full GC has compacted objects and updated TAMS but not updated
> 1521       // the prev bitmap.
> 1522       if (G1VerifyPrevBitmap) {
> 1523         ((CMBitMap*) concurrent_mark()->prevMarkBitMap())->clearAll();
> 1524       }

Is it worth the extra flag for that?

How about changing verify_bitmaps() to verify_both_bitmaps()? To make it
easier on the eyes to distinguish from verify_bitmap().

Rest looks good.

Jon


On 4/25/2014 7:30 AM, Bengt Rutisson wrote:
>
> Hi Erik,
>
> Thanks for looking at this!
>
> On 2014-04-25 15:55, Erik Helin wrote:
>> Hi Bengt,
>>
>> On 2014-04-23 10:20, Bengt Rutisson wrote:
>>>
>>> Hi all,
>>>
>>> Picking up an over 1 year old task...
>>>
>>> Tony created this patch at some point and passed it on to John
>>> Cuthbertson. John sent out a review request and there were some 
>>> comments
>>> (http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2013-May/thread.html#7202 
>>>
>>> and
>>> http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2013-May/007279.html), 
>>>
>>> but the patch never got finalized or pushed. I thought I'd try to get
>>> this pushed now.
>>>
>>> Just like John suggested I would like to do the push as contributed-by
>>> Tony unless there are major changes due to comments.
>>>
>>> Here is a new webrev:
>>> http://cr.openjdk.java.net/~brutisso/7132678/webrev.1/
>>
>> I had a look at the patch and noticed:
>>
>>   bool G1CollectedHeap::verify_bitmaps:
>>     ...
>>     if (mark_in_progress() || !_cmThread->in_progress()) {
>>       res_n = verify_bitmap("next", next_bitmap, ntams, end);
>>     }
>>     ...
>>
>> what does the condition mark_in_process() || 
>> !_cmThread->in_progress() mean here?
>>
>> What do you think about adding a comment (or extracting this check 
>> into a method) to make it more obvious why we are doing this check?
>
> Good point!
>
> Updated webrev:
> http://cr.openjdk.java.net/~brutisso/7132678/webrev.2/
>
> And the diff with only the comment added:
> http://cr.openjdk.java.net/~brutisso/7132678/webrev.1-2.diff/
>
> Thanks,
> Bengt
>
>>
>> Thanks,
>> Erik
>>
>>> Here is the diff compared to the one John sent out 15 months ago:
>>> http://cr.openjdk.java.net/~brutisso/7132678/webrev.0-1.diff/
>>>
>>> In the original patch there is a bug in the full GC case. We verify the
>>> "prev" bitmap after doing a full GC. But the full GC has compacted
>>> objects and reset tams without updating the prev bitmap. I think 
>>> this is
>>> normally safe since the prev bitmap will be cleared once it becomes the
>>> next bitmap later on. However, the new verification code looks at stale
>>> data in the prev bitmap. There was already code added to clear the prev
>>> bitmap if verification was enabled. I moved that code to just before 
>>> the
>>> verification. It might seem unnecessary to verify a bitmap that we just
>>> cleared, but that seemed easier than to propagate the state down to the
>>> verification code that it should not verify the prev bitmap for this
>>> particular call.
>>>
>>> There is one more change that I made. I removed one guarantee that I
>>> think was unnecessary.
>>>
>>> Thanks,
>>> Bengt
>>>
>>> On 2013-01-15 21:10, 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
>>>
>