refactoring, fix loosing pin counts on thread exit

Author: tschatzl

src/hotspot/share/gc/g1/g1CollectedHeap.cpp

@@ -2473,7 +2473,7 @@ void G1CollectedHeap::retire_tlabs() {
 
 void G1CollectedHeap::flush_region_pin_cache() {
   for (JavaThreadIteratorWithHandle jtiwh; JavaThread *thread = jtiwh.next(); ) {
-    Pair<uint, size_t> stats = G1ThreadLocalData::get_and_reset_pinned_region_cache(thread);
+    Pair<uint, size_t> stats = G1ThreadLocalData::get_and_reset_pin_cache(thread);
     if (stats.second != 0) {
       region_at(stats.first)->add_pinned_object_count(stats.second);
     }

src/hotspot/share/gc/g1/g1CollectedHeap.hpp

@@ -775,8 +775,8 @@ class G1CollectedHeap : public CollectedHeap {
 
   void retire_tlabs();
 
-  // Update all region's pin counts from the per-thread caches. Must be called
-  // before any decision based on pin counts.
+  // Update all region's pin counts from the per-thread caches and resets them.
+  // Must be called before any decision based on pin counts.
   void flush_region_pin_cache();
 
   void expand_heap_after_young_collection();

src/hotspot/share/gc/g1/g1CollectedHeap.inline.hpp

@@ -266,36 +266,38 @@ inline void G1CollectedHeap::pin_object(JavaThread* thread, oop obj) {
   assert(obj != nullptr, "obj must not be null");
   assert(!is_gc_active(), "must not pin objects during a GC");
   assert(obj->is_typeArray(), "must be typeArray");
-
-  uint last_idx = G1ThreadLocalData::cached_pinned_region_idx(thread);
   HeapRegion* r = heap_region_containing(obj);
-  if (last_idx == r->hrm_index()) {
-    G1ThreadLocalData::inc_cached_pinned_region_count(thread);
+  uint obj_region_idx = r->hrm_index();
+
+  uint cached_idx = G1ThreadLocalData::cached_pinned_region_idx(thread);
+  if (cached_idx == obj_region_idx) {
+    G1ThreadLocalData::inc_cached_pin_count(thread);
   } else {
     // Flush old.
-    size_t pin_count = G1ThreadLocalData::cached_pinned_region_count(thread);
-    if (pin_count != 0) {
-      region_at(last_idx)->add_pinned_object_count(pin_count);
+    size_t cached_pin_count = G1ThreadLocalData::cached_pin_count(thread);
+    if (cached_pin_count != 0) {
+      region_at(cached_idx)->add_pinned_object_count(cached_pin_count);
     }
-    G1ThreadLocalData::get_and_set_pinned_region_cache(thread, r->hrm_index(), (size_t)1);
+    G1ThreadLocalData::get_and_set_pin_cache(thread, obj_region_idx, (size_t)1);
   }
 }
 
 inline void G1CollectedHeap::unpin_object(JavaThread* thread, oop obj) {
   assert(obj != nullptr, "obj must not be null");
   assert(!is_gc_active(), "must not unpin objects during a GC");
   HeapRegion* r = heap_region_containing(obj);
+  uint obj_region_idx = r->hrm_index();
 
-  uint last_idx = G1ThreadLocalData::cached_pinned_region_idx(thread);
-  if (last_idx == r->hrm_index()) {
-    G1ThreadLocalData::dec_cached_pinned_region_count(thread);
+  uint cached_idx = G1ThreadLocalData::cached_pinned_region_idx(thread);
+  if (cached_idx == obj_region_idx) {
+    G1ThreadLocalData::dec_cached_pin_count(thread);
   } else {
     // Flush old.
-    size_t pin_count = G1ThreadLocalData::cached_pinned_region_count(thread);
-    if (pin_count != 0) {
-      region_at(last_idx)->add_pinned_object_count(pin_count);
+    size_t cached_pin_count = G1ThreadLocalData::cached_pin_count(thread);
+    if (cached_pin_count != 0) {
+      region_at(cached_idx)->add_pinned_object_count(cached_pin_count);
     }
-    G1ThreadLocalData::get_and_set_pinned_region_cache(thread, r->hrm_index(), ~(size_t)0);
+    G1ThreadLocalData::get_and_set_pin_cache(thread, obj_region_idx, ~(size_t)0);
   }
 }
 

src/hotspot/share/gc/g1/g1RegionPinCache.cpp

@@ -0,0 +1,35 @@
+/*
+ * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ *
+ */
+
+#include "precompiled.hpp"
+
+#include "gc/g1/g1RegionPinCache.hpp"
+#include "gc/g1/g1CollectedHeap.hpp"
+
+G1RegionPinCache::~G1RegionPinCache() {
+  if (_count != 0) {
+    G1CollectedHeap::heap()->region_at(_region_idx)->add_pinned_object_count(_count);
+  }
+  get_and_reset();
+}

src/hotspot/share/gc/g1/g1RegionPinCache.hpp

@@ -0,0 +1,59 @@
+/*
+ * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ *
+ */
+
+#ifndef SHARE_GC_G1_G1REGIONPINCACHE_HPP
+#define SHARE_GC_G1_G1REGIONPINCACHE_HPP
+
+#include "memory/allocation.hpp"
+#include "utilities/pair.hpp"
+#include "utilities/globalDefinitions.hpp"
+
+class G1RegionPinCache : public StackObj {
+  uint _region_idx;
+  size_t _count;
+
+public:
+  G1RegionPinCache() : _region_idx(0), _count(0) { }
+  ~G1RegionPinCache();
+
+  uint region_idx() const { return _region_idx; }
+  size_t count() const { return _count; }
+
+  void inc_count() { ++_count; }
+  void dec_count() { --_count; }
+
+  size_t get_and_set(uint new_region_idx, size_t new_count) {
+    size_t result = _count;
+    _region_idx = new_region_idx;
+    _count = new_count;
+    return result;
+  }
+
+  Pair<uint, size_t> get_and_reset() {
+    Pair<uint, size_t> result(_region_idx, get_and_set(0, 0));
+    return result;
+  }
+};
+
+#endif /* SHARE_GC_G1_G1REGIONPINCACHE_HPP */

src/hotspot/share/gc/g1/g1ThreadLocalData.hpp

@@ -26,6 +26,7 @@
 
 #include "gc/g1/g1BarrierSet.hpp"
 #include "gc/g1/g1DirtyCardQueue.hpp"
+#include "gc/g1/g1RegionPinCache.hpp"
 #include "gc/shared/gc_globals.hpp"
 #include "gc/shared/satbMarkQueue.hpp"
 #include "runtime/javaThread.hpp"
@@ -37,14 +38,12 @@ class G1ThreadLocalData {
   SATBMarkQueue _satb_mark_queue;
   G1DirtyCardQueue _dirty_card_queue;
 
-  uint _cached_pinned_region_idx;
-  size_t _cached_pinned_region_count;
+  G1RegionPinCache _pin_cache;
 
   G1ThreadLocalData() :
       _satb_mark_queue(&G1BarrierSet::satb_mark_queue_set()),
       _dirty_card_queue(&G1BarrierSet::dirty_card_queue_set()),
-      _cached_pinned_region_idx(0),
-      _cached_pinned_region_count(0) {}
+      _pin_cache() {}
 
   static G1ThreadLocalData* data(Thread* thread) {
     assert(UseG1GC, "Sanity");
@@ -97,32 +96,27 @@ class G1ThreadLocalData {
   }
 
   static uint cached_pinned_region_idx(Thread* thread) {
-    return data(thread)->_cached_pinned_region_idx;
+    return data(thread)->_pin_cache.region_idx();
   }
 
-  static size_t cached_pinned_region_count(Thread* thread) {
-    return data(thread)->_cached_pinned_region_count;
+  static size_t cached_pin_count(Thread* thread) {
+    return data(thread)->_pin_cache.count();
   }
 
-  static void inc_cached_pinned_region_count(Thread* thread) {
-    ++data(thread)->_cached_pinned_region_count;
+  static void inc_cached_pin_count(Thread* thread) {
+    data(thread)->_pin_cache.inc_count();
   }
 
-  static void dec_cached_pinned_region_count(Thread* thread) {
-    --data(thread)->_cached_pinned_region_count;
+  static void dec_cached_pin_count(Thread* thread) {
+    data(thread)->_pin_cache.dec_count();
   }
 
-  static size_t get_and_set_pinned_region_cache(Thread* thread, uint region_idx, size_t new_count) {
-    G1ThreadLocalData* d = data(thread);
-    size_t result = d->_cached_pinned_region_count;
-    d->_cached_pinned_region_idx = region_idx;
-    d->_cached_pinned_region_count = new_count;
-    return result;
+  static size_t get_and_set_pin_cache(Thread* thread, uint region_idx, size_t new_count) {
+    return data(thread)->_pin_cache.get_and_set(region_idx, new_count);
   }
 
-  static Pair<uint, size_t> get_and_reset_pinned_region_cache(Thread* thread) {
-    Pair<uint, size_t> result { data(thread)->cached_pinned_region_idx(thread), get_and_set_pinned_region_cache(thread, 0, 0) };
-    return result;
+  static Pair<uint, size_t> get_and_reset_pin_cache(Thread* thread) {
+    return data(thread)->_pin_cache.get_and_reset();
   }
 };
 

src/hotspot/share/gc/g1/g1YoungGCPreEvacuateTasks.cpp

@@ -66,7 +66,7 @@ class G1PreEvacuateCollectionSetBatchTask::JavaThreadRetireTLABAndFlushLogs : pu
       G1DirtyCardQueueSet& qset = G1BarrierSet::dirty_card_queue_set();
       _refinement_stats += qset.concatenate_log_and_stats(thread);
       // Flush region pincount cache.
-      Pair<uint, size_t> region_pin_cache = G1ThreadLocalData::get_and_reset_pinned_region_cache(thread);
+      Pair<uint, size_t> region_pin_cache = G1ThreadLocalData::get_and_reset_pin_cache(thread);
       if (region_pin_cache.second != 0) {
         G1CollectedHeap::heap()->region_at(region_pin_cache.first)->add_pinned_object_count(region_pin_cache.second);
       }
@@ -140,7 +140,7 @@ class G1PreEvacuateCollectionSetBatchTask::NonJavaThreadFlushLogs : public G1Abs
       G1DirtyCardQueueSet& qset = G1BarrierSet::dirty_card_queue_set();
       _refinement_stats += qset.concatenate_log_and_stats(thread);
 
-      assert(G1ThreadLocalData::cached_pinned_region_count(thread) == 0, "NonJava thread pinned Java objects");
+      assert(G1ThreadLocalData::cached_pin_count(thread) == 0, "NonJava thread pinned Java objects");
     }
   } _tc;
 

src/hotspot/share/gc/g1/heapRegion.inline.hpp

@@ -554,6 +554,7 @@ inline void HeapRegion::record_surv_words_in_group(size_t words_survived) {
 }
 
 inline void HeapRegion::add_pinned_object_count(size_t value) {
+  assert(value != 0, "wasted effort");
   assert(!is_free(), "trying to pin free region %u, adding %zu", hrm_index(), value);
   Atomic::add(&_pinned_object_count, value, memory_order_relaxed);
   if (UseNewCode) {