RFR: Mixed evacuation [v6]

[Roman Kennke]

On Tue, 20 Apr 2021 00:10:04 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:
> 
>   Change definition of CollectionThresholdGarbagePercent
>   
>   This is now defined to equal ShenandoahGarbageThreshold, which seems to have
>   a default value of 25.  The effect on running workloads is to choose more
>   regions for the collection set than was observed with the previous
>   configuration.
>   
>   Also addressed several improvements in white space and comments.
>   
>   The code now runs tier1 and hotspot_gc_shenandoah without regressions.  It
>   also succsessfully runs an Extremem stress test up until the point of
>   failure due to triggering of full GC (after 113 completed GC passes, including
>   two old-gen GC passes).

Nice. I believe I only have complaints about cosmetic issues.
One design-related question remains: why is it necessary to duplicate all heuristics as old variants? Is there any chance the existing heuristics can be reused? Or does it make sense to have different variants for old? Asking out of curiosity, I'm leaving the design to you.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahAdaptiveHeuristics.cpp line 99:

> 97:                      byte_size_in_proper_unit(min_garbage), proper_unit_for_byte_size(min_garbage));
> 98: 
> 99: 

We have a lone newline here.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahAdaptiveOldHeuristics.cpp line 2:

> 1: /*
> 2:  * Copyright (c) 2018, 2019, 2021, Red Hat, Inc. All rights reserved.

Please, only put first and last year in copyright notice (from-to).

src/hotspot/share/gc/shenandoah/heuristics/shenandoahAdaptiveOldHeuristics.hpp line 2:

> 1: /*
> 2:  * Copyright (c) 2018, 2019, 2021, Red Hat, Inc. All rights reserved.

Same as above.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahAdaptiveOldHeuristics.hpp line 29:

> 27: 
> 28: #include "gc/shenandoah/heuristics/shenandoahOldHeuristics.hpp"
> 29: #include "gc/shenandoah/heuristics/shenandoahAdaptiveHeuristics.hpp"

I don't think you need to include shenandoahAdaptiveHeuristics.hpp here.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahAggressiveHeuristics.cpp line 2:

> 1: /*
> 2:  * Copyright (c) 2018, 2019 Red Hat, Inc. All rights reserved.

Please don't remove the comma.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahAggressiveOldHeuristics.cpp line 2:

> 1: /*
> 2:  * Copyright (c) 2018, 2019, 2021, Red Hat, Inc. All rights reserved.

Same as above. Only put in begin and end year.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahAggressiveOldHeuristics.hpp line 2:

> 1: /*
> 2:  * Copyright (c) 2018, 2019, 2021, Red Hat, Inc. All rights reserved.

Same as above.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahCompactOldHeuristics.cpp line 2:

> 1: /*
> 2:  * Copyright (c) 2018, 2019, 2021, Red Hat, Inc. All rights reserved.

Same as above.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahCompactOldHeuristics.hpp line 2:

> 1: /*
> 2:  * Copyright (c) 2018, 2019, 2021, Red Hat, Inc. All rights reserved.

Same as above.

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

> 1: /*
> 2:  * Copyright (c) 2018, 2020, 2021, Red Hat, Inc. All rights reserved.

Same as above.

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

> 165:       old_heuristics->prime_collection_set(collection_set);
> 166:     }
> 167:     // else, thisi s global collection and doesn't need to prime_collection_set()

Typo here 'thisi s'

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

> 170:   size_t collectable_garbage = collection_set->garbage() + immediate_garbage;
> 171:   size_t collectable_garbage_percent = (total_garbage == 0) ? 0 : (collectable_garbage * 100 / total_garbage);
> 172: 

Stray newline change.

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

> 202: 
> 203: bool ShenandoahHeuristics::should_start_gc() {
> 204: 

Stray newline change.

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

> 153:   double time_since_last_gc() const;
> 154: 
> 155: 

Stray newlines.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahOldHeuristics.cpp line 26:

> 24: 
> 25: #include "gc/shenandoah/heuristics/shenandoahOldHeuristics.hpp"
> 26: #include "precompiled.hpp"

Convention is to always put include for precompiled.hpp first.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahOldHeuristics.cpp line 67:

> 65:   // collection set selection.  Here, we'd rather fall back to this contingent behavior than force a full STW
> 66:   // collection.
> 67: #define PROMOTION_BUDGET_BYTES (ShenandoahHeapRegion::region_size_bytes() / 2)

I'd put macro definitions at the top of the file, right below the includes.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahOldHeuristics.cpp line 220:

> 218:   // future, this threshold percentage may be specified on the command
> 219:   // line or preferrably determined by dynamic heuristics.
> 220: #define CollectionThresholdGarbagePercent ShenandoahGarbageThreshold

Same as above: please place define at top of file.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahOldHeuristics.cpp line 249:

> 247:   _old_coalesce_and_fill_candidates = 0;
> 248: 
> 249: #undef CollectionThresholdGarbagePercent

I don't think the constants need to be undef-ed when defined at top of file. Alternatively, you may consider making them true C++ constants.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahPassiveOldHeuristics.cpp line 2:

> 1: /*
> 2:  * Copyright (c) 2018, 2019, 2021, Red Hat, Inc. All rights reserved.

Same as above, only use first and last years.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahPassiveOldHeuristics.hpp line 2:

> 1: /*
> 2:  * Copyright (c) 2018, 2019, 2021, Red Hat, Inc. All rights reserved.

Same as above.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahStaticHeuristics.hpp line 32:

> 30: // TODO: Make this a subclass of ShenandoahHeuristics and create a "tailored" copy of this class to be subclass of
> 31: // ShenandoahOldHeuristics.  Fix the constructor implementation to invoke ShenandoahHeuristics() superclass constructor.
> 32: // Change ShenandoahMode::initialize_old_heuristics() to return the subclass of ShenandoahOldHeuristics.

Is this comment still valid? You seem to propose a ShenandoahStaticOldHeuristics too.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahStaticOldHeuristics.cpp line 2:

> 1: /*
> 2:  * Copyright (c) 2018, 2019, 2021, Red Hat, Inc. All rights reserved.

Same as above.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahStaticOldHeuristics.hpp line 2:

> 1: /*
> 2:  * Copyright (c) 2018, 2019, 2021, Red Hat, Inc. All rights reserved.

Same as above.

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

Changes requested by rkennke (Reviewer).

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