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

[Jon Masamitsu]

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