RFR(S): 7200261: G1: Liveness counting inconsistencies during marking verification

[John Cuthbertson]

Hi Jon,

Thanks for the review. And answering the questions was no problem. This 
is tricksy code and, since it's used to prune RSet entries, important to 
get right.

Thanks,

JohnC

On 09/26/12 11:04, Jon Masamitsu wrote:
> John,
>
> Thanks for the answers to my questions and for
> clarifying the comment.  Changes look good.
>
> Jon
>
> On 9/25/2012 3:44 PM, John Cuthbertson wrote:
>> Hi Everyone,
>>
>> A new webrev for these changes can be found at: 
>> http://cr.openjdk.java.net/~johnc/7200261/webrev.1/
>>
>> Based upon the code review comments - the set_card_bitmap_range() 
>> routine now takes an exclusive range of cards. As a result I have to 
>> check whether the end address of the current address 
>> range/region/object (i.e. the start the next) is card aligned (and in 
>> the heap) and increment the card index range appropriately. The 
>> essence of the bugfix is the same.
>>
>> Testing:
>> The failing tests; GC test suite with a low IHOP and verification; GC 
>> test suite with forced evacuation failures; nsk  (gc,  jit, 
>> regression, and runtime) tests with a low IHOP and verification; jprt.
>>
>> Thanks,
>>
>> JohnC
>>
>> On 09/25/12 10:54, John Cuthbertson wrote:
>>>
>>> Anyway the routine now takes an exclusive range - which, IMO, 
>>> doesn't look any cleaner than the previous code. Expect a new webrev 
>>> after some more testing.
>>>
>>> JohnC
>>>
>>> On 09/25/12 06:41, Jon Masamitsu wrote:
>>>> John,
>>>>
>>>> Is it common practice in G1 code to have the ranges
>>>> be inclusive?
>>>>
>>>> Jon
>>>>
>>>> PS.  Sorry for being tardy on my reply - so much mail,
>>>> so little time :-)
>>>>
>>>> On 09/24/12 10:03, John Cuthbertson wrote:
>>>>> Hi Jon,
>>>>>
>>>>> Thanks for the comments. I'm going to refer you to my reply to 
>>>>> Jesper. If people feel strongly about making the routine exclusive 
>>>>> - I'll make it so.
>>>>>
>>>>> JohnC
>>>>>
>>>>> On 09/21/12 22:48, Jon Masamitsu wrote:
>>>>>> I'm used to seeing a range like [start, end).  That the second 
>>>>>> index is named
>>>>>> last_idx helps but if I were just looking at the call site, I 
>>>>>> would have guessed
>>>>>> wrongly - i.e., thinking it was [start, end).
>>>>>>
>>>>>> Jon
>>>>>>
>>>>>> On 9/21/2012 9:16 PM, Jesper Wilhelmsson wrote:
>>>>>>> John,
>>>>>>>
>>>>>>> Looks good!
>>>>>>>
>>>>>>> Would it make sense to change set_card_bitmap_range to be 
>>>>>>> exclusive, or even to take a start and a size of the area? I 
>>>>>>> think inclusive functions like this one are unintuitive, 
>>>>>>> especially when the only use of last_idx is used with a +1. 
>>>>>>> Maybe that's a different change?
>>>>>>> /Jesper
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> 22 sep 2012 kl. 01:37 skrev John 
>>>>>>> Cuthbertson<john.cuthbertson at oracle.com>:
>>>>>>>
>>>>>>>> Hi Everyone,
>>>>>>>>
>>>>>>>> Can I have a couple of volunteers look over the fix for this 
>>>>>>>> CR? The webrev can be found at: 
>>>>>>>> http://cr.openjdk.java.net/~johnc/7200261/webrev.0/
>>>>>>>>
>>>>>>>> Summary:
>>>>>>>> The clipping in the code that sets the bits for a range of 
>>>>>>>> cards in the "expected" card bitmap that we check the liveness 
>>>>>>>> accounting data against was incorrect. This could lead to 
>>>>>>>> spurious verification failures. In addition to fixing the 
>>>>>>>> clipping, I've upleveled this routine and moved it into 
>>>>>>>> ConcurrentMark and now use it to generate the real liveness data.
>>>>>>>>
>>>>>>>> Testing:
>>>>>>>> The failing test cases with marking verification; jprt.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> JohnC
>>>>>
>>>
>>