RFR: 8242078: G1: Improve concurrent refinement analytics and logging

[Thomas Schatzl]

Hi,

On 04.04.20 23:21, Kim Barrett wrote:
> Please review this change to improve the data collection and logging for
> concurrent refinement.
> 
> Both concurrent refinement threads and refining mutator threads now use a
> common structure for collecting stats about refinement.  This simplifies
> and improves the reporting and analytics based on that information.
> 
> Changed logging to be in the pause-start recording stage, under the control
> of the (gc, refine, stats) tagset.  Concurrent refinement statistics are no
> longer part of the RemsetSummary.
> 
> In the gathering and reporting, made a nomenclature change from "logged"
> cards to "dirtied" cards.  This was originally done in support of some
> experiments with deferred dirtying of logged cards, but I think it makes
> sense even in the absence of such deferral.
> 
> As part of this change, moved the call to G1DirtyCardQueue::concatenate_logs
> earlier, so that other code (including the concurrent refinement stats
> recording code) doesn't need to deal with the pre-flushed state.  Also
> report time spent specifically in this phase.
> 
> Because this change uses phase time's cur_collection_start_sec() (when
> computing the rate of dirty card generation) it "suffers" from the same
> questions around GC pause start time as discussed in JDK-8240779.
> 
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8242078
> > Webrev:
> https://cr.openjdk.java.net/~kbarrett/8242078/open.00/

Some minor issues:

- G1CollectedHeap::gc_prologue - in new code doing timing measurements, 
please use Ticks/Tickspan instead of double/os::elapsedTime.

- g1RemSetSummary.hpp:36: the "private" visibility specifier could be 
removed as well.

- the modification to NonJavaThreadsList_lock maybe warrants a different 
change. It just does not seem to fit although it looks correct.

- pre-existing: G1Analytics::predict_concurrent_refine-rate_ms does not 
seem to be used. Also predict_logged_cards_rate_ms. Could that (and all 
associated code) be removed so that the latter does not need to be renamed?

Seems good otherwise.

Thanks,
   Thomas