RFR (M): 8069330: Adjustment of concurrent refinement thresholds does not take hot card cache into account

[Jon Masamitsu]

Thomas,

http://cr.openjdk.java.net/~tschatzl/8069330/webrev/src/share/vm/gc/g1/g1CollectorPolicy.cpp.frames.html

Should you be protecting against the difference at 1054 being < 0?   
Perhaps generally that will
not happen but as times get very small, the calculations could go bad.

1052 double cost_scan_hcc = 
phase_times()->average_time_ms(G1GCPhaseTimes::ScanHCC);
1053     if (_pending_cards > 0) {
1054 cost_per_card_ms = 
(phase_times()->average_time_ms(G1GCPhaseTimes::UpdateRS) - 
cost_scan_hcc) / (double) _pending_cards;
1055       _cost_per_card_ms_seq->add(cost_per_card_ms);
1056     }
1057 _cost_scan_hcc_seq->add(cost_scan_hcc);

Also, not part of your change but if _pending_cards is 0, why isn't 0
added to _cost_per_card_ms_seq?

1155 double recent_scan_hcc_time_ms = 
phase_times()->average_time_ms(G1GCPhaseTimes::ScanHCC);

"recent" in the name recent_scan_hcc_time_ms made me want to go look for
scan_hcc_time_ms (i.e., I wondered if there was another scan hcc time).  
Change
the name to scan_hcc_time_ms?

Can you point me at where G1GCPhaseTimes::UpdateRS is recorded?  How the 
value in
average_time_ms(G1GCPhaseTimes::UpdateRS)is updated.  I want to 
understand how
the ScanHCC time is included in the UpdateRS time.

Jon




On 9/23/2015 12:38 PM, Thomas Schatzl wrote:
> Hi all,
>
>    can I have reviews for this change that fixes one part of the
> concurrent refinement threshold calculation?
>
> The problem is that the update RS time is used as a whole to determine
> the refinement thresholds to calculate a cost of scanning/updating
> cards. It does so by dividing the actual update RS time by the number of
> cards processed during that time.
>
> This is not true. In addition to processing cards, g1 also clears the
> hot card cache.
>
> That means that number of cards value is too small, making the cost per
> card too high, making the prediction to be too small.
>
> Further, if the hot card cache is sized larger than default, it may take
> up more time than the time available for update RS. That makes the
> prediction basically returning numbers that do not have anything to do
> with reality.
>
> This is not indicated by G1 at all.
>
> So this change adds timing information for the scan HCC phase (using
> PrintGCDetails), informs the user that something is wrong here if the
> HCC is too large or the available time too small, and uses that
> information to get the cost per card calculation right.
>
> While in itself this change does not help a lot since refinement
> threshold calculations are pretty bad (JDK-8137022) and there are bugs
> in refinement thread management (JDK-8133051) that make g1 waste a lot
> of cpu time, this change in conjunction with some prototypes for the
> others gives very good results.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8069330
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8069330/webrev/
> Testing:
> jprt
>
> Thanks,
>    Thomas
>
>
>