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

[Bengt Rutisson]

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