RFR: 8231153: Improve concurrent refinement statistics

[Thomas Schatzl]

Hi Kim,

On 26.09.19 03:50, Kim Barrett wrote:
>> On Sep 25, 2019, at 5:40 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
>>
>> Hi Kim,
>>
>> On 24.09.19 04:58, Kim Barrett wrote:
>>> […]CR:
>>> https://bugs.openjdk.java.net/browse/JDK-8231153
>>> Webrev:
>>> https://cr.openjdk.java.net/~kbarrett/8231153/open.00/
>>> Testing:
>>> mach5 tier1-5
>>> some local by-hand testing to look at the rate tracking.
>>
>>   I think the change in functionality is good. I can't really judge the appropriateness of the input values to the predictors since I do not know their actual use and haven't gotten a feeling about whether there is a problem with that :)
> 
> Thanks for looking at it.
> 
> The current algorithm for controlling refinement thread activation has
> problems because its inputs have little to do with what it should be
> trying to achieve.  These new predictors provide information that I
> currently think will be useful to making improvements there, but since
> I haven’t fully worked out the details there might be further adjustments
> coming along later.
> 
[...]>
> I considered changing RemSet::refine_card_concurrently() to return a
> bool value indicating whether the card was scanned or not. But I'm not
> sure that's really worthwhile, since most of the the reasons for not
> scanning are (I think) relatively rare.  (Stale cards and failure to
> be filtered earlier for timing reasons.)  The exception is the HCC.
> However, doing that would really just introduce another rate, e.g. the
> expected fraction of enqueued cards that need scanning.
> 
> I have seen large variability in that fraction. It's been a while
> since I was looking at that, but I think it had to do with the
> difference between young-only and mixed GCs. It might be that separate
> statistics need to be maintained for those cases. But I also think the
> recent changes to the redirtying process may ameliorate that
> difference. I'll be looking into that as part of future changes to
> make use of this new data.
> 

Looking forward to these changes :)

>> - I would also suggest to add a "num_" prefix to numbers/counts of values.
> 
> I guess our tastes differ, because I strongly disliked those num_ prefixes.
> They smack of Hungarian Notation and the like (of which I'm not a fan).

:) I like the little better distinction particularly in code where there 
is reason for confusion: e.g. if need to handle both counts and actual 
data and they are similarly named (like "card" and "cards"), which makes 
it very hard to parse longer blocks of code where both are used (even 
worse in the same statement).

In this case, with a "size_t* processed_cards" argument (something like 
that) I can't tell easily by looking at the signature whether this is a 
single count value that is passed in via a pointer as inout-parameter, 
or this is an array of card counts or actually cards. There is still the 
possibility that this is an array of counts then, but from the context 
that does not make sense. Ymmv :)

>> - pre-existing: probably rename G1RemSet::_num_conc_scanned_cards and G1RemSetSummary::_conc_scanned_cards to "_concurrent_scanned_cards" to match the "_concurrent_refined_cards”.
> 
> RemSet::_num_conc_scanned_cards is only used for logging, and has a
> poor implementation (racy increments). Given its usage and being in a
> somewhat performance critical code path, I'm reluctant to fix the
> implementation. I considered just removing it completely, since the
> value isn't critical and could be misleading. What do you think?

I am fine with that; please make this change also fix 
https://bugs.openjdk.java.net/browse/JDK-8043505 then :)

>> - not sure, but I think exposing size() and start() and in G1FreeIdSet seems unnecessary: the only user is G1DirtyCardQueueSet anyway, and it is already owner of G1FreeIdSet. I.e. it knows these values already (and passes it to the initializer of the G1FreeIdSet instance, and already has a getter for the size() value), so getting it back from G1FreeIdSet seems a bit strange to me, but I am okay with current code.
> 
> The start() function is used in DCQS::mut_process_buffer().  However,
> the start offset now appears to be a premature generalization.  In the
> RFR for JDK-8216258 I said

This is still in the DCQS class.

> 
>    Also generalized to allow the id set to start with a non-zero id,
>    because that looks like it might simplify other changes I'm working on.
> 
> Nothing seems to have happened with that, and I no longer even recall what
> changes I was thinking about there.  Maybe I should back out the new start()
> and size() and eliminate the support for a start offset entirely.
> 
> I could do that as part of this change, or save it for another RFE.
> 

As you prefer. It does not seem an extremely big change, so either is 
fine with me.

Thanks,
   Thomas