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

[Bengt Rutisson]

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
>