<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix"><br>
Hi John,<br>
<br>
On 5/3/13 7:51 PM, John Cuthbertson wrote:<br>
</div>
<blockquote cite="mid:5183F919.4020608@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
Hi Bengt,<br>
<br>
Thanks for looking at the backports. I really appreciate it.<br>
</blockquote>
<br>
No problem. Sorry for missing the request for 8005032 and 8009536
earlier. Reviewed it now.<br>
<br>
Thanks,<br>
Bengt<br>
<br>
<blockquote cite="mid:5183F919.4020608@oracle.com" type="cite"> <br>
<div class="moz-cite-prefix">On 5/2/2013 2:27 PM, Bengt Rutisson
wrote:<br>
</div>
<blockquote cite="mid:5182DA2A.5080000@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type">
<div class="moz-cite-prefix"><br>
Hi John,<br>
<br>
Thanks for providing both the old and new webrevs. Makes this
much easier to review!<br>
<br>
Comments inline...<br>
<br>
On 5/2/13 9:04 PM, John Cuthbertson wrote:<br>
</div>
<blockquote cite="mid:5182B8BF.40709@oracle.com" type="cite">
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
Hi Everyone,<br>
<br>
Some more backports that did not apply cleanly to review:<br>
<br>
<b>8009940:</b><b> </b><b>G1: assert(_finger == _heap_end)
failed, concurrentMark.cpp:809</b><br>
Backport: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejohnc/hsx24-backports/webrev.2.8009940.hsx24.patch/">http://cr.openjdk.java.net/~johnc/hsx24-backports/webrev.2.8009940.hsx24.patch/</a><br>
Original: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejohnc/8009940/webrev.1/">http://cr.openjdk.java.net/~johnc/8009940/webrev.1/</a><br>
<br>
The changes did not apply cleanly because of
CMTask::_worker_id being renamed to CMTask::_task_id.<br>
</blockquote>
<br>
Looks good.<br>
</blockquote>
<br>
Thanks.<br>
<br>
<blockquote cite="mid:5182DA2A.5080000@oracle.com" type="cite"> <br>
<blockquote cite="mid:5182B8BF.40709@oracle.com" type="cite"> <br>
Note these changes are generated w.r.t the backports for
8005032 and 8009536 being applied.<br>
<br>
<b>8010463: G1: Crashes with -UseTLAB and heap verification</b><br>
Backport: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejohnc/hsx24-backports/webrev.4.8010463.hsx24.patch/">http://cr.openjdk.java.net/~johnc/hsx24-backports/webrev.4.8010463.hsx24.patch/</a><br>
Original: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejohnc/8010463/webrev.0/">http://cr.openjdk.java.net/~johnc/8010463/webrev.0/</a><br>
<br>
I'm not sure why this did not apply cleanly but the rejected
chunk was in g1CollectedHeap.cpp.<br>
</blockquote>
<br>
In g1CollectedHeap.cpp it did not apply cleanly because of
changes for perm gen removal.<br>
<br>
I think the original (hs25) webrev may not be up-to-date.<br>
<br>
The tests in the webrevs are different:<br>
<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejohnc/hsx24-backports/webrev.4.8010463.hsx24.patch/test/gc/TestVerifyBeforeGCDuringStartup.java.html">http://cr.openjdk.java.net/~johnc/hsx24-backports/webrev.4.8010463.hsx24.patch/test/gc/TestVerifyBeforeGCDuringStartup.java.html</a><br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejohnc/8010463/webrev.0/test/gc/TestVerifyBeforeGCDuringStartup.java.html">http://cr.openjdk.java.net/~johnc/8010463/webrev.0/test/gc/TestVerifyBeforeGCDuringStartup.java.html</a><br>
<br>
But your backported test looks like the one that was actually
pushed to hs25:<br>
<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://hg.openjdk.java.net/hsx/hotspot-gc/hotspot/file/24ef5fb05e0f/test/gc/TestVerifyBeforeGCDuringStartup.java">http://hg.openjdk.java.net/hsx/hotspot-gc/hotspot/file/24ef5fb05e0f/test/gc/TestVerifyBeforeGCDuringStartup.java</a>
<br>
<br>
Similarly, what you actually pushed to hs25 in thread.cpp looks
more like what you propose to backport to hs24:<br>
<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://hg.openjdk.java.net/hsx/hotspot-gc/hotspot/diff/24ef5fb05e0f/src/share/vm/runtime/thread.cpp">http://hg.openjdk.java.net/hsx/hotspot-gc/hotspot/diff/24ef5fb05e0f/src/share/vm/runtime/thread.cpp</a><br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejohnc/hsx24-backports/webrev.4.8010463.hsx24.patch/src/share/vm/runtime/thread.cpp.udiff.html">http://cr.openjdk.java.net/~johnc/hsx24-backports/webrev.4.8010463.hsx24.patch/src/share/vm/runtime/thread.cpp.udiff.html</a><br>
<br>
So, I think this looks good.<br>
</blockquote>
<br>
Thanks. I must not have updated the original webrev after applying
your code review comments so supplying the link to the original
was probably not useful in this case.<br>
<br>
<br>
<blockquote cite="mid:5182DA2A.5080000@oracle.com" type="cite"> <br>
<blockquote cite="mid:5182B8BF.40709@oracle.com" type="cite"> <br>
<b>8010780: G1: Eden occupancy/capacity output wrong after a
full GC</b><br>
Backport: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejohnc/hsx24-backports/webrev.5.8010780.hsx24.patch/">http://cr.openjdk.java.net/~johnc/hsx24-backports/webrev.5.8010780.hsx24.patch/</a><br>
Original: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejohnc/8010780/webrev.0/">http://cr.openjdk.java.net/~johnc/8010780/webrev.0/</a><br>
<br>
The changes (especially in g1CollectedHeap.cpp) conflicted
with the tracing changes.<br>
</blockquote>
<br>
A bit difficult to review since the patch files are quite
different. As far as I can tell it looks good.<br>
</blockquote>
<br>
Thanks. The additional indentation is probably throwing the stats
off. There's more code in G1CollectedHeap::do_collection() for the
tracing and I had to indent that additional code, causing an
increase in the number of lines modified. I was more concerned
that I had preserved relative order w.r.t. the tracing code.<br>
<br>
Could I bother you to review the backports for 8005032 and
8009536? They underpin a couple of the changes above.<br>
<br>
Thanks again,<br>
<br>
JohnC<br>
<br>
</blockquote>
<br>
</body>
</html>