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

[Bengt Rutisson]

Hi Jon,

Thanks for looking at this!


On 2014-04-25 19:28, Jon Masamitsu wrote:
> 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?

I think that is the reason, but I doubt that it is worth it. I do agree 
that it is questionable if we need two flags and if we do we should name 
them differently and make sure that they behave consistently.

Rather than making things more complicated I removed G1VerifyPrevBitmap 
and only use G1VerifyBitmaps now.

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

I renamed verify_bitmap() to verify_no_bits_over_tams() since it 
actually does not verify the whole bitmap, just that there are not bits 
set over TAMS.

New webrev:
http://cr.openjdk.java.net/~brutisso/7132678/webrev.3/

Diff compared to last one:
http://cr.openjdk.java.net/~brutisso/7132678/webrev.2-3.diff/

Thanks,
Bengt


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