8263107: PSPromotionManager::copy_and_push_safe_barrier needs acquire memory barrier

Reviewed-by: iwalulya, tschatzl, mdoerr

src/hotspot/share/gc/parallel/psClosure.inline.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2018, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2018, 2021, 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
@@ -90,13 +90,8 @@ public:
     if (PSScavenge::should_scavenge(p)) {
       assert(PSScavenge::should_scavenge(p, true), "revisiting object?");
 
-      oop o = *p;
+      oop o = RawAccess<IS_NOT_NULL>::oop_load(p);
-      oop new_obj;
+      oop new_obj = _pm->copy_to_survivor_space</*promote_immediately=*/false>(o);
-      if (o->is_forwarded()) {
-        new_obj = o->forwardee();
-      } else {
-        new_obj = _pm->copy_to_survivor_space</*promote_immediately=*/false>(o);
-      }
       RawAccess<IS_NOT_NULL>::oop_store(p, new_obj);
 
       if (PSScavenge::is_obj_in_young(new_obj)) {

src/hotspot/share/gc/parallel/psPromotionManager.hpp

@@ -110,6 +110,9 @@ class PSPromotionManager {
 
   static PSScannerTasksQueueSet* stack_array_depth() { return _stack_array_depth; }
 
+  template<bool promote_immediately>
+  oop copy_unmarked_to_survivor_space(oop o, markWord m);
+
  public:
   // Static
   static void initialize();

src/hotspot/share/gc/parallel/psPromotionManager.inline.hpp

@@ -38,6 +38,7 @@
 #include "memory/iterator.inline.hpp"
 #include "oops/access.inline.hpp"
 #include "oops/oop.inline.hpp"
+#include "runtime/orderAccess.hpp"
 #include "runtime/prefetch.inline.hpp"
 
 inline PSPromotionManager* PSPromotionManager::manager_array(uint index) {
@@ -126,175 +127,186 @@ inline void PSPromotionManager::push_contents(oop obj) {
     obj->oop_iterate_backwards(&pcc);
   }
 }
+
+template<bool promote_immediately>
+inline oop PSPromotionManager::copy_to_survivor_space(oop o) {
+  assert(should_scavenge(&o), "Sanity");
+
+  // NOTE! We must be very careful with any methods that access the mark
+  // in o. There may be multiple threads racing on it, and it may be forwarded
+  // at any time.
+  markWord m = o->mark();
+  if (!m.is_marked()) {
+    return copy_unmarked_to_survivor_space<promote_immediately>(o, m);
+  } else {
+    // Ensure any loads from the forwardee follow all changes that precede
+    // the release-cmpxchg that performed the forwarding, possibly in some
+    // other thread.
+    OrderAccess::acquire();
+    // Return the already installed forwardee.
+    return cast_to_oop(m.decode_pointer());
+  }
+}
+
 //
 // This method is pretty bulky. It would be nice to split it up
 // into smaller submethods, but we need to be careful not to hurt
 // performance.
 //
 template<bool promote_immediately>
-inline oop PSPromotionManager::copy_to_survivor_space(oop o) {
+inline oop PSPromotionManager::copy_unmarked_to_survivor_space(oop o,
+                                                               markWord test_mark) {
   assert(should_scavenge(&o), "Sanity");
 
   oop new_obj = NULL;
+  bool new_obj_is_tenured = false;
+  size_t new_obj_size = o->size();
 
-  // NOTE! We must be very careful with any methods that access the mark
+  // Find the objects age, MT safe.
-  // in o. There may be multiple threads racing on it, and it may be forwarded
+  uint age = (test_mark.has_displaced_mark_helper() /* o->has_displaced_mark() */) ?
-  // at any time. Do not use oop methods for accessing the mark!
-  markWord test_mark = o->mark();
-
-  // The same test as "o->is_forwarded()"
-  if (!test_mark.is_marked()) {
-    bool new_obj_is_tenured = false;
-    size_t new_obj_size = o->size();
-
-    // Find the objects age, MT safe.
-    uint age = (test_mark.has_displaced_mark_helper() /* o->has_displaced_mark() */) ?
       test_mark.displaced_mark_helper().age() : test_mark.age();
 
-    if (!promote_immediately) {
+  if (!promote_immediately) {
-      // Try allocating obj in to-space (unless too old)
+    // Try allocating obj in to-space (unless too old)
-      if (age < PSScavenge::tenuring_threshold()) {
+    if (age < PSScavenge::tenuring_threshold()) {
-        new_obj = cast_to_oop(_young_lab.allocate(new_obj_size));
+      new_obj = cast_to_oop(_young_lab.allocate(new_obj_size));
-        if (new_obj == NULL && !_young_gen_is_full) {
+      if (new_obj == NULL && !_young_gen_is_full) {
-          // Do we allocate directly, or flush and refill?
+        // Do we allocate directly, or flush and refill?
-          if (new_obj_size > (YoungPLABSize / 2)) {
+        if (new_obj_size > (YoungPLABSize / 2)) {
-            // Allocate this object directly
+          // Allocate this object directly
-            new_obj = cast_to_oop(young_space()->cas_allocate(new_obj_size));
+          new_obj = cast_to_oop(young_space()->cas_allocate(new_obj_size));
-            promotion_trace_event(new_obj, o, new_obj_size, age, false, NULL);
+          promotion_trace_event(new_obj, o, new_obj_size, age, false, NULL);
-          } else {
+        } else {
-            // Flush and fill
+          // Flush and fill
-            _young_lab.flush();
+          _young_lab.flush();
 
-            HeapWord* lab_base = young_space()->cas_allocate(YoungPLABSize);
+          HeapWord* lab_base = young_space()->cas_allocate(YoungPLABSize);
-            if (lab_base != NULL) {
+          if (lab_base != NULL) {
-              _young_lab.initialize(MemRegion(lab_base, YoungPLABSize));
+            _young_lab.initialize(MemRegion(lab_base, YoungPLABSize));
-              // Try the young lab allocation again.
+            // Try the young lab allocation again.
-              new_obj = cast_to_oop(_young_lab.allocate(new_obj_size));
+            new_obj = cast_to_oop(_young_lab.allocate(new_obj_size));
-              promotion_trace_event(new_obj, o, new_obj_size, age, false, &_young_lab);
+            promotion_trace_event(new_obj, o, new_obj_size, age, false, &_young_lab);
-            } else {
+          } else {
-              _young_gen_is_full = true;
+            _young_gen_is_full = true;
-            }
           }
         }
       }
     }
-
-    // Otherwise try allocating obj tenured
-    if (new_obj == NULL) {
-#ifndef PRODUCT
-      if (ParallelScavengeHeap::heap()->promotion_should_fail()) {
-        return oop_promotion_failed(o, test_mark);
-      }
-#endif  // #ifndef PRODUCT
-
-      new_obj = cast_to_oop(_old_lab.allocate(new_obj_size));
-      new_obj_is_tenured = true;
-
-      if (new_obj == NULL) {
-        if (!_old_gen_is_full) {
-          // Do we allocate directly, or flush and refill?
-          if (new_obj_size > (OldPLABSize / 2)) {
-            // Allocate this object directly
-            new_obj = cast_to_oop(old_gen()->allocate(new_obj_size));
-            promotion_trace_event(new_obj, o, new_obj_size, age, true, NULL);
-          } else {
-            // Flush and fill
-            _old_lab.flush();
-
-            HeapWord* lab_base = old_gen()->allocate(OldPLABSize);
-            if(lab_base != NULL) {
-#ifdef ASSERT
-              // Delay the initialization of the promotion lab (plab).
-              // This exposes uninitialized plabs to card table processing.
-              if (GCWorkerDelayMillis > 0) {
-                os::naked_sleep(GCWorkerDelayMillis);
-              }
-#endif
-              _old_lab.initialize(MemRegion(lab_base, OldPLABSize));
-              // Try the old lab allocation again.
-              new_obj = cast_to_oop(_old_lab.allocate(new_obj_size));
-              promotion_trace_event(new_obj, o, new_obj_size, age, true, &_old_lab);
-            }
-          }
-        }
-
-        // This is the promotion failed test, and code handling.
-        // The code belongs here for two reasons. It is slightly
-        // different than the code below, and cannot share the
-        // CAS testing code. Keeping the code here also minimizes
-        // the impact on the common case fast path code.
-
-        if (new_obj == NULL) {
-          _old_gen_is_full = true;
-          return oop_promotion_failed(o, test_mark);
-        }
-      }
-    }
-
-    assert(new_obj != NULL, "allocation should have succeeded");
-
-    // Copy obj
-    Copy::aligned_disjoint_words(cast_from_oop<HeapWord*>(o), cast_from_oop<HeapWord*>(new_obj), new_obj_size);
-
-    // Now we have to CAS in the header.
-    // Make copy visible to threads reading the forwardee.
-    if (o->cas_forward_to(new_obj, test_mark, memory_order_release)) {
-      // We won any races, we "own" this object.
-      assert(new_obj == o->forwardee(), "Sanity");
-
-      // Increment age if obj still in new generation. Now that
-      // we're dealing with a markWord that cannot change, it is
-      // okay to use the non mt safe oop methods.
-      if (!new_obj_is_tenured) {
-        new_obj->incr_age();
-        assert(young_space()->contains(new_obj), "Attempt to push non-promoted obj");
-      }
-
-      // Do the size comparison first with new_obj_size, which we
-      // already have. Hopefully, only a few objects are larger than
-      // _min_array_size_for_chunking, and most of them will be arrays.
-      // So, the is->objArray() test would be very infrequent.
-      if (new_obj_size > _min_array_size_for_chunking &&
-          new_obj->is_objArray() &&
-          PSChunkLargeArrays) {
-        // we'll chunk it
-        push_depth(ScannerTask(PartialArrayScanTask(o)));
-        TASKQUEUE_STATS_ONLY(++_arrays_chunked; ++_array_chunk_pushes);
-      } else {
-        // we'll just push its contents
-        push_contents(new_obj);
-      }
-    }  else {
-      // We lost, someone else "owns" this object
-      guarantee(o->is_forwarded(), "Object must be forwarded if the cas failed.");
-
-      // Try to deallocate the space.  If it was directly allocated we cannot
-      // deallocate it, so we have to test.  If the deallocation fails,
-      // overwrite with a filler object.
-      if (new_obj_is_tenured) {
-        if (!_old_lab.unallocate_object(cast_from_oop<HeapWord*>(new_obj), new_obj_size)) {
-          CollectedHeap::fill_with_object(cast_from_oop<HeapWord*>(new_obj), new_obj_size);
-        }
-      } else if (!_young_lab.unallocate_object(cast_from_oop<HeapWord*>(new_obj), new_obj_size)) {
-        CollectedHeap::fill_with_object(cast_from_oop<HeapWord*>(new_obj), new_obj_size);
-      }
-
-      // don't update this before the unallocation!
-      // Using acquire though consume would be accurate for accessing new_obj.
-      new_obj = o->forwardee_acquire();
-    }
-  } else {
-    assert(o->is_forwarded(), "Sanity");
-    new_obj = o->forwardee_acquire();
   }
 
-  // This code must come after the CAS test, or it will print incorrect
+  // Otherwise try allocating obj tenured
-  // information.
+  if (new_obj == NULL) {
-  log_develop_trace(gc, scavenge)("{%s %s " PTR_FORMAT " -> " PTR_FORMAT " (%d)}",
+#ifndef PRODUCT
-                                  should_scavenge(&new_obj) ? "copying" : "tenuring",
+    if (ParallelScavengeHeap::heap()->promotion_should_fail()) {
-                                  new_obj->klass()->internal_name(), p2i((void *)o), p2i((void *)new_obj), new_obj->size());
+      return oop_promotion_failed(o, test_mark);
+    }
+#endif  // #ifndef PRODUCT
 
-  return new_obj;
+    new_obj = cast_to_oop(_old_lab.allocate(new_obj_size));
+    new_obj_is_tenured = true;
+
+    if (new_obj == NULL) {
+      if (!_old_gen_is_full) {
+        // Do we allocate directly, or flush and refill?
+        if (new_obj_size > (OldPLABSize / 2)) {
+          // Allocate this object directly
+          new_obj = cast_to_oop(old_gen()->allocate(new_obj_size));
+          promotion_trace_event(new_obj, o, new_obj_size, age, true, NULL);
+        } else {
+          // Flush and fill
+          _old_lab.flush();
+
+          HeapWord* lab_base = old_gen()->allocate(OldPLABSize);
+          if(lab_base != NULL) {
+#ifdef ASSERT
+            // Delay the initialization of the promotion lab (plab).
+            // This exposes uninitialized plabs to card table processing.
+            if (GCWorkerDelayMillis > 0) {
+              os::naked_sleep(GCWorkerDelayMillis);
+            }
+#endif
+            _old_lab.initialize(MemRegion(lab_base, OldPLABSize));
+            // Try the old lab allocation again.
+            new_obj = cast_to_oop(_old_lab.allocate(new_obj_size));
+            promotion_trace_event(new_obj, o, new_obj_size, age, true, &_old_lab);
+          }
+        }
+      }
+
+      // This is the promotion failed test, and code handling.
+      // The code belongs here for two reasons. It is slightly
+      // different than the code below, and cannot share the
+      // CAS testing code. Keeping the code here also minimizes
+      // the impact on the common case fast path code.
+
+      if (new_obj == NULL) {
+        _old_gen_is_full = true;
+        return oop_promotion_failed(o, test_mark);
+      }
+    }
+  }
+
+  assert(new_obj != NULL, "allocation should have succeeded");
+
+  // Copy obj
+  Copy::aligned_disjoint_words(cast_from_oop<HeapWord*>(o), cast_from_oop<HeapWord*>(new_obj), new_obj_size);
+
+  // Now we have to CAS in the header.
+  // Make copy visible to threads reading the forwardee.
+  oop forwardee = o->forward_to_atomic(new_obj, test_mark, memory_order_release);
+  if (forwardee == NULL) {  // forwardee is NULL when forwarding is successful
+    // We won any races, we "own" this object.
+    assert(new_obj == o->forwardee(), "Sanity");
+
+    // Increment age if obj still in new generation. Now that
+    // we're dealing with a markWord that cannot change, it is
+    // okay to use the non mt safe oop methods.
+    if (!new_obj_is_tenured) {
+      new_obj->incr_age();
+      assert(young_space()->contains(new_obj), "Attempt to push non-promoted obj");
+    }
+
+    log_develop_trace(gc, scavenge)("{%s %s " PTR_FORMAT " -> " PTR_FORMAT " (%d)}",
+                                    new_obj_is_tenured ? "copying" : "tenuring",
+                                    new_obj->klass()->internal_name(),
+                                    p2i((void *)o), p2i((void *)new_obj), new_obj->size());
+
+    // Do the size comparison first with new_obj_size, which we
+    // already have. Hopefully, only a few objects are larger than
+    // _min_array_size_for_chunking, and most of them will be arrays.
+    // So, the is->objArray() test would be very infrequent.
+    if (new_obj_size > _min_array_size_for_chunking &&
+        new_obj->is_objArray() &&
+        PSChunkLargeArrays) {
+      // we'll chunk it
+      push_depth(ScannerTask(PartialArrayScanTask(o)));
+      TASKQUEUE_STATS_ONLY(++_arrays_chunked; ++_array_chunk_pushes);
+    } else {
+      // we'll just push its contents
+      push_contents(new_obj);
+    }
+    return new_obj;
+  } else {
+    // We lost, someone else "owns" this object.
+    // Ensure loads from the forwardee follow all changes that preceeded the
+    // release-cmpxchg that performed the forwarding in another thread.
+    OrderAccess::acquire();
+
+    assert(o->is_forwarded(), "Object must be forwarded if the cas failed.");
+    assert(o->forwardee() == forwardee, "invariant");
+
+    // Try to deallocate the space.  If it was directly allocated we cannot
+    // deallocate it, so we have to test.  If the deallocation fails,
+    // overwrite with a filler object.
+    if (new_obj_is_tenured) {
+      if (!_old_lab.unallocate_object(cast_from_oop<HeapWord*>(new_obj), new_obj_size)) {
+        CollectedHeap::fill_with_object(cast_from_oop<HeapWord*>(new_obj), new_obj_size);
+      }
+    } else if (!_young_lab.unallocate_object(cast_from_oop<HeapWord*>(new_obj), new_obj_size)) {
+      CollectedHeap::fill_with_object(cast_from_oop<HeapWord*>(new_obj), new_obj_size);
+    }
+    return forwardee;
+  }
 }
 
 // Attempt to "claim" oop at p via CAS, push the new obj if successful
@@ -305,18 +317,7 @@ inline void PSPromotionManager::copy_and_push_safe_barrier(T* p) {
   assert(should_scavenge(p, true), "revisiting object?");
 
   oop o = RawAccess<IS_NOT_NULL>::oop_load(p);
-  oop new_obj = o->is_forwarded()
+  oop new_obj = copy_to_survivor_space<promote_immediately>(o);
-        ? o->forwardee()
-        : copy_to_survivor_space<promote_immediately>(o);
-
-  // This code must come after the CAS test, or it will print incorrect
-  // information.
-  if (log_develop_is_enabled(Trace, gc, scavenge) && o->is_forwarded()) {
-    log_develop_trace(gc, scavenge)("{%s %s " PTR_FORMAT " -> " PTR_FORMAT " (%d)}",
-                      "forwarding",
-                      new_obj->klass()->internal_name(), p2i((void *)o), p2i((void *)new_obj), new_obj->size());
-  }
-
   RawAccess<IS_NOT_NULL>::oop_store(p, new_obj);
 
   // We cannot mark without test, as some code passes us pointers

src/hotspot/share/gc/parallel/psScavenge.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2002, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2002, 2021, 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
@@ -133,8 +133,6 @@ class PSScavenge: AllStatic {
   template <class T> static inline bool should_scavenge(T* p, MutableSpace* to_space);
   template <class T> static inline bool should_scavenge(T* p, bool check_to_space);
 
-  static void copy_and_push_safe_barrier_from_klass(PSPromotionManager* pm, oop* p);
-
   // Is an object in the young generation
   // This assumes that the 'o' is in the heap,
   // so it only checks one side of the complete predicate.

src/hotspot/share/oops/oop.hpp

@@ -257,7 +257,6 @@ class oopDesc {
   inline oop forward_to_atomic(oop p, markWord compare, atomic_memory_order order = memory_order_conservative);
 
   inline oop forwardee() const;
-  inline oop forwardee_acquire() const;
 
   // Age of object during scavenge
   inline uint age() const;

src/hotspot/share/oops/oop.inline.hpp

@@ -305,13 +305,6 @@ oop oopDesc::forwardee() const {
   return cast_to_oop(mark().decode_pointer());
 }
 
-// Note that the forwardee is not the same thing as the displaced_mark.
-// The forwardee is used when copying during scavenge and mark-sweep.
-// It does need to clear the low two locking- and GC-related bits.
-oop oopDesc::forwardee_acquire() const {
-  return cast_to_oop(Atomic::load_acquire(&_mark).decode_pointer());
-}
-
 // The following method needs to be MT safe.
 uint oopDesc::age() const {
   assert(!is_forwarded(), "Attempt to read age from forwarded mark");