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

[sangheon.kim]

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/20170913/e89bb5e5/attachment.htm>