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

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() &&

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();

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();

src/hotspot/share/gc/shenandoah/shenandoahFreeSet.hpp

@@ -354,6 +354,9 @@ class ShenandoahFreeSet : public CHeapObj<mtGC> {
   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<mtGC> {
 
   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);     }

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();

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.");