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

[Jon Masamitsu]

Thomas,

Latest changes look good.  Thanks.  Nice cleanup.

Reviewed.

Jon

On 10/2/2015 4:01 AM, Thomas Schatzl wrote:
> 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
>
>