From 13fe45653718473c2c7c97ba3880ecd881b04fd5 Mon Sep 17 00:00:00 2001 From: William Kemper Date: Tue, 19 Dec 2023 13:36:57 -0800 Subject: [PATCH 1/2] Rename gc state methods for clarity, assert that only java threads use thread local copy of gc state --- src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp | 14 +++++++------- src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp | 6 +++--- .../gc/shenandoah/shenandoahThreadLocalData.hpp | 1 + .../share/gc/shenandoah/shenandoahVMOperations.cpp | 14 +++++++------- .../share/gc/shenandoah/shenandoahVerifier.cpp | 2 +- 5 files changed, 19 insertions(+), 18 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp b/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp index acb52602735a5..12eb2aad5e803 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp @@ -1742,7 +1742,7 @@ void ShenandoahHeap::prepare_update_heap_references(bool concurrent) { _update_refs_iterator.reset(); } -void ShenandoahHeap::set_gc_state_all_threads() { +void ShenandoahHeap::propagate_gc_state_to_java_threads() { assert(ShenandoahSafepoint::is_at_shenandoah_safepoint(), "Must be at Shenandoah safepoint"); if (_gc_state_changed) { _gc_state_changed = false; @@ -1753,7 +1753,7 @@ void ShenandoahHeap::set_gc_state_all_threads() { } } -void ShenandoahHeap::set_gc_state_mask(uint mask, bool value) { +void ShenandoahHeap::set_gc_state(uint mask, bool value) { assert(ShenandoahSafepoint::is_at_shenandoah_safepoint(), "Must be at Shenandoah safepoint"); _gc_state.set_cond(mask, value); _gc_state_changed = true; @@ -1761,13 +1761,13 @@ void ShenandoahHeap::set_gc_state_mask(uint mask, bool value) { void ShenandoahHeap::set_concurrent_mark_in_progress(bool in_progress) { assert(!has_forwarded_objects(), "Not expected before/after mark phase"); - set_gc_state_mask(MARKING, in_progress); + set_gc_state(MARKING, in_progress); ShenandoahBarrierSet::satb_mark_queue_set().set_active_all_threads(in_progress, !in_progress); } void ShenandoahHeap::set_evacuation_in_progress(bool in_progress) { assert(ShenandoahSafepoint::is_at_shenandoah_safepoint(), "Only call this at safepoint"); - set_gc_state_mask(EVACUATION, in_progress); + set_gc_state(EVACUATION, in_progress); } void ShenandoahHeap::set_concurrent_strong_root_in_progress(bool in_progress) { @@ -1779,7 +1779,7 @@ void ShenandoahHeap::set_concurrent_strong_root_in_progress(bool in_progress) { } void ShenandoahHeap::set_concurrent_weak_root_in_progress(bool cond) { - set_gc_state_mask(WEAK_ROOTS, cond); + set_gc_state(WEAK_ROOTS, cond); } GCTracer* ShenandoahHeap::tracer() { @@ -1906,7 +1906,7 @@ void ShenandoahHeap::parallel_cleaning(bool full_gc) { } void ShenandoahHeap::set_has_forwarded_objects(bool cond) { - set_gc_state_mask(HAS_FORWARDED, cond); + set_gc_state(HAS_FORWARDED, cond); } void ShenandoahHeap::set_unload_classes(bool uc) { @@ -1945,7 +1945,7 @@ void ShenandoahHeap::set_full_gc_move_in_progress(bool in_progress) { } void ShenandoahHeap::set_update_refs_in_progress(bool in_progress) { - set_gc_state_mask(UPDATEREFS, in_progress); + set_gc_state(UPDATEREFS, in_progress); } void ShenandoahHeap::register_nmethod(nmethod* nm) { diff --git a/src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp b/src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp index 1873ee040e3f7..fd475fd98f301 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp @@ -293,12 +293,12 @@ class ShenandoahHeap : public CollectedHeap, public ShenandoahSpaceInfo { size_t _gc_no_progress_count; - void set_gc_state_mask(uint mask, bool value); + void set_gc_state(uint mask, bool value); public: char gc_state() const; - void set_gc_state_all_threads(); - bool has_gc_state_changed() { return _gc_state_changed; } + void propagate_gc_state_to_java_threads(); + bool has_gc_state_changed() const { return _gc_state_changed; } void set_concurrent_mark_in_progress(bool in_progress); void set_evacuation_in_progress(bool in_progress); diff --git a/src/hotspot/share/gc/shenandoah/shenandoahThreadLocalData.hpp b/src/hotspot/share/gc/shenandoah/shenandoahThreadLocalData.hpp index 422595e93135c..9eec573cc567b 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahThreadLocalData.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahThreadLocalData.hpp @@ -89,6 +89,7 @@ class ShenandoahThreadLocalData { } static char gc_state(Thread* thread) { + assert(thread->is_Java_thread(), "GC state is only synchronized to java threads"); return data(thread)->_gc_state; } diff --git a/src/hotspot/share/gc/shenandoah/shenandoahVMOperations.cpp b/src/hotspot/share/gc/shenandoah/shenandoahVMOperations.cpp index 970f99ad0f563..eeeb1dcad195a 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahVMOperations.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahVMOperations.cpp @@ -62,41 +62,41 @@ void VM_ShenandoahReferenceOperation::doit_epilogue() { void VM_ShenandoahInitMark::doit() { ShenandoahGCPauseMark mark(_gc_id, "Init Mark", SvcGCMarker::CONCURRENT); _gc->entry_init_mark(); - ShenandoahHeap::heap()->set_gc_state_all_threads(); + ShenandoahHeap::heap()->propagate_gc_state_to_java_threads(); } void VM_ShenandoahFinalMarkStartEvac::doit() { ShenandoahGCPauseMark mark(_gc_id, "Final Mark", SvcGCMarker::CONCURRENT); _gc->entry_final_mark(); - ShenandoahHeap::heap()->set_gc_state_all_threads(); + ShenandoahHeap::heap()->propagate_gc_state_to_java_threads(); } void VM_ShenandoahFullGC::doit() { ShenandoahGCPauseMark mark(_gc_id, "Full GC", SvcGCMarker::FULL); _full_gc->entry_full(_gc_cause); - ShenandoahHeap::heap()->set_gc_state_all_threads(); + ShenandoahHeap::heap()->propagate_gc_state_to_java_threads(); } void VM_ShenandoahDegeneratedGC::doit() { ShenandoahGCPauseMark mark(_gc_id, "Degenerated GC", SvcGCMarker::CONCURRENT); _gc->entry_degenerated(); - ShenandoahHeap::heap()->set_gc_state_all_threads(); + ShenandoahHeap::heap()->propagate_gc_state_to_java_threads(); } void VM_ShenandoahInitUpdateRefs::doit() { ShenandoahGCPauseMark mark(_gc_id, "Init Update Refs", SvcGCMarker::CONCURRENT); _gc->entry_init_updaterefs(); - ShenandoahHeap::heap()->set_gc_state_all_threads(); + ShenandoahHeap::heap()->propagate_gc_state_to_java_threads(); } void VM_ShenandoahFinalUpdateRefs::doit() { ShenandoahGCPauseMark mark(_gc_id, "Final Update Refs", SvcGCMarker::CONCURRENT); _gc->entry_final_updaterefs(); - ShenandoahHeap::heap()->set_gc_state_all_threads(); + ShenandoahHeap::heap()->propagate_gc_state_to_java_threads(); } void VM_ShenandoahFinalRoots::doit() { ShenandoahGCPauseMark mark(_gc_id, "Final Roots", SvcGCMarker::CONCURRENT); _gc->entry_final_roots(); - ShenandoahHeap::heap()->set_gc_state_all_threads(); + ShenandoahHeap::heap()->propagate_gc_state_to_java_threads(); } diff --git a/src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp b/src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp index 6a977c2e4a40c..f67cafdb8fe7c 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp @@ -620,7 +620,7 @@ void ShenandoahVerifier::verify_at_safepoint(const char *label, guarantee(ShenandoahSafepoint::is_at_shenandoah_safepoint(), "only when nothing else happens"); guarantee(ShenandoahVerify, "only when enabled, and bitmap is initialized in ShenandoahHeap::initialize"); - ShenandoahHeap::heap()->set_gc_state_all_threads(); + ShenandoahHeap::heap()->propagate_gc_state_to_java_threads(); // Avoid side-effect of changing workers' active thread count, but bypass concurrent/parallel protocol check ShenandoahPushWorkerScope verify_worker_scope(_heap->workers(), _heap->max_workers(), false /*bypass check*/); From 49ad345214e5e6af91669eb5ab6f9daeb4c8a6f3 Mon Sep 17 00:00:00 2001 From: William Kemper Date: Wed, 20 Dec 2023 15:48:17 -0800 Subject: [PATCH 2/2] Add comments to gc state methods --- src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp b/src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp index fd475fd98f301..deb0a972167e9 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp @@ -293,11 +293,18 @@ class ShenandoahHeap : public CollectedHeap, public ShenandoahSpaceInfo { size_t _gc_no_progress_count; + // This updates the singlular, global gc state. This must happen on a safepoint. void set_gc_state(uint mask, bool value); public: char gc_state() const; + + // This copies the global gc state into a thread local variable for java threads. + // It is primarily intended to support quick access at barriers. void propagate_gc_state_to_java_threads(); + + // This is public to support assertions that the state hasn't been changed off of + // a safepoint and that any changes were propagated to java threads after the safepoint. bool has_gc_state_changed() const { return _gc_state_changed; } void set_concurrent_mark_in_progress(bool in_progress);