RFR: 8184286: print_tracing_info() does not use Unified Logging for output

[Stefan Johansson]

Thanks for the rewview Sangheon,

I've updated the copy and added the braces as suggested and will push 
this once the repos open again.

Cheers,
Stefan

On 2017-09-13 21:41, sangheon.kim wrote:
> Hi Stefan,
>
> On 09/08/2017 02:56 AM, Stefan Johansson wrote:
>> Thanks for reviewing Erik,
>>
>> On 2017-09-05 15:49, Erik Helin wrote:
>>> Hi Stefan,
>>>
>>> thanks for cleaning this up!
>>>
>>> On 09/01/2017 02:57 PM, Stefan Johansson wrote:
>>>> Hi,
>>>>
>>>> Please review this RFE to convert two more flags to use UL:
>>>> https://bugs.openjdk.java.net/browse/JDK-8184286
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~sjohanss/8184286/hotspot.00/
>>>
>>> For GenCollectedHeap::print_tracing_info(), I would prefer it to be 
>>> renamed to GenCollectedHeap::log_tracing_info(), since the method 
>>> now logs instead of prints :)
>>>
>>> I would have written GenCollectedHeap::print_tracing_info() like:
>>>
>>>   LogTarget(Debug, gc, heap, exit) lt;
>>>
>>>
>>>   if (lt.is_enabled()) {
>>>     LogStream ls(lt);
>>>
>>>
>>>     _young_gen->print_summary_info(&ls);
>>>
>>>
>>>     _old_gen->print_summary_info(&ls);
>>>
>>>
>>>   }
>>>
>>> and then remove void Generation::print_summary_info(), only keep 
>>> Generation::print_summary_info_on(outputStream* st) (but update the 
>>> log message like you did).
>>>
>>> But, if you prefer the current approach, that is fine by me as well.
>> I made a mix to keep us both happy :) Using your approach to pass a 
>> LogStream to print_summary_info_on, but the if check uses 
>> log_is_enabled(Debug, gc, heap, exit)because all other check use that.
>>
>> Updated webrevs:
>> Inc: http://cr.openjdk.java.net/~sjohanss/8184286/hotspot.00-01
>> Full: http://cr.openjdk.java.net/~sjohanss/8184286/hotspot.01
> 01 version looks good as is.
>
> Just minor comments:
> 1. Copyright update
>     - parallelScavengeHeap.cpp
>     - generation.cpp
>     - generation.hpp
>
> 2. Adding parenthesis at if-statement. This is pre-existing issue but 
> it would be better fixing it now as those parts are mostly don't have 
> parenthesis at if-statement. Interestingly psParallelCompact.cpp line 
> 1901 has it. :)
>     1) psMarkSweep.cpp
>         177     if (log_is_enabled(Debug, gc, heap, exit)) 
> accumulated_time()->start();
>         346     if (log_is_enabled(Debug, gc, heap, exit)) 
> accumulated_time()->stop();
>
>     2) psParallelCompact.cpp
>         1782     if (log_is_enabled(Debug, gc, heap, exit)) 
> accumulated_time()->start();
>
>     3) psScavenge.cpp
>        310     if (log_is_enabled(Debug, gc, heap, exit)) 
> accumulated_time()->start();
>        612     if (log_is_enabled(Debug, gc, heap, exit)) 
> accumulated_time()->stop();
>
> If you are interested applying above, I don't need extra webrev for this.
>
> Thanks,
> Sangheon
>
>
>>
>> Thanks,
>> Stefan
>>>
>>> Thanks,
>>> Erik
>>>
>>>> Summary:
>>>> Converting two logging/tracing flags that had not been yet been 
>>>> changed to use unified logging. TraceYoungGenTime and 
>>>> TraceOldGenTime prints information at VM exit and the same 
>>>> information will now be available under tag-set "gc, heap, exit" on 
>>>> debug level.
>>>>
>>>> Testing:
>>>> * Manually verified that same information as before is available.
>>>> * JPRT for sanity
>>>>
>>>> Thanks,
>>>> Stefan
>>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20170921/e4b896cb/attachment.htm>