<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Hi Stefan,<br>
<br>
<div class="moz-cite-prefix">On 09/08/2017 02:56 AM, Stefan
Johansson wrote:<br>
</div>
<blockquote type="cite"
cite="mid:10fcdaae-4456-5257-6856-0cfb319a8795@oracle.com">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<tt>Thanks for reviewing Erik,</tt><br>
<br>
<div class="moz-cite-prefix">On 2017-09-05 15:49, Erik Helin
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:a2a45fc0-5c7b-0fae-8e47-b921e364d1d7@oracle.com">Hi
Stefan, <br>
<br>
thanks for cleaning this up! <br>
<br>
On 09/01/2017 02:57 PM, Stefan Johansson wrote: <br>
<blockquote type="cite">Hi, <br>
<br>
Please review this RFE to convert two more flags to use UL: <br>
<a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8184286"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8184286</a>
<br>
<br>
Webrev: <br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Esjohanss/8184286/hotspot.00/"
moz-do-not-send="true">http://cr.openjdk.java.net/~sjohanss/8184286/hotspot.00/</a>
<br>
</blockquote>
<tt><br>
</tt><tt>For GenCollectedHeap::pri</tt>nt_tracing_info(), I
would prefer it to be renamed to
GenCollectedHeap::log_tracing_info(), since the method now logs
instead of prints :) <br>
<br>
I would have written GenCollectedHeap::print_tracing_info()
like: <br>
<br>
LogTarget(Debug, gc, heap, exit) lt; <br>
<br>
<br>
if (lt.is_enabled()) { <br>
LogStream ls(lt); <br>
<br>
<br>
_young_gen->print_summary_info(&ls); <br>
<br>
<br>
_old_gen->print_summary_info(&ls); <br>
<br>
<br>
} <br>
<br>
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). <br>
<br>
But, if you prefer the current approach, that is fine by me as
well. <br>
</blockquote>
<tt>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
</tt><tt><span class="new">log_is_enabled(Debug, gc, heap, exit)</span></tt><tt>
because all other check use that.<br>
</tt><tt><span class="new"></span></tt><tt><br>
Updated webrevs:<br>
Inc: <a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Esjohanss/8184286/hotspot.00-01"
moz-do-not-send="true">http://cr.openjdk.java.net/~sjohanss/8184286/hotspot.00-01</a><br>
</tt><tt><span class="new"></span></tt><tt>Full: <a
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Esjohanss/8184286/hotspot.01"
moz-do-not-send="true">http://cr.openjdk.java.net/~sjohanss/8184286/hotspot.01</a><br>
</tt></blockquote>
01 version looks good as is.<br>
<br>
Just minor comments:<br>
1. Copyright update<br>
- parallelScavengeHeap.cpp<br>
- generation.cpp<br>
- generation.hpp<br>
<br>
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. :)<br>
1) psMarkSweep.cpp<br>
177 if (log_is_enabled(Debug, gc, heap, exit))
accumulated_time()->start();<br>
346 if (log_is_enabled(Debug, gc, heap, exit))
accumulated_time()->stop();<br>
<br>
2) psParallelCompact.cpp<br>
1782 if (log_is_enabled(Debug, gc, heap, exit))
accumulated_time()->start();<br>
<br>
3) psScavenge.cpp<br>
310 if (log_is_enabled(Debug, gc, heap, exit))
accumulated_time()->start();<br>
612 if (log_is_enabled(Debug, gc, heap, exit))
accumulated_time()->stop();<br>
<br>
If you are interested applying above, I don't need extra webrev for
this.<br>
<br>
Thanks,<br>
Sangheon<br>
<br>
<br>
<blockquote type="cite"
cite="mid:10fcdaae-4456-5257-6856-0cfb319a8795@oracle.com"><tt> <br>
Thanks,<br>
Stefan</tt><br>
<blockquote type="cite"
cite="mid:a2a45fc0-5c7b-0fae-8e47-b921e364d1d7@oracle.com"> </blockquote>
<blockquote type="cite"
cite="mid:a2a45fc0-5c7b-0fae-8e47-b921e364d1d7@oracle.com"> <br>
Thanks, <br>
Erik <br>
<br>
<blockquote type="cite">Summary: <br>
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. <br>
<br>
Testing: <br>
* Manually verified that same information as before is
available. <br>
* JPRT for sanity <br>
<br>
Thanks, <br>
Stefan <br>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>