RFR (M) 8150393: Maintain the set of survivor regions in an array between GCs
thomas.schatzl at oracle.com
Thu Apr 28 15:50:51 UTC 2016
On Thu, 2016-04-28 at 12:59 +0200, Mikael Gerdin wrote:
> Please review this refactoring of the G1 YoungList class to simplify
> management of young regions.
> Survivor regions are tracked in a sublist of the YoungList linked
> (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
> at the end of a GC the amazingly named
> 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
> incremental collection set and the young list and the only case
> they are not representing the exact same set of regions is during a
> where the "to-space" survivor regions need explicit tracking.
> By tracking the survivor regions in an array instead of a linked list
> can also speed up concurrent root region scanning by getting rid of
> mutex synchronization when iterating over the survivor regions to
> 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
> Webrev of complete change:
> Webrev of survivor array creation:
> Webrev of cleanup of survivor linked list uses
> Bug: https://bugs.openjdk.java.net/browse/JDK-8150393
> Testing: GC RBT Testing, Performance testing shows no significant
- 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
- instaed of
293 int claimed_index = Atomic::add(1, &_claimed_survivor_index);
294 assert(claimed_index > 0, "%d must always be positive",
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
- 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...)
More information about the hotspot-gc-dev