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

[John Cuthbertson]

Hi Jon,

AFAIK this was the only place.

I don't recall any comment from Dave Detlefs in the original code 
explaining why. Most of the ranges (address ranges in regions etc) are 
exclusive. But this is the only place in G1 where we need the range of 
cards spanned by an object. In the original code - Dave was just using 
an inclusive loop - which set each bit in the inclusive range of cards 
individually. We changed the code to use the BitMap routines because 
they can work at the word level for large ranges, which should be more 
efficient. Hence this inclusive range was mapped to the exclusive range 
needed by the range utility routines in the BitMap class.

Do you know, offhand, how the other collectors find all of the cards 
spanned by a single object? Do they calculate an inclusive or exclusive 
range of cards? Do they use the last address word in the current object, 
or the start of the next object? In an earlier life, I can remember 
fixing a few OOB assertions (in the DirtyCardToOopClosures I believe) 
caused by attempting to get the card for the address returned by 
MemRegion::end() instead of MemRegion::last().

Having made the routine and call sites exclusive we could end up with a 
pointer that is actually outside the heap (if we're examining the last 
object in the heap) which will cause an assertion in 
CardTableModRefBS::is_card_aligned() to fire unless 
CardTableModRefBS::is_card_aligned() is guarded with a heap range check. 
With an inclusive range routine you can assume that the last address 
word of any object in the heap is also in the heap and so don't need to 
check if it's card aligned (no need to increment) and hence no need to 
check if it's in the heap.

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