8261838: Shenandoah: reconsider heap region iterators memory ordering

Reviewed-by: rkennke

Author: shipilev

src/hotspot/share/gc/shenandoah/shenandoahCollectionSet.cpp

@@ -110,27 +110,25 @@ void ShenandoahCollectionSet::clear() {
 }
 
 ShenandoahHeapRegion* ShenandoahCollectionSet::claim_next() {
-  size_t num_regions = _heap->num_regions();
-  if (_current_index >= (jint)num_regions) {
-    return NULL;
-  }
+  // This code is optimized for the case when collection set contains only
+  // a few regions. In this case, it is more constructive to check for is_in
+  // before hitting the (potentially contended) atomic index.
 
-  jint saved_current = _current_index;
-  size_t index = (size_t)saved_current;
+  size_t max = _heap->num_regions();
+  size_t old = Atomic::load(&_current_index);
 
-  while(index < num_regions) {
+  for (size_t index = old; index < max; index++) {
     if (is_in(index)) {
-      jint cur = Atomic::cmpxchg(&_current_index, saved_current, (jint)(index + 1));
-      assert(cur >= (jint)saved_current, "Must move forward");
-      if (cur == saved_current) {
-        assert(is_in(index), "Invariant");
+      size_t cur = Atomic::cmpxchg(&_current_index, old, index + 1, memory_order_relaxed);
+      assert(cur >= old, "Always move forward");
+      if (cur == old) {
+        // Successfully moved the claim index, this is our region.
         return _heap->get_region(index);
       } else {
-        index = (size_t)cur;
-        saved_current = cur;
+        // Somebody else moved the claim index, restart from there.
+        index = cur - 1; // adjust for loop post-increment
+        old = cur;
       }
-    } else {
-      index ++;
     }
   }
   return NULL;
@@ -139,10 +137,11 @@ ShenandoahHeapRegion* ShenandoahCollectionSet::claim_next() {
 ShenandoahHeapRegion* ShenandoahCollectionSet::next() {
   assert(ShenandoahSafepoint::is_at_shenandoah_safepoint(), "Must be at a safepoint");
   assert(Thread::current()->is_VM_thread(), "Must be VMThread");
-  size_t num_regions = _heap->num_regions();
-  for (size_t index = (size_t)_current_index; index < num_regions; index ++) {
+
+  size_t max = _heap->num_regions();
+  for (size_t index = _current_index; index < max; index++) {
     if (is_in(index)) {
-      _current_index = (jint)(index + 1);
+      _current_index = index + 1;
       return _heap->get_region(index);
     }
   }

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

@@ -47,7 +47,7 @@ class ShenandoahCollectionSet : public CHeapObj<mtGC> {
   size_t                _region_count;
 
   shenandoah_padding(0);
-  volatile jint         _current_index;
+  volatile size_t       _current_index;
   shenandoah_padding(1);
 
 public:

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp

@@ -1497,8 +1497,8 @@ class ShenandoahParallelHeapRegionTask : public AbstractGangTask {
     size_t stride = ShenandoahParallelRegionStride;
 
     size_t max = _heap->num_regions();
-    while (_index < max) {
-      size_t cur = Atomic::fetch_and_add(&_index, stride);
+    while (Atomic::load(&_index) < max) {
+      size_t cur = Atomic::fetch_and_add(&_index, stride, memory_order_relaxed);
       size_t start = cur;
       size_t end = MIN2(cur + stride, max);
       if (start >= max) break;

src/hotspot/share/gc/shenandoah/shenandoahHeap.inline.hpp

@@ -54,7 +54,7 @@ inline ShenandoahHeap* ShenandoahHeap::heap() {
 }
 
 inline ShenandoahHeapRegion* ShenandoahRegionIterator::next() {
-  size_t new_index = Atomic::add(&_index, (size_t) 1);
+  size_t new_index = Atomic::add(&_index, (size_t) 1, memory_order_relaxed);
   // get_region() provides the bounds-check and returns NULL on OOB.
   return _heap->get_region(new_index - 1);
 }