<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix"><br>
Hi Kim,<br>
<br>
On 11/04/15 07:51, Kim Barrett wrote:<br>
</div>
<blockquote
cite="mid:E56543E1-E37F-4641-9B94-2A8939944A99@oracle.com"
type="cite">
<pre wrap="">Another round.
I've updated my local repo from the team repo, merging with the
changes from 8076265: Simplify deal_with_reference.
I've backed out the G1EagerReclaimHumongousPreSnapshotTypeArrays
experimental configuration option, per Bengt's suggestion that it will
likely be more trouble than it's worth.
Because of the latter, the test to determine whether a humongous
region should be a candidate for eager reclamation has been
simplified, since at present there is no need to check the allocation
time of the object. But I've left the commentary there about
different handling of objects containing references, and noting the
special handling of typeArray objects in this respect.
Also had to move some new inline function definitions from
g1CollectedHeap.hpp to g1CollectedHeap.inline.hpp, to account for
header cleanups since my repo was last updated.
CR:
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8069367">https://bugs.openjdk.java.net/browse/JDK-8069367</a>
Webrev:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~kbarrett/8069367/webrev.04/">http://cr.openjdk.java.net/~kbarrett/8069367/webrev.04/</a></pre>
</blockquote>
<br>
The latest webrev looks ok to me. I'm fine with pushing it as is.<br>
<br>
However, I think it would have been easier to review this if it had
been split up to two changes. First a preparation fix that does the
renaming of _humongous_is_live to <br>
_humongous_reclaim_candidates and the removal of
clear_humongous_is_live_table() and the other refactoring. Then one
that actually includes the fix, with
<meta http-equiv="content-type" content="text/html;
charset=windows-1252">
process_grey_object() and the changes to
CMTask::deal_with_reference().<br>
<br>
These two things are separable, right? Or am I missing something? As
I said I'm fine with pushing this as is, I just want to make sure
that I have understood correctly.<br>
<br>
<br>
Some minor comments that you can feel free to ignore. I won't insist
on any of them:<br>
<br>
concurrentMark.inline.hpp<br>
<meta http-equiv="content-type" content="text/html;
charset=windows-1252">
The comment saying "Immediately process arrays of binary data" is a
little odd to me. I think I would prefer something like "Immediately
process arrays of primitive types".<br>
<br>
<br>
g1CollectedHeap.cpp<br>
<br>
1942 _in_cset_fast_test.initialize(<br>
1943 _hrm.reserved().start(), _hrm.reserved().end(),
HeapRegion::GrainBytes);<br>
1944 _humongous_reclaim_candidates.initialize(<br>
1945 _hrm.reserved().start(), _hrm.reserved().end(),
HeapRegion::GrainBytes);<br>
<br>
When we line break long parameter lists we usually put one parameter
per line. The above would be:<br>
<br>
<tt>_in_cset_fast_test.initialize(_hrm.reserved().start(),</tt><tt><br>
</tt><tt> _hrm.reserved().end(),</tt><tt><br>
</tt><tt> </tt><tt>HeapRegion::GrainBytes);</tt><tt><br>
</tt><tt>_humongous_reclaim_candidates.initialize(_hrm.reserved().start(),</tt><tt><br>
</tt><tt>
_hrm.reserved().end(),</tt><tt><br>
</tt><tt> </tt><tt>HeapRegion::GrainBytes);</tt><br>
<br>
See for example the call to
JavaThread::satb_mark_queue_set().initialize() a few lines below
this code.<br>
<br>
<br>
This comment in humongous_region_is_candidate() reads a bit strange
to me.<br>
<br>
3445 // However, we presently only nominate is_typeArray()
objects.<br>
3446 // A humongous object containing references induces
remembered<br>
3447 // set entries on other regions. In order to reclaim such
an<br>
3448 // object, those remembered sets would need to be cleaned
up.<br>
<br>
I think the main problem with reclaiming objects that contain
references is that they may be the only old objects that keep an
object graph alive. If new objects reference any object in that
object graph we have to be sure that they are kept alive. Otherwise
we will have stale pointers in the new objects.<br>
<br>
<br>
g1_globals.hpp<br>
No changes. This is probably just webrev acting up. For some reason
it includes files that have been modified and then reverted. But
there shouldn't be any changes to g1_globals.hpp, right?<br>
<br>
TEST.groups<br>
Why is the test TestGreyReclaimedHumongousObjects excluded from the
hotspot_gc target?<br>
<br>
Thanks,<br>
Bengt<br>
<br>
<br>
<br>
<blockquote
cite="mid:E56543E1-E37F-4641-9B94-2A8939944A99@oracle.com"
type="cite">
<pre wrap="">
Incremental webrev:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~kbarrett/8069367/webrev.04.incr/">http://cr.openjdk.java.net/~kbarrett/8069367/webrev.04.incr/</a>
The incremental webrev might be a little misleading, since the base
for it is not the version from the previous round of review, but
rather that version plus the merge for 8076265.
Testing:
Hand testing
JPRT
Aurora Ad-hoc GC Nightly using G1
local RefWorkload using G1
</pre>
</blockquote>
<br>
</body>
</html>