RFR: 8231153: Improve concurrent refinement statistics

[Kim Barrett]

> On Sep 25, 2019, at 5:40 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
>> 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've made some updates to names, per some offline discussion with
Thomas.  The main one is to use a "total_" prefix in various places
for accumulated refinement time and accumulated number of cards
refined.  Also backed out any "num_" prefix removals from the original
change.  Removed the unsynchronized accumulation of the number of
concurrently scanned cards.

I also fixed a bug in the card logging rate calculation.  The function
I was using to get the end time for the last GC pause
(last_known_gc_end_time_sec) doesn't do any such thing, but has a
confusing name (JDK-8231638).


> When reading the change I had the following thoughts to improve readability:
> 
> - maybe some comment somewhere what "scanned" really means compared to "refined". Initially I was surprised with the change at G1RemSet::_num_conc_scanned_cards, but some thinking made me aware of the difference.

No longer relevant, as that's been removed.

> - the change reuses the "processed" term for counted cards in a few places, and it is unclear to me what the difference to just "refined" cards would be in some cases.

Fixed.

> - I would also suggest to add a "num_" prefix to numbers/counts of values.

Using "total_" prefix.

> - in G1Policy::_pending_cards should be renamed to "_pending_cards_at_start_of_gc" since we also now have a "_pending_cards_after_last_gc" to distinguish their use a little better?

Updated names:
pending_cards_at_gc_start
pending_cards_at_prev_gc_end


> - pre-existing: probably rename G1RemSet::_num_conc_scanned_cards and G1RemSetSummary::_conc_scanned_cards to "_concurrent_scanned_cards" to match the "_concurrent_refined_cards”.

Fixed by code deletion.

> - 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.

I've backed out the changes to G1FreeIdSet, and instead introduced in
G1DirtyCardQueue a private function providing the start index (always
returning 0) and used it in the two relevant places, along with noting
that there is code elsewhere that is assuming a 0 value.  That can be
cleaned up later (JDK-8231734).

New webrevs:
full: https://cr.openjdk.java.net/~kbarrett/8231153/open.01/
incr: https://cr.openjdk.java.net/~kbarrett/8231153/open.01.inc/

Testing:
mach5 tier1-5
some local by-hand testing to look at the rate tracking.