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

[John Cuthbertson]

Hi Thomas,

I like your suggested changes. I'll be sending out a new webrev shortly.

Thanks,

JohnC


On 07/26/12 01:04, Thomas Schatzl wrote:
> 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 --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20120726/394d992e/attachment.htm>