RFR [M][7/7]: 8197932: Better split work in rebuild remembered sets phase

[Thomas Schatzl]

Hi Stefan,

  thanks for your review, and sorry for the late reply...

On Mon, 2018-03-19 at 16:39 +0100, Stefan Johansson wrote:
> Thanks Thomas,
> 
> On 2018-03-19 13:03, Thomas Schatzl wrote:
> > Hi all,
> > 
> >    Stefan proposed to use some iterator over the work area that
> > internally handles the boundary at the TAMS wrt to liveness when
> > scanning.
> > 
> > This makes the change a bit more compact, performance does not seem
> > to differ, so why not... :)
> 
> I really like this approach :) A few additional small things:
> src/hotspot/share/gc/g1/g1RemSet.cpp
> 787         if (!is_live(_current)) {
> 
> I would like to invert the if-statement in the LiveObjIterator 
> constructor and move the call to move_if_below_ntams() into the 
> if-statement updating _current, something like this:
> if (is_live(_current)) {
>    // Non-objArrays were scanned by the previous part of that region.
>    if (_current < mr.start() && !oop(_current)->is_objArray()) {
>        _current += oop(_current)->size();
>        // We might have positioned _current on a non-live object. 
> Reposition to the next
>        // live one if needed.
>        move_if_below_ntams();
>      }
>    }
> } else {
>    ...
> }
> 
> 926 #ifdef ASSERT
> 927       // In the final iteration of the loop the region might
> have been eagerly reclaimed.
> 928       // Simply filter out those regions. We can not just use
> region 
> type because there
> 929       // might have already been new allocations into these
> regions.
> 930       HeapWord* const top_at_rebuild_start = 
> _cm->top_at_rebuild_start(region_idx);
> 931       assert(!hr->is_old() ||
> 
> What do you think about making this assert into a guarantee or use 
> DEBUG_ONLY() to avoid the #ifdef ASSERT? Also maybe adding something
> to the comment explaining that it is the check on
> top_at_rebuild_start that filters out eagerly reclaimed regions.

Done.

> ---
> src/hotspot/share/gc/g1/g1_globals.hpp
> 259   experimental(size_t, G1RebuildRemSetChunkSize, 256 * K,
> 
> Did you test to see if changing the G1RebuildRemSetChunkSize flag
> had any impact on performance, otherwise maybe we could make it a
> constant instead.

The problem is not so much increasing the value, but decreasing it for
very slow systems. Decreasing it further adds some overhead.

The other point is that since we track the time a chunk takes, it seems
strange to not have any knobs to deal with problems in that area.

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

In this webrev I also did some somewhat messy looking renaming of nTAMS
to TAMS (removing the n) and passing the (n)TAMS around in conjunction
with the bitmap.
This will make a follow-up change a lot easier to read.

Thanks,
  Thomas