<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Hi Stefan,<br>
<br>
Thank you for reviewing this!<br>
<br>
Sangheon<br>
<br>
<br>
<div class="moz-cite-prefix">On 03/15/2016 06:52 AM, Stefan
Johansson wrote:<br>
</div>
<blockquote cite="mid:56E813A9.8080107@oracle.com" type="cite">
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
<tt>Hi Sangheon,</tt><br>
<br>
<div class="moz-cite-prefix">On 2016-03-14 20:12, sangheon wrote:<br>
</div>
<blockquote cite="mid:56E70D06.9040207@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
Hi Bengt,<br>
<br>
Thank you for reviewing this.<br>
<br>
<div class="moz-cite-prefix">On 03/14/2016 05:17 AM, Bengt
Rutisson wrote:<br>
</div>
<blockquote cite="mid:56E6ABEF.5060400@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<br>
Hi Sangheon,<br>
<br>
I like this cleanup. I think it makes things much more clear.<br>
<br>
I'm fine with the behavioral changes regarding the timing.
Thanks for listing them clearly in the table.<br>
</blockquote>
</blockquote>
I also like this, it's a big improvement. <br>
<br>
<blockquote cite="mid:56E70D06.9040207@oracle.com" type="cite">
<blockquote cite="mid:56E6ABEF.5060400@oracle.com" type="cite">
</blockquote>
Thanks!<br>
<br>
<blockquote cite="mid:56E6ABEF.5060400@oracle.com" type="cite">
<br>
One minor nit:<br>
<br>
g1CollectedHeap.cpp<br>
<br>
2800 return G1HeapSummary(heap_summary,
(Heap_lock->owned_by_self() ? used() : used_unlocked()),<br>
2801 eden_used_bytes,
eden_capacity_bytes, survivor_used_bytes, num_regions());<br>
<br>
I would prefer to store (Heap_lock->owned_by_self() ?
used() : used_unlocked()) in a local variable, just like
eden_used_bytes, eden_capacity_bytes and survivor_used_bytes.<br>
<br>
Other than that it looks good to me.<br>
</blockquote>
Here's updated webrev.<br>
<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Esangheki/8151085/webrev.01">http://cr.openjdk.java.net/~sangheki/8151085/webrev.01</a><br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Esangheki/8151085/webrev.01_to_00">http://cr.openjdk.java.net/~sangheki/8151085/webrev.01_to_00</a>
(inc)<br>
<br>
</blockquote>
Looks good.<br>
<br>
Thanks,<br>
Stefan<br>
<blockquote cite="mid:56E70D06.9040207@oracle.com" type="cite">
FYI: test result of RBT for hotspot_all and testlist is OK.<br>
<br>
Thanks,<br>
Sangheon<br>
<br>
<br>
<blockquote cite="mid:56E6ABEF.5060400@oracle.com" type="cite">
<br>
Thanks,<br>
Bengt<br>
<br>
<div class="moz-cite-prefix">On 2016-03-11 20:48, sangheon
wrote:<br>
</div>
<blockquote cite="mid:56E3210A.2050309@oracle.com" type="cite">
<meta http-equiv="content-type" content="text/html;
charset=utf-8">
Hi all,<br>
<br>
Could I have some reviews for this change?<br>
<br>
Concurrent gc timer and tracer are mainly used from
concurrent mark thread except when abort happens. This abort
results in a race condition between VM thread and concurrent
mark thread.<br>
So I propose to do timer and tracer related works only from
concurrent mark thread.<br>
<br>
Measurements are changed like below:<br>
<table border="1" width="100%" cellpadding="2"
cellspacing="2">
<tbody>
<tr>
<td valign="top"><br>
</td>
<td valign="top">Current<br>
</td>
<td valign="top">Proposal<br>
</td>
</tr>
<tr>
<td valign="top">Start timer and tracer<br>
</td>
<td valign="top">When Young Initial Mark GC starts
from VM thread<br>
</td>
<td valign="top">When actually starts concurrent cycle
from concurrent mark thread<br>
</td>
</tr>
<tr>
<td valign="top">End timer and tracer<br>
</td>
<td valign="top">1. At the end of concurrent cycle<br>
2. When abort happens</td>
<td valign="top">At the end of concurrent cycle<br>
<br>
* I think this will be okay as even though we abort,
we continue working for concurrent stuffs to finish
concurrent work cycle. And we were just not
measuring them.</td>
</tr>
<tr>
<td valign="top">Heap summary (before-gc)<br>
</td>
<td valign="top">At the beginning of Young Initial
Mark GC.<br>
But concurrent mark thread is not working at that
time.</td>
<td valign="top">At the start of concurrent cycle.</td>
</tr>
<tr>
<td valign="top">Heap summary (after-gc)<br>
</td>
<td valign="top">1. When abort happens before cleanup:
sent when abort happens.<br>
2. When abort happens after cleanup or normal
case(w/o abort): sent at the end of cleanup.</td>
<td valign="top">At the end of concurrent cycle which
means it will include all minor clearing after
'Pause Cleanup'.</td>
</tr>
</tbody>
</table>
<br>
This proposal also includes related changes like below:<br>
1. Removed the patch for JDK-8149834(race between VM thread
and concurrent mark thread for concurrent gc timer): 8149834
was making related functions atomically while this proposal
is changing not to happen that race.<br>
2. Removed G1CollectorState::_concurrent_cycle_started: no
longer needed as related logics are running from concurrent
mark thread.<br>
<br>
Currently 'total concurrent time' is not same as 'sum of
concurrent phases' and the reason is that the concurrent
timer is started when young initial mark gc happens.
Secondly, concurrent mark thread is not counting the time
for waiting VMThread to finish VM
operation(SuspendibleThreadSet). <br>
I am planning to enhance this after '8151614: Improve
logging in concurrent mark code' is pushed.<br>
<br>
CR: <a moz-do-not-send="true" class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8151085">https://bugs.openjdk.java.net/browse/JDK-8151085</a><br>
Webrev: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Esangheki/8151085/webrev.00">http://cr.openjdk.java.net/~sangheki/8151085/webrev.00</a><br>
Testing: JPRT, RBT(hotspot_all, testlist: running now),
local reproducer for JDK-8149834<br>
<br>
Thanks,<br>
Sangheon<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>