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

Bengt Rutisson bengt.rutisson at oracle.com
Mon Oct 6 11:16:14 UTC 2014


Hi Sangheon,

On 2014-10-04 23:20, sangheon.kim at oracle.com wrote:
> 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/

This looks good to me.

We need to release note this change since we are replacing one product 
flag with two other.

Thanks for fixing this!

Bengt

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



More information about the hotspot-gc-dev mailing list