RFR (M) 8150393: Maintain the set of survivor regions in an array between GCs

[Thomas Schatzl]

Hi Mikael,

On Mon, 2016-05-02 at 13:19 +0200, Mikael Gerdin wrote:
> Hi,
> 
> On 2016-04-28 17:50, Thomas Schatzl wrote:
> > 
> > Hi Mikael,
> > 
> > On Thu, 2016-04-28 at 12:59 +0200, Mikael Gerdin wrote:
> > > 
> > > Hi,
> > > 
> > > Please review this refactoring of the G1 YoungList class to
> > > simplify
> > > the
> > > management of young regions.
> > > 
> > > Survivor regions are tracked in a sublist of the YoungList linked
> > > list
> > > (using HeapRegion::_next_young_region) with explicit tracking of
> > > the
> > > head and tail of the survivor regions.
> > > The number of survivors is tracked in a special counter in
> > > YoungList
> > > and
> > > at the end of a GC the amazingly named
> > > YoungList::reset_auxillary_lists
> > > makes the survivor regions proper members of the YoungList.
> > > 
> > > My suggested fix is to explicitly track survivors in a
> > > GrowableArray
> > > instead to make way for the complete removal of
> > > HeapRegion::_next_young_region. There is a lot of overlap between
> > > the
> > > incremental collection set and the young list and the only case
> > > where
> > > they are not representing the exact same set of regions is during
> > > a
> > > GC,
> > > where the "to-space" survivor regions need explicit tracking.
> > > 
> > > By tracking the survivor regions in an array instead of a linked
> > > list
> > > we
> > > can also speed up concurrent root region scanning by getting rid
> > > of
> > > mutex synchronization when iterating over the survivor regions to
> > > scan.
> > > 
> > > In order to maintain the integrity of the YoungList linked list
> > > I've
> > > modified the code to append the survivors to that list in
> > > reset_auxillary_lists for now, but I plan to change that in an
> > > upcoming
> > > change.
> > > 
> > > Webrev of complete change:
> > > http://cr.openjdk.java.net/~mgerdin/8150393/webrev.0.full/
> > > Webrev of survivor array creation:
> > > http://cr.openjdk.java.net/~mgerdin/8150393/webrev.0.survarray/
> > > Webrev of cleanup of survivor linked list uses
> > > http://cr.openjdk.java.net/~mgerdin/8150393/webrev.0.cleanup/
> > > 
> > > 
> > > Bug: https://bugs.openjdk.java.net/browse/JDK-8150393
> > > Testing: GC RBT Testing, Performance testing shows no significant
> > > regressions
> >    - could you make the _claimed_survivor_index a size_t because it
> > can
> > never ever get negative because it's never decremented.
> Since it's used together with a GrowableArray I'd just be forced to 
> perform a bunch of casts if I made it a size_t so I'd prefer to keep
> it an int. Hopefully someday we'll create a sane array class.

Okay :)

> >    - I find the "1-based index" comments quite confusing, but
> > that's
> > probably just me. Lots of other code does the same without any
> > further
> > commenting on that. I am also not sure how this "avoids signed
> > overflow" issues.

There is still one of those in g1ConcurrentMark.hpp. Care to improve
the "1-based" first word there too?

I do not need a re-review for this.

> >    - instaed of
> > 
> >   293   int claimed_index = Atomic::add(1,
> > &_claimed_survivor_index);
> >   294   assert(claimed_index > 0, "%d must always be positive",
> > claimed_index);
> >   295   claimed_index--;
> > 
> > I would kind of prefer
> > 
> > int claimed_index = Atomic::add(1, &_claimed_survivor_index) - 1;
> > 
> > which is more concise, and the assert quite confusing as
> > Atomic::add guarantees the addition anyway. Since the resulting
> > value is at least always 0 (even more obvious if you make that
> > variable a size_t), I do not see a need for these three lines.
> diff --git a/src/share/vm/gc/g1/g1ConcurrentMark.cpp 
> b/src/share/vm/gc/g1/g1ConcurrentMark.cpp
> --- a/src/share/vm/gc/g1/g1ConcurrentMark.cpp
> +++ b/src/share/vm/gc/g1/g1ConcurrentMark.cpp
> @@ -288,11 +288,7 @@
>     // Currently, only survivors can be root regions.
>     const GrowableArray<HeapRegion*>* survivor_regions = 
> _young_list->survivor_regions();
> 
> -  // The claimed survivor index is a 1-based index into the
> survivor 
> regions array
> -  // this allows us to initialize the index to 0 and avoid signed 
> overflow issues.
> -  int claimed_index = Atomic::add(1, &_claimed_survivor_index);
> -  assert(claimed_index > 0, "%d must always be positive",
> claimed_index);
> -  claimed_index--;
> +  int claimed_index = Atomic::add(1, &_claimed_survivor_index) - 1;
>     if (claimed_index < survivor_regions->length()) {
>       return survivor_regions->at(claimed_index);
>     }
> 

Looks good.

> > 
> > 
> >    - this is just personal preference (and I do not know what's
> > better),
> > I tend to put the region indices into such arrays instead of the
> > pointers. Region indices are always 4 bytes, which is negligible I
> > guess here.
> > 
> >    - pre-existing, and just a note to create a CR:
> > maybe G1ConcurrentMark::scan_root_regions() can be simplified with
> > the
> > recent addition of the run_task() with the # of threads method.
> > 
> > Also, that number of threads should also be capped by the number of
> > available survivor regions (may be interesting on large systems
> > with >100 marking threads...)
> > 

Done, see the other rfr.

Thanks,
  Thomas