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

Zhengyu Gu zgu at redhat.com
Wed May 6 20:19:41 UTC 2020

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: 

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



More information about the hotspot-gc-dev mailing list