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

[Thomas Schatzl]

Hi Stefan,

On Thu, 2018-03-08 at 10:15 +0100, Stefan Johansson wrote:
> 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).

Fixed.

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

As discussed I would like to postpone this for now as it is much harder
than it looks and warrants its own CR (actually two or three).

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

All done.

New webrevs:
http://cr.openjdk.java.net/~tschatzl/8180415/webrev.1_to_2/ (diff)
http://cr.openjdk.java.net/~tschatzl/8180415/webrev.2/ (full)

Thomas