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

[Stefan Johansson]

Hi Thomas,

On 2019-11-20 16:26, Thomas Schatzl wrote:
> 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)
This looks good,
Stefan

> 
> Thanks,
>    Thomas