RFR (S): JDK-8027428: Different conditions for printing taskqueue statistics for parallel gc, parNew and G1

[Bengt Rutisson]

On 2014-10-01 19:45, Sangheon Kim wrote:
> Hi ,
>
> On 10/01/2014 10:29 AM, Sangheon Kim wrote:
>> Hi,
>>
>> On 10/01/2014 03:51 AM, Jesper Wilhelmsson wrote:
>>> Thomas Schatzl skrev 1/10/14 10:12:
>>>> Hi,
>>>>
>>>> On Tue, 2014-09-30 at 22:58 +0200, Jesper Wilhelmsson wrote:
>>>>> Hi,
>>>>>
>>>>> Sangheon Kim skrev 30/9/14 19:41:
>>>>>> Hi Bengt,
>>>>>>
>>>>>> Thank you for looking at this change.
>>>>>>
>>>>>> On 09/30/2014 01:36 AM, Bengt Rutisson wrote:
>>>>>>>
>>>>>>> Hi Sangheon,
>>>>>>>
>>>>>>> On 2014-09-30 01:30, Sangheon Kim wrote:
>>>>>>>> Hi all,
>>>>>>>>
>>>>>>>> can I have reviews for the following small change to print 
>>>>>>>> taskqueue statistics
>>>>>>>> for parallel gc, parNew and G1?
>>>>>>>> Main changes are:
>>>>>>>> 1) printing by 'ParallelGCVerbose' flag, not 'PrintGCDetails &&
>>>>>>>> ParallelGCVerbose'.
>>>>>>>> 2) printing logs via 'gclog_or_tty', not 'tty'.
>>>>>>>> 3) adding 'totals' row to sums up.
>>>>>>>>
>>>>>>>> CR:
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8027428
>>>>>>>>
>>>>>>>> Webrev:
>>>>>>>> http://cr.openjdk.java.net/~jwilhelm/8027428/
>>>>>>>
>>>>
>>>> [...]
>>>>
>>>>>>> But there is a lot of logging that has to do with parallel work 
>>>>>>> (at least in
>>>>>>> G1) that is not controlled by this flag. It seems to me that the 
>>>>>>> flag actually
>>>>>>> only controls the task queue statistics. Ideally we could change 
>>>>>>> the name of
>>>>>>> the flag, but that will be too much overhead I guess. But we can 
>>>>>>> maybe updated
>>>>>>> the description to indicate more what it actually controls.
>>>>>> After applying this patch, I see this flag in 4 routines (3 files).
>>>>>> And as all of them are stats for 'taskqueue' and 'termination', I 
>>>>>> changed the
>>>>>> description as a statistics related flag.
>>>>>> But if it's not enough, may I ask your opinion?
>>>>>
>>>>> I don't like that this flag controls some, but not all logging for 
>>>>> parallel
>>>>> Stuff. I think we should do one of the following:
>>>>>
>>>>> A. Use ParallelGCVerbose for all verbose logging related to 
>>>>> parallel stuff in
>>>>> all GCs, or
>>>>>
>>>>> B. Only use ParallelGCVerbose in ParallelGC.
>>>>>
>>>>> There aren't that many occurrences of the flag so at least B 
>>>>> doesn't seem to be
>>>>> too much work. A is trickier since it involves finding all verbose 
>>>>> logging
>>>>> related to parallel stuff.
>>>>
>>>> The question is, what how do we intend to print the task queue
>>>> statistics then, consistently across all collectors? This seems to be
>>>> the only purpose of ParallelGCVerbose right now.
>>>> Sometimes (actually in very specific cases) this information is
>>>> interesting.
>>>>
>>>> There could be an option C) that just adds a new experimental or
>>>> diagnostic flag with a proper name that prints task queue statistics,
>>>> and let ParallelGCVerbose do nothing and start retiring the flag.
>>>>
>>>> If the task queue statistics are not compiled in, ParallelGCVerbose
>>>> prints nothing at all. So this would not be a change in behavior too.
>>>
>>> Thomas, you always have the best suggestions! :)
>>> If task queue is all it verboses about today, can't we simply rename 
>>> the flag to describe that? It would take a CCC, but I doubt that 
>>> would meet any objections.
>>> I vote for PrintTaskQueue to get it into the Print<Stuff> convention.
>>> /Jesper
>> I agree to Jesper's suggestion.
>> If retiring ParallelGCVerbose is preferred, it would be better just 
>> renaming it.
>> And a new one would be an experimental flag to print taskqueue stats.
> Sorry for sending again.
> I forgot that ParallelGCVerbose prints not only taskqueue stats but 
> also termination stats(G1ParScanThreadState::print_termination_stats).
> So the new flag name shouldn't be PrintTaskqueue.

Flags names are always difficult. :)

>
> How about leave the flag name as is?
> i.e. ParallelGCVerbose can be used widely and for now it will print 
> two stats information.

I still think we have lots of logging that concerns parallel work that 
we don't want to guard with ParallelGCVerbose.

How about doing a first change that separates out the termination stat 
printing to be guarded by a separate flag (Maybe PrintTerminationStats) 
and then rename ParallelGCVerbose to PrintTaskqueue?

If we do that I think both new flags, PrintTerminationStats and 
PrintTaskqueue, should be diagnostic rather than experiemental.

Thanks,
Bengt

>
> Thanks,
> Sangheon
>
>>
>> Thanks,
>> Sangheon
>>>
>>>
>>>>
>>>> Thanks,
>>>>    Thomas
>>>>
>>>>
>>
>