RFR: Handle old, pinned regions [v7]

[William Kemper]

On Wed, 3 Aug 2022 01:36:36 GMT, Y. Srinivas Ramakrishna <ysr at openjdk.org> wrote:

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

Yes, the `trigger_heuristic` is a delegate. The old heuristics have their own logic for choosing the collection set, but they delegate the trigger decision to the adaptive heuristic (by default).

> 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).

`concurrent_normal` could be a young or _global_ collection. I think the control thread could do with some sort of gc-request/parameter object to tie everything together.

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

Will do.

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

Will add a comment here.

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

Okay.

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

No, that is a mistake. I will fix it. Although, I do not think an old cycle can ever be abbreviated, it's a feature we should support one day.

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

I created such a diagram with GraphViz, but haven't translated it to ascii. I will add a simplified diagram.

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

It's checking if this young collection is a "bootstrap" collection. The non-generational modes are meant to use the `GlobalGeneration` and should not even instantiate the young/old generations.

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

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