RFR: 8333457: GenShen: Move remembered set into new generational code [v2]

[Y. Srinivas Ramakrishna]

On Mon, 3 Jun 2024 23:59:49 GMT, William Kemper <wkemper at openjdk.org> wrote:

>> The remembered set is moved into the old generation and the age census is moved into the generational mode heap.
>
> William Kemper has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix typo in comment

I didn't follow all of the logic in ShenandoahMark::mark_through_ref, but otherwise looks good.

Left a few remarks, none of them super-important.

Thanks for making these changes, further separating out the generational code from the non-generational original.

src/hotspot/share/gc/shenandoah/shenandoahGeneration.cpp line 95:

> 93:   RememberedScanner* _scanner;
> 94:  public:
> 95:   ShenandoahMergeWriteTable() : _heap(ShenandoahHeap::heap()), _scanner(_heap->card_scan()) {}

Wow :-)

src/hotspot/share/gc/shenandoah/shenandoahGeneration.cpp line 106:

> 104: };
> 105: 
> 106: class ShenandoahCopyWriteCardTableToRead: public ShenandoahHeapRegionClosure {

Provides appropriate gravitas to the operation that was lacking in the old name, which conjured up images of [Scrat](https://en.wikipedia.org/wiki/Scrat) :-)

src/hotspot/share/gc/shenandoah/shenandoahGenerationalEvacuationTask.cpp line 171:

> 169:       assert(fill_size >= ShenandoahHeap::min_fill_size(), "previously allocated objects known to be larger than min_size");
> 170:       ShenandoahHeap::fill_with_object(obj_addr, fill_size);
> 171:       old_gen->card_scan()->register_object_without_lock(obj_addr);

May be just stash the RememberedScanner pointer in a const local, and use it everywhere here.

src/hotspot/share/gc/shenandoah/shenandoahGenerationalEvacuationTask.cpp line 236:

> 234: 
> 235:   ShenandoahOldGeneration* const old_generation = _heap->old_generation();
> 236:   ShenandoahGeneration* const young_generation = _heap->young_generation();

Could rename `old_gen, youn_gen` for brevity like at lines 143,144 above. (But not crucial; ignore if it's too much change for no gain.)

src/hotspot/share/gc/shenandoah/shenandoahGenerationalHeap.hpp line 50:

> 48:   // True when regions and objects should be aged during the current cycle
> 49:   ShenandoahSharedFlag  _is_aging_cycle;
> 50:   // Age census used for adapting tenuring threshold in generational mode

Here and at line 62 below, the "in generational mode" in the documentation comment can probably now be dropped, since we are now part of the generational subtype of Shenandoah heap?

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 1:

> 1: /*

Nice cleanups & refactoring!

src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 1:

> 1: /*

Nice cleanup, thank you!!

src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 740:

> 738:   void clear_cards_for(ShenandoahHeapRegion* region);
> 739:   void mark_card_as_dirty(void* location);
> 740: 

Hurray!!

src/hotspot/share/gc/shenandoah/shenandoahHeapRegion.hpp line 32:

> 30: #include "gc/shared/spaceDecorator.hpp"
> 31: #include "gc/shenandoah/shenandoahAffiliation.hpp"
> 32: #include "gc/shenandoah/shenandoahAgeCensus.hpp"

This seemed very curious. Is it just for `CENSUS_NOISE()`? If so, I wonder if we should just pull it up into `shenandoah_globals.hpp` and avoid this seemingly curious include; or may be that's worse than the curious include? :-) 

Doesn't need to be done here necessarily, but the question might come up in a review when pushing this upstream in the future.

src/hotspot/share/gc/shenandoah/shenandoahMark.inline.hpp line 293:

> 291: 
> 292:   assert((GENERATION == GLOBAL || GENERATION == NON_GEN), "Unexpected generation type");
> 293:   return true;

For safety should this return `heap->is_in(obj)`, or if always returning `true` here, should it `assert(heap->is_in(obj), ...);`?

We should document the semantics of both `ShenandoahMark::in_generation()` and `ShenandoahGeneration::contains()` more precsiely than currently done, and audit uses to ensure the callers are cognizant of those sematics. For instance, it sometimes makes sense to have `_or_null` variants of these when a nullptr may be passed to these methods by callers, etc.

src/hotspot/share/gc/shenandoah/shenandoahMark.inline.hpp line 330:

> 328:       // Old mark, found a young pointer.
> 329:       // TODO:  Rethink this: may be redundant with dirtying of cards identified during young-gen remembered set scanning
> 330:       // and by mutator write barriers.  Assert

I assume "Assert" here meant: "Assert card is dirty, instead of marking it."
Would be worth doing that. It makes sense that this card dirtying is redundant, since now old mark has been cleared before that needs to be remarked, nor a new cross-gen pointer being created here.

May be we need a ticket to get rid of this separately.

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

> 191: 
> 192:   if (ShenandoahCardBarrier) {
> 193:     // TODO: Old and young generations should only be instantiated for generational mode

I assume that'll be in a follow-up as discussed at meeting this morning?

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

> 784: }
> 785: 
> 786: void ShenandoahOldGeneration::mark_card_as_dirty(void* location) {

Do we need a paranoid assert here?


 assert(is_in(location), ...);

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

Marked as reviewed by ysr (Committer).

PR Review: https://git.openjdk.org/shenandoah/pull/443#pullrequestreview-2094971710
PR Review Comment: https://git.openjdk.org/shenandoah/pull/443#discussion_r1625123649
PR Review Comment: https://git.openjdk.org/shenandoah/pull/443#discussion_r1625124542
PR Review Comment: https://git.openjdk.org/shenandoah/pull/443#discussion_r1625179047
PR Review Comment: https://git.openjdk.org/shenandoah/pull/443#discussion_r1625181249
PR Review Comment: https://git.openjdk.org/shenandoah/pull/443#discussion_r1625168605
PR Review Comment: https://git.openjdk.org/shenandoah/pull/443#discussion_r1625166926
PR Review Comment: https://git.openjdk.org/shenandoah/pull/443#discussion_r1625165578
PR Review Comment: https://git.openjdk.org/shenandoah/pull/443#discussion_r1625165267
PR Review Comment: https://git.openjdk.org/shenandoah/pull/443#discussion_r1625163101
PR Review Comment: https://git.openjdk.org/shenandoah/pull/443#discussion_r1625155504
PR Review Comment: https://git.openjdk.org/shenandoah/pull/443#discussion_r1625213267
PR Review Comment: https://git.openjdk.org/shenandoah/pull/443#discussion_r1625144445
PR Review Comment: https://git.openjdk.org/shenandoah/pull/443#discussion_r1625139784