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

[Thomas Schatzl]

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.

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

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

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

Thanks,
  Thomas