RFR [L][4/7]: 8180415: Rebuild remembered sets during the concurrent cycle

Thu Mar 8 09:15:46 UTC 2018

Thanks for addressing my initial comments Thomas,

On 2018-03-07 09:59, Thomas Schatzl wrote:
> 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:
> http://cr.openjdk.java.net/~tschatzl/8180415/webrev.0_to_1  (diff)
> http://cr.openjdk.java.net/~tschatzl/8180415/webrev.1  (full)
The change looks good. Just some additional comments:
src/hotspot/share/gc/g1/g1ConcurrentMark.cpp
1255     GCTraceTime(Debug, gc)("Complete Remembered Set Tracking");
...
1291     GCTraceTime(Debug, gc)("Finalize Concurrent Mark Cleanup");

Please add the "phases" tag to the above logging to be more consistent 
with the remark and other pauses.
---
src/hotspot/share/gc/g1/g1FullGCOopClosures.inline.hpp
52 template <class T> inline void G1AdjustClosure::adjust_pointer(T* p)

In this method the comments should be updated since we no longer return 
anything.
---
src/hotspot/share/gc/g1/g1RemSet.cpp
757         HeapRegion* start_region = hr->humongous_start_region();

start_region is only used to get bottom and crate an oop, so maybe 
instead create the oop directly, like:
oop humongous = oop(hr->humongous_start_region()->bottom());

  761         // A humongous object is live (with respect to the 
scanning) either
  762         // a) it is marked on the bitmap as such
  763         // b) its TARS is larger than nTAMS, i.e. has been 
allocated during marking.
  764         if 
(_cm->next_mark_bitmap()->is_marked(start_region->bottom()) ||
  765           (top_at_rebuild_start > hr->next_top_at_mark_start())) {

Break this check out to get something like: is_live(humongous, tars, ntams).

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.
---

Also in G1FullCollector::phase3_adjust_pointers() there is a GCTraceTime 
that needs updating, since we no longer rebuild remember sets.

Thanks,
Stefan

> Thanks,
>    Thomas
>
> On Mon, 2018-03-05 at 16:07 +0100, Thomas Schatzl wrote:
>> 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:
>> https://bugs.openjdk.java.net/browse/JDK-8180415
>> Webrev:
>> http://cr.openjdk.java.net/~tschatzl/8180415/webrev/index.html
>> Testing:
>> hs-tier 1-5, etc. etc.
>>
>> Thanks,
>>    Thomas

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20180308/faab4d4a/attachment.htm>