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

[Y. Srinivas Ramakrishna]

On Wed, 24 Jul 2024 15:11:56 GMT, Y. Srinivas Ramakrishna <ysr at openjdk.org> wrote:

>> Kelvin Nilsen has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Improve comment
>
> 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.)

(Applies to all the methods line 150-157.)

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

Also, are the comments at line 215 and 226 really needed, since the methods do much more than just rebuilding the free set.

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

Also, can you take a look at `op_final_updaterefs` where `rebuild_free_set` is called? There may be an opportunity to refactor the code here into a method as suggested above, and to share that with `rebuild_free_set` perhaps.

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

The TODO comment at lines 727-734 should similarly move out into the method about which it talks.

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

PR Review Comment: https://git.openjdk.org/shenandoah/pull/395#discussion_r1690632111
PR Review Comment: https://git.openjdk.org/shenandoah/pull/395#discussion_r1690610219
PR Review Comment: https://git.openjdk.org/shenandoah/pull/395#discussion_r1690616973
PR Review Comment: https://git.openjdk.org/shenandoah/pull/395#discussion_r1690620818