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

[Thomas Schatzl]

Hi Jon,

  new webrevs at
http://cr.openjdk.java.net/~tschatzl/8069330/webrev.1/ (full)
http://cr.openjdk.java.net/~tschatzl/8069330/webrev.0_to_1/ (diff)

Changes got more extensive than I thought because then I thought about
cleaning up things like removing unused parameters and such...

Some comments inline:

On Wed, 2015-09-30 at 14:12 -0700, Jon Masamitsu wrote:
> 
> On 9/30/2015 12:42 PM, Thomas Schatzl wrote:
> > Hi Jon,
[...]
> > I will think of some protection about really small times though.
> 
> Thanks.  If you're confident that there is no real problem, don't think to
> hard about it.  I thought that the times were measured separately so
> inaccuracies might cause a problem.  But again, don't spend too much time
> on it.

I do not think there is a problem. The code structure makes it obvious
that there cannot be an issue, unless time increments backwards.

> >> 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?
> > "No work to do" does not add any information about how much time the
> > processing of a card took, so we do not add it.
> 
> OK.  I thought that the add() added a value to the sequence that was
> used to calculate the average and so a 0 would matter.  If not so,
> then nothing needs to be done.

It does. But in this particular case we do not care. We only want to see
about the time scanning a card takes, i.e. the average cost per card.
However, when you did not do any work, you can not give a current cost
per card. So it is skipped.

> > The reason why we add times to _cost_scan_hcc_seq anyway is that
> > _pending_cards only counts cards (actually it only gives a very
> > conservative estimate. That might be problematic for small numbers where
> > that estimate has not a lot to do with the actual values) in the DCQ
> > buffers. The HCC is extra.
> >
> > What do you think about moving the HCC time out of
> > G1CollectedHeap::iterate_dirty_card_closure(), to G1RemSet::updateRS to
> > make the difference more clear?
> 
> You mean moving
> 
> 2454   // Clean cards in the hot card cache
> 2455 double hcc_start = os::elapsedTime();
> 2456   G1HotCardCache* hot_card_cache = _cg1r->hot_card_cache();
> 2457   hot_card_cache->drain(worker_i, g1_rem_set(), into_cset_dcq);
> 2458 
> g1_policy()->phase_times()->record_time_secs(G1GCPhaseTimes::ScanHCC, 
> worker_i, os::elapsedT
> 
> 
> up into updateRS?  Yes, I like that better.

Done.

[...]
> >> 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?
> > Okay. I will fix that.
> >

Done.

> >> 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.
> > In G1RemSet::updateRS(). This is also the only place that calls this
> > method.
> 
> The code
> 
>    G1GCParPhaseTimesTracker x(_g1p->phase_times(), 
> G1GCPhaseTimes::UpdateRS, worker_i);
> 
> does the measurement?  Thanks.

Yes.

Thanks,
  Thomas