RFR: 8369068: GenShen: Generations still aren't reconciled assertion failure [v2]
Y. Srinivas Ramakrishna
ysr at openjdk.org
Wed Oct 22 01:53:16 UTC 2025
On Tue, 14 Oct 2025 23:03:16 GMT, William Kemper <wkemper at openjdk.org> wrote:
>> There are certain code paths in Shenandoah's generational mode that need to _know_ which generation is being collected, but it is not possible to pass this information on the stack (barriers, for example). To address this, we introduced an `_active_generation` member in `ShenandoahHeap`. Over time, the usage of this field grew beyond its intended purpose and we began to have issues where not all threads would see a consistent value for this field. To address _this_ issue, we added another field `_gc_generation` which was only meant to be used by GC threads. At this point, we have three different ways to determine which generation is being collected: the _active_ generation, the _gc_ generation, and the usual function parameters and member fields of the gc components. This PR removes `_gc_generation` and reduces the use of `_active_generation` to only those places where it is not possible to get this information from other means (barriers, mostly). All GC components that can have t
his information passed through function calls, now do so.
>
> William Kemper has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains nine commits:
>
> - Merge remote-tracking branch 'jdk/master' into reduce-gc_generation-usage
> - Merge remote-tracking branch 'jdk/master' into reduce-gc_generation-usage
> - Remove _gc_generation from ShenandoahHeap
> - Little cleanup, remove one active generation usage
> - Merge remote-tracking branch 'jdk/master' into reduce-gc_generation-usage
> - Finish removing usages of gc_generation, start on reducing usages of active_generation
> - Fix build
> - Use existing _generation field instead of Heap::_gc_generation where possible
> - Only shenandoah vm operations participate in active/gc generation scheme
Left a few minor suggestions. This is a very nice clean-up, thank you!
src/hotspot/share/gc/shenandoah/shenandoahFullGC.cpp line 358:
> 356: assert(_from_region != nullptr, "must set before work");
> 357: assert(_heap->marking_context()->is_marked(p), "must be marked");
> 358: assert(!_heap->marking_context()->allocated_after_mark_start(p), "must be truly marked");
Why not `_generation->complete_marking_context()...` with the help of a `_generation` field kept in the object? It would make the idiom uniform and consistent across all these closures.
src/hotspot/share/gc/shenandoah/shenandoahFullGC.cpp line 802:
> 800: }
> 801: void do_object(oop p) {
> 802: assert(_heap->marking_context()->is_marked(p), "must be marked");
Just use `_ctx` from line 784?
src/hotspot/share/gc/shenandoah/shenandoahFullGC.cpp line 886:
> 884:
> 885: void do_object(oop p) {
> 886: assert(_heap->marking_context()->is_marked(p), "must be marked");
ditto (comment at line 802).
src/hotspot/share/gc/shenandoah/shenandoahGenerationalEvacuationTask.cpp line 174:
> 172: // contained herein.
> 173: void ShenandoahGenerationalEvacuationTask::promote_in_place(ShenandoahHeapRegion* region) {
> 174: assert(!_generation->is_old(), "Sanity check");
Would `assert(_generation->is_young()...` be too strong at line 174 for some reason? e.g. could `_generation` be `is_global()` here?
You could then confidently replace the `_heap->young_generation()` indirection at line 175 below with just `_generation`, also simplifying comprehension complexity & code maintainability.
src/hotspot/share/gc/shenandoah/shenandoahGenerationalFullGC.cpp line 56:
> 54: auto heap = ShenandoahGenerationalHeap::heap();
> 55: // Since we may arrive here from degenerated GC failure of either young or old, establish generation as GLOBAL.
> 56: heap->set_active_generation(heap->global_generation());
While this is good practice for consistency, is `_active_generation` ever used by the worker threads when doing a full gc? Not asking to remove this line, just wondering if it's ever used now, following your changes in this PR.
src/hotspot/share/gc/shenandoah/shenandoahGenerationalHeap.cpp line 851:
> 849: }
> 850:
> 851: if (!_generation->is_global()) {
Is this ever `_generation->is_old()` ? I am guessing this is either global or young? I am guessing for the case of update_refs for a mixed collection we still pass in the `young` gen here?
It might be a good idea wherever we have a `_generation` field member in a class to indicate what it represents. In most cases, it's probably clear that it represents the generation subject to collection in that cycle. But for mixed collection sets, what does the `_generation` field represent?
src/hotspot/share/gc/shenandoah/shenandoahGenerationalHeap.hpp line 92:
> 90: oop try_evacuate_object(oop p, Thread* thread, ShenandoahHeapRegion* from_region, ShenandoahAffiliation target_gen);
> 91: void evacuate_collection_set(ShenandoahGeneration* generation, bool concurrent) override;
> 92: void promote_regions_in_place(ShenandoahGeneration* generation, bool concurrent);
Document what the parameters to the call represent, in particular `generation`.
src/hotspot/share/gc/shenandoah/shenandoahGenerationalHeap.hpp line 102:
> 100: // ---------- Update References
> 101: //
> 102: void update_heap_references(ShenandoahGeneration* generation, bool concurrent) override;
Ditto (to comment at line 92).
src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 149:
> 147:
> 148: // This is set and cleared by only the VMThread
> 149: // at each STW pause (safepoint) to the value given to the VM operation.
Very nice simplification which cleans up a lot of the old messiness. Thanks!
src/hotspot/share/gc/shenandoah/shenandoahMark.cpp line 82:
> 80: template<bool CANCELLABLE, StringDedupMode STRING_DEDUP>
> 81: void ShenandoahMark::mark_loop(uint worker_id, TaskTerminator* terminator,
> 82: ShenandoahGenerationType generation, StringDedup::Requests* const req) {
Since `ShenandoahMark` has a `_generation` data member with a `generation()` accessor, I'd rename the formal parameters of type `ShenandoahGenerationType` for all these methods from `generation` to `gen_type` or something like that to reduce confusion while reading the code.
src/hotspot/share/gc/shenandoah/shenandoahMark.cpp line 104:
> 102: }
> 103:
> 104: void ShenandoahMark::mark_loop(uint worker_id, TaskTerminator* terminator, ShenandoahGenerationType generation,
parm name comment at line 82, applies below as well.
src/hotspot/share/gc/shenandoah/shenandoahReferenceProcessor.cpp line 1:
> 1: /*
Some of the spots here on where we need a completed marking context and where we are permitted to use a possibly in flight marking context isn't terribly clear. At some point, we should probably go through and clarify.
src/hotspot/share/gc/shenandoah/shenandoahRootVerifier.hpp line 47:
> 45: // Used to seed ShenandoahVerifier, do not honor root type filter
> 46: static void roots_do(OopIterateClosure* cl, ShenandoahGeneration* generation);
> 47: static void strong_roots_do(OopIterateClosure* cl, ShenandoahGeneration* generation);
Document what the parameter `generation` represents here.
src/hotspot/share/gc/shenandoah/shenandoahSTWMark.cpp line 152:
> 150: StringDedup::Requests requests;
> 151:
> 152: // TODO: Why are we passing our own fields to our own method?
It's a method on our parent class which doesn't have a Terminator object handy.
src/hotspot/share/gc/shenandoah/shenandoahVMOperations.hpp line 55:
> 53: explicit VM_ShenandoahOperation(ShenandoahGeneration* generation)
> 54: : _gc_id(GCId::current())
> 55: , _generation(generation) {
style question: shouldn't the comma go at the end of the previous line?
-------------
Marked as reviewed by ysr (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/27703#pullrequestreview-3352384744
PR Review Comment: https://git.openjdk.org/jdk/pull/27703#discussion_r2450040309
PR Review Comment: https://git.openjdk.org/jdk/pull/27703#discussion_r2450050185
PR Review Comment: https://git.openjdk.org/jdk/pull/27703#discussion_r2450050669
PR Review Comment: https://git.openjdk.org/jdk/pull/27703#discussion_r2441449826
PR Review Comment: https://git.openjdk.org/jdk/pull/27703#discussion_r2450058990
PR Review Comment: https://git.openjdk.org/jdk/pull/27703#discussion_r2450082830
PR Review Comment: https://git.openjdk.org/jdk/pull/27703#discussion_r2450086356
PR Review Comment: https://git.openjdk.org/jdk/pull/27703#discussion_r2450086707
PR Review Comment: https://git.openjdk.org/jdk/pull/27703#discussion_r2441455419
PR Review Comment: https://git.openjdk.org/jdk/pull/27703#discussion_r2450129501
PR Review Comment: https://git.openjdk.org/jdk/pull/27703#discussion_r2450130541
PR Review Comment: https://git.openjdk.org/jdk/pull/27703#discussion_r2450145164
PR Review Comment: https://git.openjdk.org/jdk/pull/27703#discussion_r2450147249
PR Review Comment: https://git.openjdk.org/jdk/pull/27703#discussion_r2450159840
PR Review Comment: https://git.openjdk.org/jdk/pull/27703#discussion_r2441471022
More information about the shenandoah-dev
mailing list