RFR (M): 8227739: Merge cost predictions for scanning cards and log buffer entries

[Thomas Schatzl]

Hi,

On 20.11.19 14:29, Stefan Johansson wrote:
> Hi Thomas,
> 
> Sorry for taking so long to get to this review.
> 
> On 2019-10-22 20:26, Thomas Schatzl wrote:
>> Hi all,
>>
>>    can I have reviews for this change that aligns the cost predictions 
>> to the way we do evacuations, i.e. that we first drop all remembered 
>> sets onto the card table, and only a fraction of that will be scanned 
>> as introduced by JDK-8213108.
>>
>> This code adds all the predictions for ratios etc to align to that 
>> code in our prediction model too.
>>
>> After this change (and all previous) changes just sent out for review, 
>> mostly JDK-8228609 (which is a prerequisite for this change), 
>> predictions are a bit (noticably) better than before :)
>>
>> CR:
>> https://bugs.openjdk.java.net/browse/JDK-8227739
>> Webrev:
>> http://cr.openjdk.java.net/~tschatzl/8227739/webrev/
> Looks good in general, just some small comments:
> src/hotspot/share/gc/g1/g1Analytics.cpp
> ---
> 255   if (for_young_gc || _mixed_cost_per_card_merge_ms_seq->num() < 3)
> 
> We have a few of these "seq->num() < 3" checks, what do you think about 
> adding a helper for those? Something like, ready_for_prediction(seq) and 
> do:
> for_young_gc || !ready_for_prediction(_mixed_cost_per_card_merge_ms_seq)
> ---
> 
> src/hotspot/share/gc/g1/g1Policy.cpp
> ---
>   725     if (total_cards_merged > 10) {
>   ...
>   738     if (total_cards_scanned > 10) {
> 
> Kind of pre-existing, but do you know why we have this limit of 10 in 
> these cases. Would be nice to add a comment about it and maybe add a 
> constant with some descriptive name.
> ---

Fixed. I do not know the history about the particular values, and only 
took them over from existing code. I guess these values are just guesses 
one way or another.

I will file an RFE to look into this. There is already one for the 1.1 
in predict_object_copy_time_ms_during_cm().

http://cr.openjdk.java.net/~tschatzl/8227739/webrev.0_to_1/ (diff)
http://cr.openjdk.java.net/~tschatzl/8227739/webrev.1/ (full)

Thanks,
   Thomas