RFR: 8325808: GenShen: Move generational mode code out of shFullGC.cpp

[Y. Srinivas Ramakrishna]

On Fri, 16 Feb 2024 19:42:54 GMT, William Kemper <wkemper at openjdk.org> wrote:

> This change reduces the differences from the upstream branch by moving large chunks of generational mode code into separate files.

Looks good, but left a few suggestions, one of which (if the suggested slight simplification were to work out) would affect the sister PR as well.

Otherwise looks great; the refactoring and pulling the work out of line into its separate class like you've done here should make upstreaming easier. Thanks!

src/hotspot/share/gc/shenandoah/shenandoahFullGC.cpp line 183:

> 181:     // b. Cancel all concurrent marks, if in progress
> 182:     if (heap->is_concurrent_mark_in_progress()) {
> 183:       // TODO: Send cancel_concurrent_mark upstream? Does it really not have it already?

We have:

void ShenandoahHeap::cancel_concurrent_mark() {
  _young_generation->cancel_marking();
  _old_generation->cancel_marking();
  _global_generation->cancel_marking();

  ShenandoahBarrierSet::satb_mark_queue_set().abandon_partial_marking();
}


whereas upstream has:

    // b. Cancel concurrent mark, if in progress
    if (heap->is_concurrent_mark_in_progress()) {
      ShenandoahConcurrentGC::cancel();
      heap->set_concurrent_mark_in_progress(false);
    }


Should probably be reconciled and upstreamed if not here, then in a separate but linked CR.

src/hotspot/share/gc/shenandoah/shenandoahFullGC.cpp line 460:

> 458: 
> 459: template<typename ClosureType>
> 460: void ShenandoahPrepareForCompactionTask::prepare_for_compaction(ClosureType& cl,

Looking at this method, it looks like it actually belongs as a public method in the closure, with that closure's methods invoked here becoming private to the closure. Makes for a narrower public API all around and keeps the loop in the right place.

src/hotspot/share/gc/shenandoah/shenandoahFullGC.cpp line 773:

> 771:     distribute_slices(worker_slices);
> 772: 
> 773:     // TODO: This is ResourceMark is missing upstream.

May be roll it into the refactoring PR you have open and under review at https://github.com/openjdk/jdk/pull/17894/files#diff-d58d0fe2415e3fa64f687435baf73227fb4e3e9eee8ba52ba1f3bb8301437106 ?

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

> 124:   size_t available() const override;
> 125:   size_t available_with_reserve() const;
> 126:   size_t used_and_wasted() const {

Nit: technically `used_or_wasted()`, since space that is used is not wasted.

src/hotspot/share/gc/shenandoah/shenandoahGenerationalFullGC.hpp line 46:

> 44:   static void account_for_region(ShenandoahHeapRegion* r, size_t &region_count, size_t &region_usage, size_t &humongous_waste);
> 45:   static void restore_top_before_promote(ShenandoahHeap* heap);
> 46:   static void maybe_coalesce_and_fill_region(ShenandoahHeapRegion* r);

Please provide a 1-line documentation for each of these public API methods.

src/hotspot/share/gc/shenandoah/shenandoahGenerationalFullGC.hpp line 49:

> 47: };
> 48: 
> 49: class ShenandoahPrepareForGenerationalCompactionObjectClosure : public ObjectClosure {

See comment in `ShenandoahPrepareForCompactionTask::prepare_for_compaction()`. I think this closure should include `prepare_for_compaction` in its public API, and full gc should just invoke that method of the closure directly.

If this simplification works out, then it would also affect the sister upstream PR https://github.com/openjdk/jdk/pull/17894 similarly.

src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp line 35:

> 33: 
> 34: 
> 35: class ShenandoahSetRememberedCardsToDirtyClosure : public BasicOopIterateClosure {

Nit: I realize this is the existing name of the closure, and you just moved it here, but consider the following renaming suggestion: `ShenandoahDirtyRememberedSetClosure`

src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp line 47:

> 45:   template<class T>
> 46:   inline void work(T* p) {
> 47:     T o = RawAccess<>::oop_load(p);

May be a paranoid assert (unless it slows things down too much):

assert(_heap->is_in_old(p), "Expecting to get an old gen address");

src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp line 51:

> 49:       oop obj = CompressedOops::decode_not_null(o);
> 50:       if (_heap->is_in_young(obj)) {
> 51:         // Found interesting pointer.  Mark the containing card as dirty.

Nit: may be:

// Dirty the card containing the cross-generational pointer.

src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp line 411:

> 409:   ShenandoahHeap* heap = ShenandoahHeap::heap();
> 410:   RememberedScanner* scanner = heap->card_scan();
> 411:   ShenandoahSetRememberedCardsToDirtyClosure dirty_cards_for_interesting_pointers;

Nit: I might rename "interesting" to something more concrete & meaningful, like "cross_generational", so:

ShenandoahDirtyRememberedSetClosure dirty_cards_for_cross_generational_pointers;

src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp line 452:

> 450:     }
> 451:     // else, this region is FREE or YOUNG or inactive and we can ignore it.
> 452:     // TODO: Assert this.

I'd delete the TODO, since it would be tautological and not provide any additional value if you were to say:

  assert(some disjunction of conditions || !r->is_active(), "Error");

because of the last disjunct which is always true because of the:

  if (r->is_old() && r->is_active()) {

src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.hpp line 1051:

> 1049: // After Full GC is done, reconstruct the remembered set by iterating over OLD regions,
> 1050: // registering all objects between bottom() and top(), and setting remembered set cards to
> 1051: // DIRTY if they hold interesting pointers.

Nit: instead of:

// ... setting remembered set cards to DIRTY if they hold interesting pointers

perhaps:

// ... dirty the cards containing cross-generational pointers.

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

PR Review: https://git.openjdk.org/shenandoah/pull/398#pullrequestreview-1888718888
PR Review Comment: https://git.openjdk.org/shenandoah/pull/398#discussion_r1494881735
PR Review Comment: https://git.openjdk.org/shenandoah/pull/398#discussion_r1494910380
PR Review Comment: https://git.openjdk.org/shenandoah/pull/398#discussion_r1494902839
PR Review Comment: https://git.openjdk.org/shenandoah/pull/398#discussion_r1494802378
PR Review Comment: https://git.openjdk.org/shenandoah/pull/398#discussion_r1494905029
PR Review Comment: https://git.openjdk.org/shenandoah/pull/398#discussion_r1494913729
PR Review Comment: https://git.openjdk.org/shenandoah/pull/398#discussion_r1494740029
PR Review Comment: https://git.openjdk.org/shenandoah/pull/398#discussion_r1494762829
PR Review Comment: https://git.openjdk.org/shenandoah/pull/398#discussion_r1494765314
PR Review Comment: https://git.openjdk.org/shenandoah/pull/398#discussion_r1494774978
PR Review Comment: https://git.openjdk.org/shenandoah/pull/398#discussion_r1494800047
PR Review Comment: https://git.openjdk.org/shenandoah/pull/398#discussion_r1494782959