<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<tt>Thanks for addressing my initial comments Thomas,</tt><br>
<br>
<div class="moz-cite-prefix">On 2018-03-07 09:59, Thomas Schatzl
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:1520413176.2346.9.camel@oracle.com">
<pre wrap="">Hi all,
Stefan had some comments about the HeapRegionType parameter in the
G1RemSetTrackingPolicy::update_at_allocate() method - that one is not
really required when placing the call to this method correctly and just
using the HeapRegion's type directly.
Changing this removes another 40 LOC of changes.
There has been another bug introduced by me during cleaning up for
final review: G1RemSetTrackingPolicy::update_at_free() is empty in this
change, which is wrong for this change - in this change we still do not
free the remembered sets during the cleanup pause, only in the
Concurrent Cleanup phase. When doing this concurrently, an assert
triggers when setting the remembered set state to empty outside a
safepoint.
The fix is to make the remembered set untracked during the Cleanup
phase still.
New webrevs:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Etschatzl/8180415/webrev.0_to_1">http://cr.openjdk.java.net/~tschatzl/8180415/webrev.0_to_1</a> (diff)
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Etschatzl/8180415/webrev.1">http://cr.openjdk.java.net/~tschatzl/8180415/webrev.1</a> (full)
</pre>
</blockquote>
<tt>The change looks good. Just some additional comments:<br>
src/hotspot/share/gc/g1/g1ConcurrentMark.cpp<br>
1255 GCTraceTime(Debug, gc)("Complete Remembered Set
Tracking");<br>
...<br>
1291 GCTraceTime(Debug, gc)("Finalize Concurrent Mark
Cleanup");<br>
<br>
Please add the "phases" tag to the above logging to be more
consistent with the remark and other pauses. <br>
---<br>
src/hotspot/share/gc/g1/g1FullGCOopClosures.inline.hpp<br>
52 template <class T> inline void
G1AdjustClosure::adjust_pointer(T* p) <br>
<br>
In this method the comments should be updated since we no longer
return anything.<br>
---<br>
src/hotspot/share/gc/g1/g1RemSet.cpp<br>
757 HeapRegion* start_region =
hr->humongous_start_region();<br>
<br>
start_region is only used to get bottom and crate an oop, so maybe
instead create the oop directly, like:<br>
oop humongous = oop(hr->humongous_start_region()->bottom());<br>
<br>
761 // A humongous object is live (with respect to the
scanning) either<br>
762 // a) it is marked on the bitmap as such<br>
763 // b) its TARS is larger than nTAMS, i.e. has been
allocated during marking.<br>
764 if
(_cm->next_mark_bitmap()->is_marked(start_region->bottom())
||<br>
765 (top_at_rebuild_start >
hr->next_top_at_mark_start())) {<br>
<br>
Break this check out to get something like: is_live(humongous,
tars, ntams).<br>
<br>
There is also the marked_bytes counting in this method that makes
the code a bit harder to follow. I've discussed this with Thomas
offline and he has an idea of how to avoid this. <br>
---<br>
<br>
Also in G1FullCollector::phase3_adjust_pointers() there is a
GCTraceTime that needs updating, since we no longer rebuild
remember sets.<br>
<br>
Thanks,<br>
Stefan<br>
<br>
</tt>
<blockquote type="cite"
cite="mid:1520413176.2346.9.camel@oracle.com">
<pre wrap="">Thanks,
Thomas
On Mon, 2018-03-05 at 16:07 +0100, Thomas Schatzl wrote:
</pre>
<blockquote type="cite">
<pre wrap="">Hi all,
can I have reviews for this change that implements the bulk of the
"rebuild remembered sets concurrently" mechanism?
This change modifies the management of remembered sets so that those
are only maintained when they are useful. This roughly means:
- always maintain remembered sets for young gen regions
- (almost) always maintain remembered sets for humongous regions (for
eager reclaim) as long as they meet the eager reclaim criteria.
- never create remembered sets for archive regions
- do not create remembered sets for any regions during full gc
- do not create remembered sets for regions with evacuation failure
(The latter two decisions may not be optimal, but the idea was to
strictly only maintain remembered sets for regions we are going to
try
to evacuate soon)
- create remembered sets for old gen regions after finishing marking
(starting at the Remark pause), iff
- the liveness of these regions indicate so (liveness <=
G1MixedGCLiveThresholdPercent)
- any (non-objArray) humongous region that does not have a
remembered
set yet (eg. because of full gc)
(Please see the new G1RemSetTrackingPolicy implementation for exact
details)
During the following "Rebuild Remembered Set" phase, that replaces
the
"Create Live Data" phase, all non-young regions that may contain
outgoing pointers (i.e. excluding closed archive regions), G1 scans
the
live objects in the regions between bottom and TARS
("top_at_rebuild_start" - yes, that's new :) for inter-region
references and add them to the appropriate remembered set.
The TARS, as the name indicates, is the top-value at the start of
remark, and the regions contain live data between this value and
bottom
that needs to be scanned during this rebuild phase.
The reason for a new indicator is that nTAMS is *not* sufficient:
during marking there might have been more allocations in the old gen
that also need to be scanned for new references.
All references above TARS (and below the region's top) will be
handled
by the existing barrier mechanism.
After rebuild, during the Cleanup pause, we create the new
(candidate)
collection set, eventually filtering out more regions, like humongous
ones that exceed the eager reclaim criteria.
The current state of the remembered set for a given region is tracked
within the remembered set for that region (HeapRegionRememberedSet
class): a remembered set can be either Untracked, Updating or
Complete.
Untracked means that we do not manage a remembered set for this
region,
Complete means just that, and Updating means that we are currently in
the process of updating the remembered set. This distinction is
relevant for evacuation, e.g. we obviously must not evacuate not-
Complete remembered sets.
Note that this change contains one single temporary ugly hack that
allows G1 to work until the next change :) - so in this change G1
determines whether there will be a mixed gc phase during the Cleanup
pause. It needs to determine this information at that time, instead
of
previously during concurrent cleanup, to, if there is no mixed gc
coming, drop all remembered sets to not maintain them further.
For this reason, and to avoid duplicate recalculation that may yield
different results after dropping remembered sets, there is a new flag
mixed_gc_pending in CollectorState, making it even more complicated.
That one is going away in the next change, promised. :)
Some imho useful messages about remembered set tracking are printed
using "gc, remset, tracking" log tags, both in debug and trace level.
CR:
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8180415">https://bugs.openjdk.java.net/browse/JDK-8180415</a>
Webrev:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Etschatzl/8180415/webrev/index.html">http://cr.openjdk.java.net/~tschatzl/8180415/webrev/index.html</a>
Testing:
hs-tier 1-5, etc. etc.
Thanks,
Thomas
</pre>
</blockquote>
</blockquote>
<br>
</body>
</html>