[15] RFR 8244243: Shenandoah: Cleanup Shenandoah phase timing tracking and JFR event supporting

[Zhengyu Gu]

Hi Aleksey,

On 5/6/20 7:30 AM, Aleksey Shipilev wrote:
> On 5/1/20 7:08 PM, Zhengyu Gu wrote:
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8244243
>> Webrev: http://cr.openjdk.java.net/~zgu/JDK-8244243/webrev.00/index.html
> 
> It is a useful cleanup. I think it is better to get that cleanup complete:
> 
> *) It feels it should be moved to shenandoahPhaseTimings and be renamed to ShenandoahTimingsTracker,
> to match ShenandoahWorkerTimingsTracker. "Trackers" are the good name for something that only
> touches the timings.

Sure, updated: 
http://cr.openjdk.java.net/~zgu/JDK-8244243/webrev.01/index.html

> 
> *) Current ShenandoahGCPhaseTiming also carries current_phase for
> Shenandoah{Parallel,Concurrent}WorkerSession, shouldn't that be moved to ShenandoahGCPhase?
ShenandoahPausePhase/ShenandoahConcurrentPhase need to setup the phase 
for Shenandoah{Parallel,Concurrent}WorkerSession. Especially for 
concurrent phase, there might not have ShenandoahGCPhase in between, as 
JFR only supports 2 levels of concurrent event.


> 
> *) Do these really need to be subclasses? I think delegation is called for here.
> 
> class ShenandoahGCPhase : public ShenandoahGCPhaseTiming
> class ShenandoahPausePhase : public ShenandoahGCPhaseTiming
> class ShenandoahConcurrentPhase : public ShenandoahGCPhaseTiming

I don't have preference either way. But delegation model requires 
ShenandoahGCPhaseTiming (now ShenandoahTimingsTracker) to be declared as 
value object (based on hotspot convention), which is inconsistent with 
other classes ...

-Zhengyu

>