RFR(XXS): 8017070: G1: assert(_card_counts[card_num] <= G1ConcRSHotCardLimit) failed

[John Cuthbertson]

Hi Bengt,

The motivation to bound was purely defensive. The actual count is a 
jubyte (to save space) even though the intermediate calculations are 
done as unsigned ints, and guarding against an overflow was a very 
simple expression.

The push failed because of an incorrect cast so I have an opportunity to 
add you to the reviewers.

Thanks,

JohnC

On 6/30/2013 10:48 PM, Bengt Rutisson wrote:
>
> Hi John,
>
> Change looks good to me too. Thanks for providing a good motivation.
>
> One question:
>
> Why do you cap the value to G1ConcRSHotCardLimit?
>
>  155         _card_counts[card_num] =
>  156           MIN2((uint)(_card_counts[card_num] + 1), 
> G1ConcRSHotCardLimit);
>
> It seems like the only place where we check these values is in 
> G1CardCounts::is_hot(), where we use ">=". So, it should be safe to 
> just keep incrementing the card count without capping it. I assume 
> that overflowing an uint is not an issue here, since if we have that 
> kind of heavy load on a card it will quickly reach any reasonable 
> value for G1ConcRSHotCardLimit after an overflow.
>
> bool G1CardCounts::is_hot(uint count) {
>   return (count >= G1ConcRSHotCardLimit);
> }
>
> Just to be clear, I'm fine with you pushing the current patch. I'm 
> just curious about the motivation for the capping.
>
> Thanks,
> Bengt
>
> On 6/28/13 9:01 PM, John Cuthbertson wrote:
>> Hi All,
>>
>> Can I couple of volunteers review this extremely small change? The 
>> webrev can be found at: 
>> http://cr.openjdk.java.net/~johnc/8017070/webrev.0/
>>
>> Summary:
>> The assert that fired is invalid. A card can be added to more than 
>> one update buffer since reads and writes to the card table are not 
>> atomic. If two threads end up refining the card at the same time then 
>> the count for that card can be incremented twice. If count is just 
>> below the hot threshold, the double increment will trip the assert. 
>> Since the card was enqueued by two different threads we do want the 
>> the double increment to more accurately reflect how hot the card is. 
>> I have removed the invalid assert and use a bounding expression to 
>> assign the new count value.
>>
>> Testing:
>> Weblogic+medrec on the failing SQE machine (though I couldn't 
>> reproduce the original problem)
>> Weblogic+medrec on an Intel Haswell machine
>> GC test suite
>> jprt.
>>
>> Thanks,
>>
>> JohnC
>>
>