8160539: Stack frame scanning acquires DerivedPointerTableGC_lock mutex
authorkbarrett
Thu, 16 May 2019 20:14:54 -0400
changeset 54916 7136c9ac56a7
parent 54915 278600885731
child 54917 81852d53e585
8160539: Stack frame scanning acquires DerivedPointerTableGC_lock mutex Summary: Use lock-free stack for accumulating table entries. Reviewed-by: tschatzl, kvn
src/hotspot/share/compiler/oopMap.cpp
src/hotspot/share/compiler/oopMap.hpp
src/hotspot/share/runtime/mutexLocker.cpp
src/hotspot/share/runtime/mutexLocker.hpp
--- a/src/hotspot/share/compiler/oopMap.cpp	Thu May 16 18:45:30 2019 -0400
+++ b/src/hotspot/share/compiler/oopMap.cpp	Thu May 16 20:14:54 2019 -0400
@@ -38,6 +38,7 @@
 #include "runtime/handles.inline.hpp"
 #include "runtime/signature.hpp"
 #include "utilities/align.hpp"
+#include "utilities/lockFreeStack.hpp"
 #ifdef COMPILER1
 #include "c1/c1_Defs.hpp"
 #endif
@@ -337,10 +338,6 @@
       }
 #endif
 #endif // !TIERED
-      // Protect the operation on the derived pointers.  This
-      // protects the addition of derived pointers to the shared
-      // derived pointer table in DerivedPointerTable::add().
-      MutexLocker x(DerivedPointerTableGC_lock, Mutex::_no_safepoint_check_flag);
       do {
         omv = oms.current();
         oop* loc = fr->oopmapreg_to_location(omv.reg(),reg_map);
@@ -745,44 +742,56 @@
 
 #if COMPILER2_OR_JVMCI
 
-class DerivedPointerEntry : public CHeapObj<mtCompiler> {
- private:
-  oop*     _location; // Location of derived pointer (also pointing to the base)
-  intptr_t _offset;   // Offset from base pointer
- public:
-  DerivedPointerEntry(oop* location, intptr_t offset) { _location = location; _offset = offset; }
-  oop* location()    { return _location; }
-  intptr_t  offset() { return _offset; }
+class DerivedPointerTable::Entry : public CHeapObj<mtCompiler> {
+  oop* _location;   // Location of derived pointer, also pointing to base
+  intptr_t _offset; // Offset from base pointer
+  Entry* volatile _next;
+
+  static Entry* volatile* next_ptr(Entry& entry) { return &entry._next; }
+
+public:
+  Entry(oop* location, intptr_t offset) :
+    _location(location), _offset(offset), _next(NULL) {}
+
+  oop* location() const { return _location; }
+  intptr_t offset() const { return _offset; }
+  Entry* next() const { return _next; }
+
+  typedef LockFreeStack<Entry, &next_ptr> List;
+  static List* _list;
 };
 
-
-GrowableArray<DerivedPointerEntry*>* DerivedPointerTable::_list = NULL;
+DerivedPointerTable::Entry::List* DerivedPointerTable::Entry::_list = NULL;
 bool DerivedPointerTable::_active = false;
 
+bool DerivedPointerTable::is_empty() {
+  return Entry::_list == NULL || Entry::_list->empty();
+}
 
 void DerivedPointerTable::clear() {
   // The first time, we create the list.  Otherwise it should be
   // empty.  If not, then we have probably forgotton to call
   // update_pointers after last GC/Scavenge.
   assert (!_active, "should not be active");
-  assert(_list == NULL || _list->length() == 0, "table not empty");
-  if (_list == NULL) {
-    _list = new (ResourceObj::C_HEAP, mtCompiler) GrowableArray<DerivedPointerEntry*>(10, true); // Allocated on C heap
+  assert(is_empty(), "table not empty");
+  if (Entry::_list == NULL) {
+    void* mem = NEW_C_HEAP_OBJ(Entry::List, mtCompiler);
+    Entry::_list = ::new (mem) Entry::List();
   }
   _active = true;
 }
 
-
 // Returns value of location as an int
-intptr_t value_of_loc(oop *pointer) { return cast_from_oop<intptr_t>((*pointer)); }
-
+inline intptr_t value_of_loc(oop *pointer) {
+  return cast_from_oop<intptr_t>((*pointer));
+}
 
 void DerivedPointerTable::add(oop *derived_loc, oop *base_loc) {
   assert(Universe::heap()->is_in_or_null(*base_loc), "not an oop");
   assert(derived_loc != base_loc, "Base and derived in same location");
   if (_active) {
     assert(*derived_loc != (void*)base_loc, "location already added");
-    assert(_list != NULL, "list must exist");
+    assert(Entry::_list != NULL, "list must exist");
     intptr_t offset = value_of_loc(derived_loc) - value_of_loc(base_loc);
     // This assert is invalid because derived pointers can be
     // arbitrarily far away from their base.
@@ -798,21 +807,21 @@
     }
     // Set derived oop location to point to base.
     *derived_loc = (oop)base_loc;
-    assert_lock_strong(DerivedPointerTableGC_lock);
-    DerivedPointerEntry *entry = new DerivedPointerEntry(derived_loc, offset);
-    _list->append(entry);
+    Entry* entry = new Entry(derived_loc, offset);
+    Entry::_list->push(*entry);
   }
 }
 
-
 void DerivedPointerTable::update_pointers() {
-  assert(_list != NULL, "list must exist");
-  for(int i = 0; i < _list->length(); i++) {
-    DerivedPointerEntry* entry = _list->at(i);
+  assert(Entry::_list != NULL, "list must exist");
+  Entry* entries = Entry::_list->pop_all();
+  while (entries != NULL) {
+    Entry* entry = entries;
+    entries = entry->next();
     oop* derived_loc = entry->location();
     intptr_t offset  = entry->offset();
     // The derived oop was setup to point to location of base
-    oop  base        = **(oop**)derived_loc;
+    oop base = **(oop**)derived_loc;
     assert(Universe::heap()->is_in_or_null(base), "must be an oop");
 
     *derived_loc = (oop)(((address)base) + offset);
@@ -826,13 +835,8 @@
 
     // Delete entry
     delete entry;
-    _list->at_put(i, NULL);
   }
-  // Clear list, so it is ready for next traversal (this is an invariant)
-  if (TraceDerivedPointers && !_list->is_empty()) {
-    tty->print_cr("--------------------------");
-  }
-  _list->clear();
+  assert(Entry::_list->empty(), "invariant");
   _active = false;
 }
 
--- a/src/hotspot/share/compiler/oopMap.hpp	Thu May 16 18:45:30 2019 -0400
+++ b/src/hotspot/share/compiler/oopMap.hpp	Thu May 16 20:14:54 2019 -0400
@@ -29,7 +29,6 @@
 #include "code/vmreg.hpp"
 #include "memory/allocation.hpp"
 #include "oops/oopsHierarchy.hpp"
-#include "utilities/growableArray.hpp"
 
 // Interface for generating the frame map for compiled code.  A frame map
 // describes for a specific pc whether each register and frame stack slot is:
@@ -42,7 +41,6 @@
 
 class frame;
 class RegisterMap;
-class DerivedPointerEntry;
 class OopClosure;
 
 class OopMapValue: public StackObj {
@@ -433,13 +431,14 @@
 class DerivedPointerTable : public AllStatic {
   friend class VMStructs;
  private:
-   static GrowableArray<DerivedPointerEntry*>* _list;
-   static bool _active;                      // do not record pointers for verify pass etc.
+  class Entry;
+  static bool _active;                      // do not record pointers for verify pass etc.
+
  public:
   static void clear();                       // Called before scavenge/GC
   static void add(oop *derived, oop *base);  // Called during scavenge/GC
   static void update_pointers();             // Called after  scavenge/GC
-  static bool is_empty()                     { return _list == NULL || _list->is_empty(); }
+  static bool is_empty();
   static bool is_active()                    { return _active; }
   static void set_active(bool value)         { _active = value; }
 };
--- a/src/hotspot/share/runtime/mutexLocker.cpp	Thu May 16 18:45:30 2019 -0400
+++ b/src/hotspot/share/runtime/mutexLocker.cpp	Thu May 16 20:14:54 2019 -0400
@@ -89,7 +89,6 @@
 Mutex*   MarkStackChunkList_lock      = NULL;
 Mutex*   MonitoringSupport_lock       = NULL;
 Mutex*   ParGCRareEvent_lock          = NULL;
-Mutex*   DerivedPointerTableGC_lock   = NULL;
 Monitor* CGCPhaseManager_lock         = NULL;
 Mutex*   Compile_lock                 = NULL;
 Monitor* MethodCompileQueue_lock      = NULL;
@@ -250,7 +249,6 @@
     def(StringDedupTable_lock      , PaddedMutex  , leaf,        true,  Monitor::_safepoint_check_never);
   }
   def(ParGCRareEvent_lock          , PaddedMutex  , leaf     ,   true,  Monitor::_safepoint_check_always);
-  def(DerivedPointerTableGC_lock   , PaddedMutex  , leaf,        true,  Monitor::_safepoint_check_never);
   def(CGCPhaseManager_lock         , PaddedMonitor, leaf,        false, Monitor::_safepoint_check_always);
   def(CodeCache_lock               , PaddedMutex  , special,     true,  Monitor::_safepoint_check_never);
   def(RawMonitor_lock              , PaddedMutex  , special,     true,  Monitor::_safepoint_check_never);
--- a/src/hotspot/share/runtime/mutexLocker.hpp	Thu May 16 18:45:30 2019 -0400
+++ b/src/hotspot/share/runtime/mutexLocker.hpp	Thu May 16 20:14:54 2019 -0400
@@ -66,7 +66,6 @@
 extern Mutex*   MethodData_lock;                 // a lock on installation of method data
 extern Mutex*   TouchedMethodLog_lock;           // a lock on allocation of LogExecutedMethods info
 extern Mutex*   RetData_lock;                    // a lock on installation of RetData inside method data
-extern Mutex*   DerivedPointerTableGC_lock;      // a lock to protect the derived pointer table
 extern Monitor* CGCPhaseManager_lock;            // a lock to protect a concurrent GC's phase management
 extern Monitor* VMOperationQueue_lock;           // a lock on queue of vm_operations waiting to execute
 extern Monitor* VMOperationRequest_lock;         // a lock on Threads waiting for a vm_operation to terminate