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

[Mikael Gerdin]

Hi Thomas,

On 2016-05-02 14:40, Thomas Schatzl wrote:
> 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.

Oh, right.
Will do.
Thanks for the review.

/Mikael

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