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

[Erik Helin]

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?

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
>