RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v8]

[Aleksey Shipilev]

On Thu, 21 Nov 2024 00:59:27 GMT, William Kemper <wkemper at openjdk.org> wrote:

>> This PR merges JEP 404, a generational mode for the Shenandoah garbage collector. The JEP can be viewed here: https://openjdk.org/jeps/404. We would like to target JDK24 with this PR.
>
> William Kemper has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 524 commits:
> 
>  - Merge remote-tracking branch 'shenandoah/master' into great-genshen-pr-redux
>  - 8344670: GenShen: Use concurrent worker session for concurrent mark phase
>    
>    Reviewed-by: kdnilsen
>  - 8344640: GenShen: Reuse existing card mark barrier function when dropping references
>    
>    Reviewed-by: shade, ysr
>  - 8344592: GenShen: Remove unnecessary comments and changes
>    
>    Reviewed-by: kdnilsen
>  - 8344263: GenShen: Reduce extraneous log messages at INFO level
>    
>    Reviewed-by: ysr, shade
>  - 8344638: GenShen: Verifier should not touch claim token
>    
>    Reviewed-by: shade
>  - Merge
>  - 8344320: GenShen: Possible null pointer usage in shGenerationalHeap
>    
>    Reviewed-by: wkemper, ysr
>  - 8344321: GenShen: Fix various sonar scan warnings
>    
>    Reviewed-by: kdnilsen, shade
>  - 8344264: GenShen: Improve comments and method names
>    
>    Reviewed-by: shade
>  - ... and 514 more: https://git.openjdk.org/jdk/compare/95a00f8a...7ab16403

229/229 files read! This is an impressive, monumental piece of work, kudos. 

More comments from this read follow. I am going to circle back to some open threads in this PR. I am also running local tests, and in my ad-hoc performance tests generational mode performs admirably well.

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

> 879:   // update costs on slow path.
> 880:   monitoring_support()->notify_heap_changed();
> 881:   _heap_changed.set();

Why not leave `try_set` intact? This alleviates contention on the shared counter, AFAICS.

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

> 1970:   // Check that if concurrent weak root is set then active_gen isn't null
> 1971:   assert(!is_concurrent_weak_root_in_progress() || active_generation() != nullptr, "Error");
> 1972:   shenandoah_assert_generations_reconciled();

Why all of this is checked here? I would have thought `gc_state` machinery should only check things related to gc-state.

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

> 132:   bool is_thread_safe() override { return true; }
> 133: };
> 134: 

This looks like belonging in one of the `*closures.hpp` header.

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

> 512: 
> 513: public:
> 514:   ShenandoahController*   control_thread() { return _control_thread; }

This method and field is probably `controller` then, right?

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

> 661: // ---------- CDS archive support
> 662: 
> 663:   bool can_load_archived_objects() const override { return !ShenandoahCardBarrier; }

This means CDS heap loading is not yet working with generational Shenandoah? This looks OK for the experimental code. Please submit a bug for this and assign it to me. I will take a look at it some time later.

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

> 752:   oop try_evacuate_object(oop src, Thread* thread, ShenandoahHeapRegion* from_region, ShenandoahAffiliation target_gen);
> 753: public:
> 754: 

I think this new line should be before `public:`

src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.cpp line 64:

> 62:     }
> 63:   }
> 64:   return _mark_bit_map.is_bitmap_clear_range(start, end);

Comprehension check: this seems to bail out the moment it discovers an uncommitted slice. Does it really happen? More worryingly, if there is a mix of committed and uncommitted chunks, this method returns `true`, even if committed chunks are not actually clean?

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

PR Review: https://git.openjdk.org/jdk/pull/21273#pullrequestreview-2450709063
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1852054187
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1852075850
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1851646208
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1851763351
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1851761345
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1851765188
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1852139996