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

Sat Oct 4 21:20:05 UTC 2014

Hi all again,

Here's updated webrev which includes:
- Changed 'ParallelGCVerbose' to 'PrintTaskqueue' and 
'PrintTerminationStats'. (product type)
And these new flags are controlled by TASKQUEUE_STATS, same as 
'ParalleleGCVerbose'.

http://cr.openjdk.java.net/~jwilhelm/8027428/webrev.02/

Thanks,
Sangheon

On 2014. 10. 2. 오후 3:33, Sangheon Kim wrote:
> Hi all,
>
> Here's 2nd webrev and this fix includes:
> - Changed 'ParallelGCVerbose' to 'PrintTaskqueue' and 
> 'PrintTerminationStats'. (product type)
>
> webrev:
> http://cr.openjdk.java.net/~jwilhelm/8027428/webrev.01/
>
> Thanks,
> Sangheon
>
>
> On 10/02/2014 02:17 AM, Jesper Wilhelmsson wrote:
>>
>>> 2 okt 2014 kl. 09:05 skrev Bengt Rutisson <bengt.rutisson at oracle.com>:
>>>
>>>
>>>> On 2014-10-02 09:06, Jesper Wilhelmsson wrote:
>>>>
>>>> Bengt Rutisson skrev 2/10/14 08:54:
>>>>>
>>>>>> 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?
>>>>
>>>> Yes, I think this is good.
>>>>
>>>>>
>>>>> If we do that I think both new flags, PrintTerminationStats and 
>>>>> PrintTaskqueue, should be diagnostic rather than experiemental.
>>>>
>>>> I'm curious, what is the rationale to make the flag diagnostic 
>>>> rather than product? We have had requests in the past to make more 
>>>> verbose information available in product. Is there any reason in 
>>>> particular to make the flags something else than product?
>>>
>>> Diagnostic options are available in product builds.
>>>
>>> Making it diagnostic is just an indication that it is a flag if for 
>>> diagnosing the VM as opposed to tuning it. We don't use that in any 
>>> strict way so I don't have strong opinions about it. I just figured 
>>> that experimental does not seem to be the right category for these 
>>> flags.
>>
>> Agreed that experimental is not the right category.
>>
>> My take on the product vs diagnostic is that since diagnostic flags 
>> require the additional XX:+UnlockDiagnosticVMOptions they should be 
>> used for stuff that is not supposed to be enabled per default in 
>> production systems. That could be logging that affects performance 
>> too much or other diagnostic stuff that is only enabled when 
>> debugging or testing a production system. Logging that is expected to 
>> be enabled by default out there should be product imho.
>>
>> The risk when setting verbose flags that people are likely to use in 
>> production systems as diagnostic are that they start enabling 
>> UnlockDiagnosticVMOptions per default, and suddenly they are running 
>> with various really expensive logging and verification code enabled 
>> without realizing it affects performance.
>>
>> I don't know if anyone out there is likely to use either flags 
>> discussed here per default in a running production system, but unless 
>> they are bad for performance I would prefer to have them be product 
>> flags and leave it to the user to decide.
>>
>> /Jesper
>>
>>>
>>> Bengt
>>>
>>>> /Jesper
>>>>
>>>>>
>>>>> Thanks,
>>>>> Bengt
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Sangheon
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Sangheon
>>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Thomas
>>>
>