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

[Bengt Rutisson]

Hi John,

On 7/1/13 6:21 PM, John Cuthbertson wrote:
> 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.

OK. Thanks.

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

:) Sounds good.

Bengt

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