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

[Erik Helin]

On 2014-04-28 13:09, 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.
>
>>
>> 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/

Looks good!

Thanks,
Erik

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