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

[Stefan Johansson]

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

Otherwise I think this looks great.

Thanks,
Stefan

>
> Webrevs:
> http://cr.openjdk.java.net/~tschatzl/8197932/webrev.1_to_2/ (diff)
> http://cr.openjdk.java.net/~tschatzl/8197932/webrev.2/ (full)
> testing:
> hs-tier1-5
>
> Thanks,
>    Thomas
>