RFR (M): 8074037: Refactor the G1GCPhaseTime logging to make it easier to add new phases

Thomas Schatzl thomas.schatzl at oracle.com
Mon Mar 2 14:05:13 UTC 2015


Hi,

On Fri, 2015-02-27 at 18:43 -0500, Kim Barrett wrote:
> On Feb 27, 2015, at 10:38 AM, Bengt Rutisson <bengt.rutisson at oracle.com> wrote:
> > 
> > Can I have a couple of reviews for this cleanup change?
> > 
> > http://cr.openjdk.java.net/~brutisso/8074037/webrev.00/
> > https://bugs.openjdk.java.net/browse/JDK-8074037
> > 
> > […]
> > The patch also introduces a new class called G1GCPhaseTimesTracker, which is
> >a scoped object to simplify time measurements. I've also tried to move
>>conversion of times down to when we print the log messages instead of
>>when we store them. This removes several "* 1000.0" in the code.
>>Eventually I would like to replace os::elapsedTime() with
>>os::elapsed_counter(). But that should be done as a separate patch.
> 
> Before I proceed any further with this review, I feel like I'm getting
> lost in unit changes. In some places values of msecs used to be used
> but now we're using secs, right? But it seems like there might be some
> confusion about that, and I'm not sure whether it's me or the code
> that's confused.

Given the confusion one option is to do the change above before that, or
in this change. At least the change of type from double to jlong in the
passed parameters should cause compiler warnings which are automatically
translated to errors.

Some more comments about the change:

- g1CollectedHeap.cpp:4659:

if (mark_in_progress() && !
_process_strong_tasks->is_task_claimed(G1H_PS_filter....) {
  G1GCPhaseTimesTracker x(timer, G1GCPhaseTimes::SATB_Filtering,
worker_i);
} else {
  set_satb_filtering_time_to_zero()
}

I think g1_process_roots() must always claim all tasks, even if the task
itself does nothing.

I suggest something like this instead:

{
  G1GCPhaseTimesTracker x(timer, G1GCPhaseTimes::SATB_Filtering,
worker_i);
  if (!_process_strong_tasks->is_task_claimed() &&
    mark_in_progress()) { // optionally keep the extra if-clause
  }
}

- the mentioned unit issues with ms and s in g1CollectorPolicy.cpp:1079,
1087, 1129, 1138, 1139, 1180, 1181, 2123-2128, 2130-2135

- in G1GCPhaseTimes::note_gc_start() there is some difference in how the
optional gc phases are enabled. I.e. for marking, the change passes a
new parameter, for the string deduplication the code uses some global
variables. Mark_in_progress could as well be determined by getting the
global g1collectorpolicy instance and querying it.
That's just a note, it seems somewhat ugly to me.

- g1GCphasetimes.cpp, line 269, instead of using the Sentinel directly
the change could use the ARRAY_SIZE macro on _gc_phases.

- The template methods WorkerDataArray<something>::uninitialized() are
declared NOT_PRODUCT, but there is no guard for them in the cpp file.

- ms/s mismatch in g1StringDedup.cpp:162

Thanks,
  Thomas





More information about the hotspot-gc-dev mailing list