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

[Thomas Schatzl]

Hi all,

  while working on some follow-up changes and discussing with StefanJ,
there some issues came up:

- the amount of marked bytes for humongous objects has been calculated
incorrectly, regardless of TAMS value it always considered humongous
objects as below tams.
There is no actual impact except in gc+liveness log messages in that
even if the humongous object were allocated during marking, it looked
like it had been allocated before marking.

This has been found by the sanity check at the end of the rebuilding in
later changes where it has been made to work with humongous regions too
due to other changes.

- StefanJ mentioned that gc+remset+tracking log messages should have a
gc id. Added.

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

Thanks,
  Thomas

On Thu, 2018-03-08 at 12:49 +0100, Thomas Schatzl wrote:
> 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
>