RFR: Mixed evacuation [v2]

[Roman Kennke]

On Thu, 8 Apr 2021 15:32:13 GMT, Kelvin Nilsen <kdnilsen at openjdk.org> wrote:

>> This code adds to generational Shenandoah the ability to perform concurrent garbage collection of young-gen and old-gen memory. Following completion of an old-gen concurrent marking effort, we select certain old-gen heap regions to serve as candidates for future collection sets. All dead objects within the old-gen heap regions that are not part of this candidate set are coalesced and filled so that remembered-set scanning of these old-gen heap regions will not be confused by "zombie objects" (objects that old-gen has decided are dead, which reside in regions that have not yet been collected). After concurrently coalescing and filling these dead objects, each subsequent young-gen evacuation pass includes a subset of the old-gen candidates until all candidates have been collected. This code passes TIER1 and hotspot-gc-shenandoah jtreg tests without regressions. A new jtreg test has been added to exercise concurrent old/young GC.
>
> Kelvin Nilsen has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Chasing bug during prepare_for_old_collection
>   
>   One bug fix, a few cosmetic improvements, a change in loop structure to possibly avoid a gcc
>   optimization error.

Nice progress!
First-pass review follows.

Please, check out jcheck complaints under the 'Checks' tab and fix them. I'll start a review in the meantime.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahHeuristics.cpp line 92:

> 90:     prepare_for_old_collections();
> 91:   } else {
> 92:     ShenandoahHeap* heap = ShenandoahHeap::heap();

I'd factor out the whole else block here to call prepare_for_young_collection() or whatever is fitting there.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahHeuristics.cpp line 440:

> 438:   return (_generation->generation_mode() == GLOBAL)
> 439:     || (_generation->generation_mode() == YOUNG && region->affiliation() == YOUNG_GENERATION)
> 440:     || (_generation->generation_mode() == OLD && region->affiliation() == OLD_GENERATION);

I wouldn't change indentation here, or if it needs changing, then line up the || with the _generation above. Iow, shift it 4 spaces to the right instead of 2 spaces to the left.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahHeuristics.cpp line 454:

> 452:   for (size_t i = 0; i < num_regions; i++) {
> 453:     ShenandoahHeapRegion* region = heap->get_region(i);
> 454:     if (!in_generation(region))

Please always use { and } in if (and else) constructs.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahHeuristics.cpp line 475:

> 473: 
> 474:   // This loop is written as while rather than for because of
> 475:   // suspected gcc error in translating/optimizing for-loop

WTF, really? Do we have any indication of this?

src/hotspot/share/gc/shenandoah/heuristics/shenandoahHeuristics.cpp line 515:

> 513:   // evacuated.  In the future, this threshold percentage may be specified on
> 514:   // the command line or preferrably determined by dynamic heuristics.
> 515: #define CollectionThresholdGarbagePercent 50

We already have ShenandoahGarbageThreshold (defaults to 60 iirc) maybe we want to use this? Or use separate global/old/young thresholds?

src/hotspot/share/gc/shenandoah/heuristics/shenandoahHeuristics.hpp line 80:

> 78: // For expediency, we'll have a single class that is a "union" of
> 79: // necessary functionality.
> 80: 

Hmmhmm, this seems a suboptimal design. My intuition would have been to not have each heuristic need to know about any old heuristic, but keep them separate, and compose functionality elsewhere, but I leave that to you. The way it looks now it may end up recursive (young-heuristics->old-heuristics->old-heuristics->...-> etc)

src/hotspot/share/gc/shenandoah/shenandoahControlThread.cpp line 518:

> 516:   // precisely where the regulator is allowed to cancel a GC.
> 517:   ShenandoahOldGC gc(generation, _allow_old_preemption);
> 518: 

Lone whitespace change here.

src/hotspot/share/gc/shenandoah/shenandoahGeneration.cpp line 129:

> 127: void ShenandoahGeneration::prepare_gc() {
> 128:   reset_mark_bitmap();
> 129: 

Lone whitespace change here.

test/hotspot/jtreg/gc/shenandoah/generational/TestConcurrentEvac.java line 35:

> 33:  *  of old-gen GC passes is very simplistic.  A further shortcoming of the
> 34:  *  Generational Shenandoah as of introduction of this test is that it does
> 35:  *  currently support full GC.  If garbage collection falls behind mutator

You probably meant to say that it "does currently *not* support full GC"

-------------

Changes requested by rkennke (Reviewer).

PR: https://git.openjdk.java.net/shenandoah/pull/29