<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>
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>
<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 class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~johnc/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 class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~johnc/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 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 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 class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~johnc/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>
<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>
<br>
<blockquote cite="mid:5182B8BF.40709@oracle.com" type="cite"> <br>
The following webrevs applied cleanly to hsx24, so no review is
needed but I'm including them for completeness.<br>
</blockquote>
<br>
Did not look at these. Since they apply cleanly I'm sure their fine.<br>
<br>
All in all: Looks good! Ship it!<br>
<br>
Bengt<br>
<br>
<br>
<blockquote cite="mid:5182B8BF.40709@oracle.com" type="cite"> <br>
<b>8008301: </b><b>G1:
guarantee(satb_mq_set.completed_buffers_num() == 0) failure</b><br>
Backport:
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejohnc/hsx24-backports/webrev.3.8008301.hsx24.patch/">http://cr.openjdk.java.net/~johnc/hsx24-backports/webrev.3.8008301.hsx24.patch/</a><br>
Original: <a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejohnc/8008301/webrev.0/">http://cr.openjdk.java.net/~johnc/8008301/webrev.0/</a><br>
<br>
Note this webrev is was applied on top of 8005032, 8009536, and
8009940. <br>
<br>
<b>8012335: G1: Fix bug with compressed oops in template
interpreter on x86 and sparc.</b><br>
Backport:
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejohnc/hsx24-backports/webrev.6.8012335.hsx24.patch/">http://cr.openjdk.java.net/~johnc/hsx24-backports/webrev.6.8012335.hsx24.patch/</a><br>
Original: <a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Egoetz/webrevs/g1-cOops_bug/">http://cr.openjdk.java.net/~goetz/webrevs/g1-cOops_bug/</a><br>
<br>
<b>8012715: G1: GraphKit accesses PtrQueue::_index as int but is
size_t</b><br>
Backport:
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejohnc/hsx24-backports/webrev.7.8012715.hsx24.patch/">http://cr.openjdk.java.net/~johnc/hsx24-backports/webrev.7.8012715.hsx24.patch/</a><br>
Original: <a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Egoetz/webrevs/8012715/">http://cr.openjdk.java.net/~goetz/webrevs/8012715/</a><br>
<br>
Note: the last two were contributed by Martin Doerr from SAP.<br>
<br>
Thanks,<br>
<br>
JohnC<br>
<br>
<br>
</blockquote>
<br>
</body>
</html>