RFR: 8259404: Shenandoah: Fix time tracking in parallel_cleaning

[Aleksey Shipilev]

On Thu, 14 Jan 2021 01:39:02 GMT, Zhengyu Gu <zgu at openjdk.org> wrote:

> Please review this patch fixes timing tracking for parallel cleaning.
> 
> Before:
> `[9.844s][info][gc,stats] System Purge = 0.000 s (a = 76 us) (n = 1) (lvls, us = 76, 76, 76, 76, 76)` **<<== looks wrong**
> `[9.844s][info][gc,stats] Unload Classes = 0.001 s (a = 541 us) (n = 1) (lvls, us = 541, 541, 541, 541, 541)`
> `[9.844s][info][gc,stats] Weak Roots = 0.000 s (a = 75 us) (n = 1) (lvls, us = 75, 75, 75, 75, 75)`
> `[9.844s][info][gc,stats] CLDG = 0.000 s (a = 0 us) (n = 1) (lvls, us = 0, 0, 0, 0, 0)`
> After:
> `[9.936s][info][gc,stats] System Purge = 0.001 s (a = 611 us) (n = 1) (lvls, us = 609, 609, 609, 609, 611)`
> `[9.936s][info][gc,stats] Unload Classes = 0.000 s (a = 475 us) (n = 1) (lvls, us = 475, 475, 475, 475, 475)`
> `[9.936s][info][gc,stats] DCU: <total> = 0.000 s (a = 162 us) (n = 1) (lvls, us = 160, 160, 160, 160, 162)`
> `[9.936s][info][gc,stats] DCU: Code Cache Roots = 0.000 s (a = 162 us) (n = 1) (lvls, us = 160, 160, 160, 160, 162)`
> `[9.936s][info][gc,stats] Weak Roots = 0.000 s (a = 105 us) (n = 1) (lvls, us = 105, 105, 105, 105, 105)`
> `[9.936s][info][gc,stats] DWR: <total> = 0.000 s (a = 210 us) (n = 1) (lvls, us = 209, 209, 209, 209, 210)`
> `[9.936s][info][gc,stats] DWR: VM Weak Roots = 0.000 s (a = 210 us) (n = 1) (lvls, us = 209, 209, 209, 209)`

It is okay, but I have suggestions.

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 1795:

> 1793:   // Unload classes and purge SystemDictionary.
> 1794:   {
> 1795:     ShenandoahPhaseTimings::Phase p = full_gc ?

Please name the local variable `phase`. `p` is usually a "location" around GC code.

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 1854:

> 1852:   assert(SafepointSynchronize::is_at_safepoint(), "Must be at a safepoint");
> 1853:   assert(is_stw_gc_in_progress(), "Only for Degenerated and Full GC");
> 1854:   ShenandoahGCPhase root_phase(full_gc ?

Name it `phase`?

src/hotspot/share/gc/shenandoah/shenandoahParallelCleaning.cpp line 49:

> 47:   {
> 48:     ShenandoahWorkerTimingsTracker x(_phase, ShenandoahPhaseTimings::CodeCacheRoots, worker_id);
> 49:     _code_cache_task.work(worker_id);

This does not look like "normal" code cache root operation, though, right? Consider adding another type to `SHENANDOAH_PAR_PHASE_DO` instead? I.e. `CodeCacheUnload`?

src/hotspot/share/gc/shenandoah/shenandoahParallelCleaning.cpp line 56:

> 54:   if (_unloading_occurred) {
> 55:     ShenandoahWorkerTimingsTracker x(_phase, ShenandoahPhaseTimings::CLDGRoots, worker_id);
> 56:     _klass_cleaning_task.work();

Same thing, maybe new type of `SHENANDOAH_PAR_PHASE_DO`?

src/hotspot/share/gc/shenandoah/shenandoahParallelCleaning.inline.hpp line 60:

> 58:     _weak_processing_task.work<IsAlive, KeepAlive>(worker_id, _is_alive, _keep_alive);
> 59:   }
> 60:   _dedup_roots.oops_do(_is_alive, _keep_alive, worker_id);

This might need `ShenadoahWorkerTimingsTracker(... ShenandoahPhaseTimings::StringDedupTableRoots, ...)`?

-------------

Changes requested by shade (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2073