<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<br>
<br>
<div class="moz-cite-prefix">On 3/17/2016 6:45 PM, Derek White
wrote:<br>
</div>
<blockquote cite="mid:56EB3375.2020003@oracle.com" type="cite">
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
<div class="moz-cite-prefix">Possible pre-existing bug, see
below...<br>
<br>
On 3/17/16 6:34 PM, Derek White wrote:<br>
</div>
<blockquote cite="mid:56EB30DF.1000801@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<div class="moz-cite-prefix">On 3/10/16 3:10 PM, Joseph Provino
wrote:<br>
</div>
<blockquote cite="mid:56E1D4C1.5030208@oracle.com" type="cite">
<meta http-equiv="content-type" content="text/html;
charset=utf-8">
Please review these changes. <br>
<br>
CR: <a moz-do-not-send="true" class="issue-link"
data-issue-key="JDK-8085983"
href="https://bugs.openjdk.java.net/browse/JDK-8085983"
id="key-val" rel="4786093" style="color: rgb(59, 115, 175);
text-decoration: none;">JDK-8085983
G1CollectedHeap::collection_set_iterate_from() has unused
code and can be simplified</a><br>
<br>
Webrev: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejprovino/8085983/webrev.02">http://cr.openjdk.java.net/~jprovino/8085983/webrev.02</a><br>
<br>
Thanks.<br>
<br>
joe<br>
</blockquote>
Hi Joe,<br>
Here are my comments. The last issue looks serious, but I
suggested several possible solutions.<br>
<br>
heapRegionManager.hpp<br>
- Copyright<br>
</blockquote>
</blockquote>
Fixed.<br>
<blockquote cite="mid:56EB3375.2020003@oracle.com" type="cite">
<blockquote cite="mid:56EB30DF.1000801@oracle.com" type="cite"> <br>
- Line 267: too many b's<br>
- Does iterate really call doHeapRegionAbortabble()? (sp)<br>
</blockquote>
</blockquote>
Fixed. Good eyes. ;-)<br>
<blockquote cite="mid:56EB3375.2020003@oracle.com" type="cite">
<blockquote cite="mid:56EB30DF.1000801@oracle.com" type="cite"> <br>
heapRegionManager.inline.hpp<br>
- Odd spacing at line 102.<br>
</blockquote>
</blockquote>
Fixed.<br>
<blockquote cite="mid:56EB3375.2020003@oracle.com" type="cite">
<blockquote cite="mid:56EB30DF.1000801@oracle.com" type="cite"> <br>
- In HeapRegionManager::iterate(), line 118:<br>
- Should be calling HeapRegionManager::do_heap_region() here,
so iterate can abort?<br>
Wait! What happened to the call to incomplete()?<br>
</blockquote>
<br>
And why didn't the old (or new) par_iterate() call <tt>blk->incomplete();</tt>
when bailing out of iteration? I think the current callers of
par_iterate() don't check for complete() iterations, but that's
waiting to happen.
<blockquote cite="mid:56EB30DF.1000801@oracle.com" type="cite">
Wait! not all Closure objects have an incomplete() method, only<br>
AbortableHeapRegionClosures.<br>
<br>
OK, one fix would be to catch the abort condition in<br>
HeapRegionManager::do_heap_region(AbortableHeapRegionClosure,
*):<br>
<br>
<tt> bool do_heap_region(AbortableHeapRegionClosure* blk,
HeapRegion* r) const {</tt><tt><br>
</tt><tt> bool res = blk->doHeapRegion(r);</tt><tt><br>
</tt><tt> if (res) {</tt><tt><br>
</tt><tt> blk->incomplete();</tt><tt><br>
</tt><tt> }</tt><tt><br>
</tt><tt> return res;</tt><tt><br>
</tt><tt> }</tt><br>
<br>
But this means that the _complete field is only valid if<br>
AbortableHeapRegionClosure::doHeapRegion() is only called
from the<br>
new method above. Could enforce this by making doHeapRegion()<br>
private and making HeapRegionManager a friend.<br>
<br>
Alternatively, if we want doHeapRegion() callable from other<br>
places, we could have a public non-virtual doHeapRegion()
method<br>
that is a wrapper around a protected virtual
doHeapRegionWork()<br>
method. The wrapper calls doHeapRegionWork() and calls<br>
incomplete() if it return TRUE. But this means renaming all
of the<br>
overriden method implementations (or could do it in reverse
and<br>
rename the wrapper and all of it's callers).<br>
<br>
Alternatively (3rd option), we could make the<br>
AbortableHeapRegionClosure::doHeapRegion() overrides all
responsible for<br>
calling incomplete() if they return true.<br>
<br>
Anyone have an opinion here? Am I calling wolf on this issue?<br>
</blockquote>
</blockquote>
I'll look at this in more detail. Seems there's a problem and it's
going to get more complicated.<br>
<blockquote cite="mid:56EB3375.2020003@oracle.com" type="cite">
<blockquote cite="mid:56EB30DF.1000801@oracle.com" type="cite"> <br>
OK, what were we simplifying again in this RFE? :-)<br>
</blockquote>
</blockquote>
Good question!<br>
<br>
Thanks.<br>
<br>
joe<br>
<blockquote cite="mid:56EB3375.2020003@oracle.com" type="cite">
<blockquote cite="mid:56EB30DF.1000801@oracle.com" type="cite"> <br>
- Derek<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>