From 8562abbe27c11fd3a5aa5ea874c318e2555e9242 Mon Sep 17 00:00:00 2001 From: William Kemper Date: Mon, 7 Oct 2024 20:16:22 +0000 Subject: [PATCH] 8334147: Shenandoah: Avoid taking lock for disabled free set logging 8341554: Shenandoah: Missing heap lock when updating usage for soft ref policy Reviewed-by: ysr, kdnilsen Backport-of: c47a0e005e54551e42ee1ae33d7169417a5f86d4 --- .../gc/shenandoah/shenandoahConcurrentGC.cpp | 7 +------ .../gc/shenandoah/shenandoahControlThread.cpp | 17 +++++++---------- .../share/gc/shenandoah/shenandoahFreeSet.cpp | 9 +++++++++ .../share/gc/shenandoah/shenandoahFreeSet.hpp | 6 +++++- .../shenandoahGenerationalControlThread.cpp | 18 ++++++++---------- .../share/gc/shenandoah/shenandoahOldGC.cpp | 5 +---- 6 files changed, 31 insertions(+), 31 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp b/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp index ec8f4a2d74f..ebfa1d3db4f 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp @@ -181,12 +181,7 @@ bool ShenandoahConcurrentGC::collect(GCCause::Cause cause) { // we will not age young-gen objects in the case that we skip evacuation. entry_cleanup_early(); - { - // TODO: Not sure there is value in logging free-set status right here. Note that whenever the free set is rebuilt, - // it logs the newly rebuilt status. - ShenandoahHeapLocker locker(heap->lock()); - heap->free_set()->log_status(); - } + heap->free_set()->log_status_under_lock(); // Perform concurrent class unloading if (heap->unload_classes() && diff --git a/src/hotspot/share/gc/shenandoah/shenandoahControlThread.cpp b/src/hotspot/share/gc/shenandoah/shenandoahControlThread.cpp index 5bad64d68eb..f7dc7cc88a3 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahControlThread.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahControlThread.cpp @@ -148,10 +148,7 @@ void ShenandoahControlThread::run_service() { heap->set_forced_counters_update(true); // If GC was requested, we better dump freeset data for performance debugging - { - ShenandoahHeapLocker locker(heap->lock()); - heap->free_set()->log_status(); - } + heap->free_set()->log_status_under_lock(); switch (mode) { case concurrent_normal: @@ -179,19 +176,19 @@ void ShenandoahControlThread::run_service() { // Report current free set state at the end of cycle, whether // it is a normal completion, or the abort. - { - ShenandoahHeapLocker locker(heap->lock()); - heap->free_set()->log_status(); + heap->free_set()->log_status_under_lock(); + { // Notify Universe about new heap usage. This has implications for // global soft refs policy, and we better report it every time heap // usage goes down. + ShenandoahHeapLocker locker(heap->lock()); heap->update_capacity_and_used_at_gc(); - - // Signal that we have completed a visit to all live objects. - heap->record_whole_heap_examined_timestamp(); } + // Signal that we have completed a visit to all live objects. + heap->record_whole_heap_examined_timestamp(); + // Disable forced counters update, and update counters one more time // to capture the state at the end of GC session. heap->handle_force_counters_update(); diff --git a/src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp b/src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp index 1ac983be285..c535b754ed8 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp @@ -1763,6 +1763,15 @@ void ShenandoahFreeSet::establish_old_collector_alloc_bias() { (available_in_second_half > available_in_first_half)); } +void ShenandoahFreeSet::log_status_under_lock() { + // Must not be heap locked, it acquires heap lock only when log is enabled + shenandoah_assert_not_heaplocked(); + if (LogTarget(Info, gc, free)::is_enabled() + DEBUG_ONLY(|| LogTarget(Debug, gc, free)::is_enabled())) { + ShenandoahHeapLocker locker(_heap->lock()); + log_status(); + } +} void ShenandoahFreeSet::log_status() { shenandoah_assert_heaplocked(); diff --git a/src/hotspot/share/gc/shenandoah/shenandoahFreeSet.hpp b/src/hotspot/share/gc/shenandoah/shenandoahFreeSet.hpp index 5f05f402054..f740e86ac3b 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahFreeSet.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahFreeSet.hpp @@ -354,6 +354,9 @@ class ShenandoahFreeSet : public CHeapObj { void establish_generation_sizes(size_t young_region_count, size_t old_region_count); size_t get_usable_free_words(size_t free_bytes) const; + // log status, assuming lock has already been acquired by the caller. + void log_status(); + public: ShenandoahFreeSet(ShenandoahHeap* heap, size_t max_regions); @@ -408,7 +411,8 @@ class ShenandoahFreeSet : public CHeapObj { void recycle_trash(); - void log_status(); + // Acquire heap lock and log status, assuming heap lock is not acquired by the caller. + void log_status_under_lock(); inline size_t capacity() const { return _partitions.capacity_of(ShenandoahFreeSetPartitionId::Mutator); } inline size_t used() const { return _partitions.used_by(ShenandoahFreeSetPartitionId::Mutator); } diff --git a/src/hotspot/share/gc/shenandoah/shenandoahGenerationalControlThread.cpp b/src/hotspot/share/gc/shenandoah/shenandoahGenerationalControlThread.cpp index 949af4cc1a6..babd7a7d6e1 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahGenerationalControlThread.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahGenerationalControlThread.cpp @@ -208,10 +208,8 @@ void ShenandoahGenerationalControlThread::run_service() { heap->set_forced_counters_update(true); // If GC was requested, we better dump freeset data for performance debugging - { - ShenandoahHeapLocker locker(heap->lock()); - heap->free_set()->log_status(); - } + heap->free_set()->log_status_under_lock(); + // In case this is a degenerated cycle, remember whether original cycle was aging. const bool was_aging_cycle = heap->is_aging_cycle(); heap->set_aging_cycle(false); @@ -265,19 +263,19 @@ void ShenandoahGenerationalControlThread::run_service() { // Report current free set state at the end of cycle, whether // it is a normal completion, or the abort. - { - ShenandoahHeapLocker locker(heap->lock()); - heap->free_set()->log_status(); + heap->free_set()->log_status_under_lock(); + { // Notify Universe about new heap usage. This has implications for // global soft refs policy, and we better report it every time heap // usage goes down. + ShenandoahHeapLocker locker(heap->lock()); heap->update_capacity_and_used_at_gc(); - - // Signal that we have completed a visit to all live objects. - heap->record_whole_heap_examined_timestamp(); } + // Signal that we have completed a visit to all live objects. + heap->record_whole_heap_examined_timestamp(); + // Disable forced counters update, and update counters one more time // to capture the state at the end of GC session. heap->handle_force_counters_update(); diff --git a/src/hotspot/share/gc/shenandoah/shenandoahOldGC.cpp b/src/hotspot/share/gc/shenandoah/shenandoahOldGC.cpp index 24cd41d84c6..46e930b5359 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahOldGC.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahOldGC.cpp @@ -131,10 +131,7 @@ bool ShenandoahOldGC::collect(GCCause::Cause cause) { // the space. This would be the last action if there is nothing to evacuate. entry_cleanup_early(); - { - ShenandoahHeapLocker locker(heap->lock()); - heap->free_set()->log_status(); - } + heap->free_set()->log_status_under_lock(); assert(!heap->is_concurrent_strong_root_in_progress(), "No evacuations during old gc.");