RFR(S): 7185699: G1: Prediction model discrepancies

[Thomas Schatzl]

Hi,

On Wed, 2012-07-25 at 11:39 -0700, John Cuthbertson wrote:
> Hi Thomas,
> 
> On 7/25/2012 2:14 AM, Thomas Schatzl wrote:
> >
> > Looking at the code (copied below for convenience), during that time the
> > number of cards prediction for young gen regions is based on the
> > prediction of mixed collection card lengths (as expected), but at the
> > same time, the "other" time prediction is based on the time prediction
> > for the young collections.
> >
> > This seems odd, because I'd expect both prediction components to be
> > based on the same prior decision at all times.
> [...]
>
> It looks like we're keeping track of how long it takes to add a region 
> to the collection set and how long it takes to free a region in the 
> collection set (based upon the region type). Haven't you shown that 
> freeing old regions during mixed GCs takes a lot longer than freeing 
> young regions? So choosing the other time for a region (vs. the base 
> other time for the pause) is a function of the region's type and not the 
> GC type. Though I'm not sure why we pass it as a parameter. I'll play 

My issue was more about that the current code predicts the number of
cards for young gen regions as a function of the gc type, and not as a
function of its type.

I believe a similar argument than for the "other" time applies to the
number of cards for young gen regions during mixed collections.
However, the difference between the two is that G1 only has a per-gc
type prediction for card numbers. This is unlike the "other" time where
it is calculated per region type, although the naming is very similar.

That's probably what was confusing me.

> around with refactoring the code to eliminate inlining the code into 
> calc_gc_efficiency().


Okay, then maybe something like this:

double
// the bool parameter selects the gc type we want the prediction for
G1CollectorPolicy::predict_region_elapsed_time_ms(HeapRegion* hr, bool
estimate_for_during_young_gc) {
  size_t rs_length = hr->rem_set()->occupied();
  size_t card_num;

  if (estimate_for_during_young_gc) {
    card_num = predict_young_card_num(rs_length);
  } else {
    card_num = predict_non_young_card_num(rs_length);
  }
  size_t bytes_to_copy = predict_bytes_to_copy(hr);

  double region_elapsed_time_ms =
    predict_rs_scan_time_ms(card_num) +
    predict_object_copy_time_ms(bytes_to_copy);

  // we use the region type here to discriminate between the predictors
  // for the other time because they are by region type.
  if (hr->is_young())
    region_elapsed_time_ms += predict_young_other_time_ms(1);
  else
    region_elapsed_time_ms += predict_non_young_other_time_ms(1);

  return region_elapsed_time_ms;
}

and replace the actual parameters in calls to
predict_region_elapsed_time_ms() to gcs_are_young(), but keep the
"false" value in HeapRegion::calc_gc_efficiency().

I attached a patch to demonstrate the change.

Thomas

-------------- next part --------------
A non-text attachment was scrubbed...
Name: predict_region_elapsed_time.tar.bz2
Type: application/x-bzip-compressed-tar
Size: 1444 bytes
Desc: not available
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20120726/46aa542f/predict_region_elapsed_time.tar.bz2>