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

Tue Apr 29 07:39:00 UTC 2014

Thanks for the reviews, Jon, Thomas and Erik!

Bengt

On 2014-04-28 18:07, Jon Masamitsu wrote:
>
> 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
>>>>>>
>>>>
>>>
>>
>