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

[Jon Masamitsu]

On 04/28/2014 04:09 AM, Bengt Rutisson wrote:
>
> 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.

Sounds good.

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

Even better.

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

Changes look good.

Reviewed.

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