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

[Bengt Rutisson]

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

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

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