RFR: Handle old, pinned regions [v7]

[Y. Srinivas Ramakrishna]

On Wed, 3 Aug 2022 01:10:57 GMT, William Kemper <wkemper at openjdk.org> wrote:

>> Prior to this change, mixed collections were entirely ignoring the pinned status of old regions. Pinned old regions would not be coalesced and filled (which could lead to a crash) and their pin status was not being synchronized with the pin count (which could also lead to a crash). To address these problems, the following changes have been made:
>> 
>> * Before adding old regions to the collection set, their pinned status is synchronized.
>> * Pinned old regions will _not_ be added to a mixed collection, they will be skipped and considered for inclusion in the next mixed collection.
>> * Any old regions which have not been evacuated after the last mixed collection will be made parseable (coalesced and filled) _at the start of the next old marking cycle_.
>
> William Kemper has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 21 commits:
> 
>  - Merge branch 'shenandoah-master' into fill-old-pinned
>  - Do not drop abbreviated argument when recording success concurrent cycle
>  - Fix whitespace
>  - Prepare for review
>  - Merge branch 'shenandoah-master' into fill-old-pinned
>  - Validate old gen state transitions
>  - Make use of old generation without down casting
>  - Make verifier aware of changes to old generation state machine
>  - Make rset scan aware of changes to old generation state machine
>  - Fix use of index as count and over-counting of garbage
>  - ... and 11 more: https://git.openjdk.org/shenandoah/compare/60e43d04...bdda91d6

Reviewed about half the code. Thought I'd release those pending review comments and then continue the rest of the review later today. Sorry for the slow pace of review because of my relative unfamiliarity with the minutiae and history of the code, and thanks for your patience!

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

> 83:   ShenandoahOldHeuristics(ShenandoahOldGeneration* generation, ShenandoahHeuristics* trigger_heuristic);
> 84: 
> 85:   virtual void choose_collection_set(ShenandoahCollectionSet* collection_set, ShenandoahOldHeuristics* old_heuristics) override;

Not your code, but why pass a pointer to a ShenandoahHeuristics object to a method on that same object type? Are there multiple heuristics objects somehow being composed here via delegation or something?

src/hotspot/share/gc/shenandoah/shenandoahControlThread.hpp line 73:

> 71:   typedef enum {
> 72:     none,
> 73:     concurrent_normal,

In generational mode, is `concurrent_normal` mode a synonym for `servicing_young` mode (in your new `servicing_old` mode parlance?)

It would be good to somehow match these modes with those in ASCII art state transition diagram in ShenandoahControlThread::service_concurrent_cycle() (may be expressed as a morphism from that state transition graph to a `_mode` state transition graph here, in case such a correspondence exists).

src/hotspot/share/gc/shenandoah/shenandoahControlThread.hpp line 106:

> 104: 
> 105:   bool check_cancellation_or_degen(ShenandoahGC::ShenandoahDegenPoint point);
> 106:   bool resume_concurrent_old_cycle(ShenandoahGeneration* generation, GCCause::Cause cause);

For these methods returning a boolean result, please add a line of documentation stating what the return values represent, as done for `service_stw_degenerated_cycle` further below.

src/hotspot/share/gc/shenandoah/shenandoahGeneration.hpp line 123:

> 121:   void merge_write_table();
> 122: 
> 123:   // Used by concurrent and degenerated GC to reset regions.

The best way to write comments in a header file is to specify post-conditions of methods (and as appropriate any preconditions that callers must satisfy), saying as little as possible about what is done to accomplish the post-conditions. The latter documentation would belong in the implementation of the method.

src/hotspot/share/gc/shenandoah/shenandoahGeneration.hpp line 126:

> 124:   virtual void prepare_gc();
> 125: 
> 126:   // Return true iff prepared collection set includes at least one old-gen HeapRegion.

The comment about a return value seems outdated. A brief description of what the method is expected to do would be good here.

src/hotspot/share/gc/shenandoah/shenandoahMarkClosures.cpp line 40:

> 38: void ShenandoahFinalMarkUpdateRegionStateClosure::heap_region_do(ShenandoahHeapRegion* r) {
> 39:   if (r->is_active()) {
> 40:     if (_ctx != nullptr) {

What are the possible circumstances when we might call here with `_ctx == nullptr` ?

May be a brief comment would be helpful.

src/hotspot/share/gc/shenandoah/shenandoahMarkClosures.cpp line 70:

> 68:   } else {
> 69:     assert(!r->has_live(), "Region " SIZE_FORMAT " should have no live data", r->index());
> 70:     if (_ctx != nullptr) {

Nit: The `if` clause could be pulled into the assert as `_ctx == nullptr || ....`.

src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.cpp line 399:

> 397: }
> 398: 
> 399: void ShenandoahOldGeneration::record_success_concurrent(bool abbreviated) {

Is the `abbreviated` parameter being deliberately ignored here? (Sometimes we specify that intent by naming the parm `ignored` or by specifying `/* ignored */` next to it.) The specification of the method in the corresponding header file should specify the semantics of the parameter and what is being recorded.

src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.hpp line 82:

> 80:   virtual void record_success_concurrent(bool abbreviated) override;
> 81: 
> 82:   enum State {

An intended state transition graph would be useful here as a documentation aid/device (or may be in `validate_transition` ?).

src/hotspot/share/gc/shenandoah/shenandoahYoungGeneration.cpp line 46:

> 44:   ShenandoahHeap* heap = ShenandoahHeap::heap();
> 45:   heap->set_concurrent_young_mark_in_progress(in_progress);
> 46:   if (_old_gen_task_queues != nullptr && in_progress && !heap->is_prepare_for_old_mark_in_progress()) {

Not a change, but what exactly is `_old_gen_task_queues != nullptr` checking? Is it checking if we are in a config where old gen is configured (i.e. generation shenandoah)? If so, this seems a bit subtle and fragile and might be made a more direct check? Indeed what would happen then if the the first conjunct were dropped entirely. I'd expect that the last would always return false and you'd get the desired result anyway?

It's of course likely I am misunderstanding the rationale behind that check. In which case, perhaps the comment below could be extended to address this.

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

PR: https://git.openjdk.org/shenandoah/pull/149