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

[Mikael Gerdin]

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.

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

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);
    }

/Mikael

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