CRR (XS): 7089625: G1: policy for how many old regions to add to the CSet (when young gen is fixed) is broken

[Tony Printezis]

Bengt and Ramki,

Thanks for the quick code reviews! I'll piggy-back on this change the 
removal of the erroneous comma from g1ErgoVerbose.hpp that's currently 
causing compilation warnings. Also see inline.

> Tony,
>
> Looks better this way ;-)
>
> To be strict, shouldn't line 3036 be "if (time_remaining_ms <= 0.0) {" 
> ? Probably does not matter in real life...

As you said FP equality is so inaccurate that I never use == for FP values.

> Do you think this could have been the cause of the new failure in the 
> nightlies that Stefan saw?

I don't think so. Maybe, this uncovered a separate issue but I doubt 
this is the actual cause.... I could of course be wrong. :-)

Tony

> Bengt
>
> On 2011-09-12 18:09, Tony Printezis wrote:
>> I had to re-arrange some code when I added the ergo decision output 
>> to G1 and I unfortunately got a condition wrong which broke the 
>> policy of how many old regions to add to the CSet.
>>
>> Could I please get a couple of quick code review for the two-line fix?
>>
>> http://cr.openjdk.java.net/~tonyp/7089625/webrev.0/
>>
>> I attached a description of the issue (from the CR) below.
>>
>> Tony
>>
>> --------------------------
>>
>> The original code was:
>>
>>      should_continue =
>>        ( hr != NULL) &&
>>        ( (adaptive_young_list_length()) ? time_remaining_ms > 0.0
>>          : _collection_set_size < _young_list_fixed_length );
>>
>> In the fix for 7050392 I refactored it to be able to emit the correct 
>> ergo policy output:
>>
>>      should_continue = true;
>>      if (hr == NULL) {
>>        // No need for an ergo verbose message here,
>>        // getNextMarkRegion() does this when it returns NULL.
>>        should_continue = false;
>>      } else {
>>        if (adaptive_young_list_length()) {
>>          if (time_remaining_ms < 0.0) {
>>            ergo_verbose1(ErgoCSetConstruction, ...);
>>            should_continue = false;
>>          }
>>        } else {
>>          if (_collection_set_size < _young_list_fixed_length) {
>>            ergo_verbose2(ErgoCSetConstruction, ...);
>>            should_continue = false;
>>          }
>>        }
>>      }
>>
>> and unfortunately I didn't negate the _collection_set_size < 
>> _young_list_fixed_length condition. The intention of this code is: if 
>> hr != NULL (which means: we've just found and added an old region to 
>> the CSet) and !adaptive_young_list_length() (which means: the young 
>> gen size is fixed), we should carry on adding old regions to the CSet 
>> until the CSet length (_collection_set_size) reaches the fixed young 
>> list target length (_young_list_fixed_length). So, should_continue 
>> should be set to false when _collection_set size >= 
>> _young_list_fixed_length, not when _collection_set_size < 
>> _young_list_fixed_length.
>>
>