RFR: 8325673: GenShen: Share Reserves between Old and Young Collector [v6]

[Y. Srinivas Ramakrishna]

On Thu, 18 Jul 2024 18:00:25 GMT, Kelvin Nilsen <kdnilsen at openjdk.org> wrote:

>> Allow young-gen Collector reserve to share memory with old-gen Collector reserve in order to support prompt processing of mixed evacuations, as constrained by ShenandoahOldEvacRatioPercent.
>
> Kelvin Nilsen has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Improve comment

Left a few suggestions; would be great to take care of the re-order thing that confuses the github diffs. Not sure if restoring old order of those methods in `shenandoahOldHeuritsics.cpp` is possible/easy. See comments inline.

Flushing these for now, but will continue in that file and remaining ones in a subsequent session later today/tomorrow.

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

> 81: #ifdef ASSERT
> 82:   if (uint(os::random()) % 100 < ShenandoahCoalesceChance) {
> 83:     return true;

The change in the order of the methods has left the diffs very confused, making it a it difficult to read. Would it be possible to restore the old order of definition of these methods, unless there was a specific reason to reorder them? (May be the reordering, if important, could be deferred to a separate PR that only does that; just to make reviewers' lives easier :-)

src/hotspot/share/gc/shenandoah/heuristics/shenandoahOldHeuristics.hpp line 106:

> 104:   size_t _fragmentation_last_old_region;
> 105: 
> 106:   // State variables involved in construction of a mixed-evacuation collection set

Can you add a sentence describing when the state is updated, and when it is used, as well as which are inputs to the construction process and which the results of (or updated upon completion of) the construction process and used as inputs to downstream operations?

src/hotspot/share/gc/shenandoah/heuristics/shenandoahOldHeuristics.hpp line 141:

> 139:   void choose_collection_set_from_regiondata(ShenandoahCollectionSet* set, RegionData* data, size_t data_size, size_t free) override;
> 140: 
> 141:   // Return true iff we need to finalize mixed evacs

In addition to the return value, please add a sentence stating something like "Adds old regions to the collection set." What does "need to finalize mixed evacs" mean? Did you mean: "Returns true if and only if this step should be followed by ... (see method finalize_mixed_evacs)." In general, it's better to write comments such that they do not require chains of lookup (although in some cases it may be unavoidable).

src/hotspot/share/gc/shenandoah/heuristics/shenandoahOldHeuristics.hpp line 156:

> 154:   bool top_off_collection_set();
> 155: 
> 156:   // Return true iff the collection set holds at least one unpinned mixed evacuation candidate

Same comment as the last one. The comment must not only state the value returned, but also what actions the method takes or side-effect it has on this or another interacting object's state. You want to do that for all imperative methods. (Only accessor methods may have a single line stating what they return; otherwise the effect of the method should be described.)

src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp line 226:

> 224:     // on its next iteration and run a degenerated young cycle.
> 225: 
> 226:     // vmop_entry_final_updaterefs rebuilds free set in preparation for next GC.

Did you mean to say `vmop_entry_final_roots` in this comment?

src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp line 342:

> 340:   op_final_roots();
> 341: 
> 342:   if (heap->mode()->is_generational()) {

Can this code be factored out into a virtual method `rebuild_free_set()` that `ShenandoahGenerationalHeap` overrides?

src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp line 752:

> 750:     //  been set aside to hold objects evacuated from the young-gen collection set.  Conservatively, this value
> 751:     //  equals the entire amount of live young-gen memory within the collection set, even though some of this memory
> 752:     //  will likely be promoted.

It sounds like these comments should be moved (and split) into the `prepare_regions_and_collection_set()` header specification, and in its override for `ShenandoahOldGeneration`?

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

Changes requested by ysr (Committer).

PR Review: https://git.openjdk.org/shenandoah/pull/395#pullrequestreview-2197025746
PR Review Comment: https://git.openjdk.org/shenandoah/pull/395#discussion_r1690638256
PR Review Comment: https://git.openjdk.org/shenandoah/pull/395#discussion_r1689989807
PR Review Comment: https://git.openjdk.org/shenandoah/pull/395#discussion_r1689995437
PR Review Comment: https://git.openjdk.org/shenandoah/pull/395#discussion_r1689997626
PR Review Comment: https://git.openjdk.org/shenandoah/pull/395#discussion_r1690605746
PR Review Comment: https://git.openjdk.org/shenandoah/pull/395#discussion_r1690609502
PR Review Comment: https://git.openjdk.org/shenandoah/pull/395#discussion_r1690620191