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

[Jesper Wilhelmsson]

On 2012-09-24 19:00, John Cuthbertson wrote:
> Hi Jesper,
>
> Thanks for review. That is a good idea (and we have entertained doing it in
> the past) but is does have a slight downside.
>
> We need to set bits for all the cards spanned by a live object - including the
> last card if the object doesn't end at a card boundary. If we made
> set_card_bitmap_range() exclusive we end up moving the logic to determine
> which card index would be the second parameter into the callers (or a utility
> routine). The logic in the caller becomes something like:
>
> HeapWord* end = obj + obj->size();
> idx_t end_idx = card_bitmap_index_for(end);
> if (!card_aligned(end)) {
> // Next object doesn't start on a card boundary.We need to also include the
> card in which the current object ends.
> end_idx += 1;
> }
> set_card_bitmap_range(start_idx, end_idx);
>
> versus:
>
> HeapWord* last = obj + obj->size() - 1;
> idx_t last_idx = card_bitmap_index_for(last)
> set_card_bitmap_range(start_idx, last_idx)
>
> The mapping from the inclusive range to cards to the exclusive range expected
> by the bitmap routines is localized in set_card_bitmap_range().
>
> I don't think either is really better than the other.

set_card_bitmap_range seems to be called from three places. If I'm not 
mistaken only one of these calls have a end index based on an object that 
might stretch in to an extra card. On the contrary it seems as if the two 
other call sites have some extra logic imposed on them because of the set 
function being inclusive. Again, I may be wrong as I don't know the code very 
well.

If it is the case that only one call site needs the extra code in your example 
then I think set_card_bitmap_range should be changed to be exclusive. If the 
extra code is needed in all three places it may be better to keep it as it is.
/Jesper


>
> JohnC
>
> On 09/21/12 21:16, 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
>