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

[Mikael Gerdin]

On 2015-03-02 15:05, Thomas Schatzl wrote:
> 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'm pretty sure that is incorrect. There is a comment before 
all_tasks_completed which hints at some verification that all tasks have 
been claimed but I have never found any code which performs this 
verification.

/Mikael

>
> 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
>
>