<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<tt>Hi Thomas,</tt><br>
<br>
<div class="moz-cite-prefix">On 2018-03-29 16:35, Thomas Schatzl
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:1522334136.2451.15.camel@oracle.com">
<pre wrap="">Hi all,
can I have reviews for this small change that makes G1 switch the
mark bitmaps already during the Remark pause as opposed waiting until
Cleanup?
This is the last step before G1 can reclaim regions at Remark, which
means that empty regions can be cleaned up more more quickly than
before (JDK-8154528, coming soon :)).
The main changes apart from actually switching the bitmaps consist of
updating the liveness information, i.e. how many bytes are live in a
region, now already available at Remark since JDK-8197850, also at the
same time.
Previously G1 used the Rebuild Remsets phase to calculate that at the
same time.
I could not find significant differences in timing.
This change depends on the recent g1ConcurrentMark refactorings,
particularly JDK-8200234.
CR:
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8178105">https://bugs.openjdk.java.net/browse/JDK-8178105</a>
Webrev:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tschatzl/8178105/">http://cr.openjdk.java.net/~tschatzl/8178105/</a></pre>
</blockquote>
<tt>Looks good, but a few comments:<br>
</tt><tt><br>
src/hotspot/share/gc/g1/g1ConcurrentMark.cpp</tt><br>
<tt>1031 for (uint i = region_idx; i < (region_idx +
num_regions_in_humongous); i++) {</tt><br>
<tt>1032 HeapRegion* const r = _g1h->region_at(i);</tt><br>
<tt>1033 size_t const words_to_add =
MIN2(HeapRegion::GrainWords, marked_words);</tt><br>
<tt>1034 r->add_to_marked_bytes(words_to_add *
HeapWordSize);</tt><br>
<tt>1035 marked_words -= words_to_add;</tt><br>
<tt>1036 }</tt><br>
<tt>Could be nice to assert that marked_words is zero after the
iteration, i.e. that everything got distributed and maybe also
that marked_words is greater than 0 during the iteration.</tt><br>
<br>
<tt>1047 log_trace(gc)("Added " SIZE_FORMAT " words to region
%u (%s)", marked_words, region_idx, hr->get_type_str());</tt><br>
<tt>I think the logging should use at least one more tag, maybe
"region" or "marking", but you probably know what makes most sense
in this area. I also think it would be nice if we got this same
log-line for all regions. Now we get this for "normal" regions,
but we get "Distributing..." for humongous start regions and "Not
Added..." for humongous continuous. So I would suggest adding the
same log-line to the distribute-function after line 1034.<br>
<br>
1045 if (!hr->is_humongous()) {<br>
Since the comment talks about humongous regions I would prefer not
negating the statement and have something like:<br>
if (hr->is_humongous()) {<br>
if (hr->is_starts_humongous()) {<br>
distribute_marked_bytes(hr, marked_words);<br>
} else {<br>
assert(marked_words == 0, "continuous humongous are not
accounted separately");<br>
}<br>
} else {<br>
hr->add_to_marked_bytes(marked_words * HeapWordSize);<br>
log_trace(gc)("Added " SIZE_FORMAT " words to region %u (%s)",
marked_words, region_idx, hr->get_type_str());<br>
}<br>
<br>
</tt><tt><span class="new">1058 }</span></tt><br>
<tt><span class="new"></span></tt><tt><span class="new">Newline
before public :)<br>
<br>
1686 if (mark_completed) {<br>
1687 is_alive = &is_alive_prev;<br>
1688 } else {<br>
1689 is_alive = &is_alive_next;<br>
1690 }<br>
1691 _gc_tracer_cm->report_object_count_after_gc(is_alive);<br>
<br>
I would prefer moving the call into the if and only setup the
closure when needed:<br>
if (mark_completed) {<br>
G1ObjectCountIsAliveClosure is_alive(_g1h);<br>
_gc_tracer_cm->report_object_count_after_gc(&is_alive);<br>
} else {<br>
G1CMIsAliveClosure is_alive(_g1h);<br>
_gc_tracer_cm->report_object_count_after_gc(&is_alive);
<br>
}<br>
</span></tt><tt>----<br>
</tt>
<pre><span class="new"></span></pre>
<tt><span class="new">Thanks,<br>
Stefan<br>
<br>
</span></tt><tt><br>
</tt>
<blockquote type="cite"
cite="mid:1522334136.2451.15.camel@oracle.com">
<pre wrap="">
Testing:
hs-tier 1-5
Thanks,
Thomas
</pre>
</blockquote>
<br>
</body>
</html>