8186042: Optimize OopMapCache lookup

Use lock free access to oopMapCache

Co-authored-by: Frederic Parain <frederic.parain@oracle.com>
Reviewed-by: dholmes, sspitsyn

hotspot/src/share/vm/gc/shared/vmGCOperations.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2005, 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2005, 2017, 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
@@ -30,6 +30,7 @@
 #include "gc/shared/gcLocker.inline.hpp"
 #include "gc/shared/genCollectedHeap.hpp"
 #include "gc/shared/vmGCOperations.hpp"
+#include "interpreter/oopMapCache.hpp"
 #include "logging/log.hpp"
 #include "memory/oopFactory.hpp"
 #include "runtime/handles.inline.hpp"
@@ -111,6 +112,9 @@ bool VM_GC_Operation::doit_prologue() {
 
 void VM_GC_Operation::doit_epilogue() {
   assert(Thread::current()->is_Java_thread(), "just checking");
+  // Clean up old interpreter OopMap entries that were replaced
+  // during the GC thread root traversal.
+  OopMapCache::cleanup_old_entries();
   if (Universe::has_reference_pending_list()) {
     Heap_lock->notify_all();
   }

hotspot/src/share/vm/interpreter/oopMapCache.cpp

@@ -25,6 +25,7 @@
 #include "precompiled.hpp"
 #include "interpreter/oopMapCache.hpp"
 #include "logging/log.hpp"
+#include "logging/logStream.hpp"
 #include "memory/allocation.inline.hpp"
 #include "memory/resourceArea.hpp"
 #include "oops/oop.inline.hpp"
@@ -37,6 +38,9 @@ class OopMapCacheEntry: private InterpreterOopMap {
   friend class OopMapCache;
   friend class VerifyClosure;
 
+ private:
+  OopMapCacheEntry* _next;
+
  protected:
   // Initialization
   void fill(const methodHandle& method, int bci);
@@ -54,8 +58,9 @@ class OopMapCacheEntry: private InterpreterOopMap {
 
  public:
   OopMapCacheEntry() : InterpreterOopMap() {
+    _next = NULL;
 #ifdef ASSERT
-     _resource_allocate_bit_mask = false;
+    _resource_allocate_bit_mask = false;
 #endif
   }
 };
@@ -263,23 +268,26 @@ bool OopMapCacheEntry::verify_mask(CellTypeState* vars, CellTypeState* stack, in
 
   // Check if map is generated correctly
   // (Use ?: operator to make sure all 'true' & 'false' are represented exactly the same so we can use == afterwards)
-  if (TraceOopMapGeneration && Verbose) tty->print("Locals (%d): ", max_locals);
+  Log(interpreter, oopmap) logv;
+  LogStream st(logv.trace());
 
+  st.print("Locals (%d): ", max_locals);
   for(int i = 0; i < max_locals; i++) {
     bool v1 = is_oop(i)               ? true : false;
     bool v2 = vars[i].is_reference()  ? true : false;
     assert(v1 == v2, "locals oop mask generation error");
-    if (TraceOopMapGeneration && Verbose) tty->print("%d", v1 ? 1 : 0);
+    st.print("%d", v1 ? 1 : 0);
   }
+  st.cr();
 
-  if (TraceOopMapGeneration && Verbose) { tty->cr(); tty->print("Stack (%d): ", stack_top); }
+  st.print("Stack (%d): ", stack_top);
   for(int j = 0; j < stack_top; j++) {
     bool v1 = is_oop(max_locals + j)  ? true : false;
     bool v2 = stack[j].is_reference() ? true : false;
     assert(v1 == v2, "stack oop mask generation error");
-    if (TraceOopMapGeneration && Verbose) tty->print("%d", v1 ? 1 : 0);
+    st.print("%d", v1 ? 1 : 0);
   }
-  if (TraceOopMapGeneration && Verbose) tty->cr();
+  st.cr();
   return true;
 }
 
@@ -373,8 +381,6 @@ void OopMapCacheEntry::set_mask(CellTypeState *vars, CellTypeState *stack, int s
 
   // verify bit mask
   assert(verify_mask(vars, stack, max_locals, stack_top), "mask could not be verified");
-
-
 }
 
 void OopMapCacheEntry::flush() {
@@ -385,16 +391,6 @@ void OopMapCacheEntry::flush() {
 
 // Implementation of OopMapCache
 
-#ifndef PRODUCT
-
-static long _total_memory_usage = 0;
-
-long OopMapCache::memory_usage() {
-  return _total_memory_usage;
-}
-
-#endif
-
 void InterpreterOopMap::resource_copy(OopMapCacheEntry* from) {
   assert(_resource_allocate_bit_mask,
     "Should not resource allocate the _bit_mask");
@@ -435,15 +431,11 @@ inline unsigned int OopMapCache::hash_value_for(const methodHandle& method, int
          ^ ((unsigned int) method->size_of_parameters() << 6);
 }
 
+OopMapCacheEntry* volatile OopMapCache::_old_entries = NULL;
 
-OopMapCache::OopMapCache() :
-  _mut(Mutex::leaf, "An OopMapCache lock", true)
-{
-  _array  = NEW_C_HEAP_ARRAY(OopMapCacheEntry, _size, mtClass);
-  // Cannot call flush for initialization, since flush
-  // will check if memory should be deallocated
-  for(int i = 0; i < _size; i++) _array[i].initialize();
-  NOT_PRODUCT(_total_memory_usage += sizeof(OopMapCache) + (sizeof(OopMapCacheEntry) * _size);)
+OopMapCache::OopMapCache() {
+  _array  = NEW_C_HEAP_ARRAY(OopMapCacheEntry*, _size, mtClass);
+  for(int i = 0; i < _size; i++) _array[i] = NULL;
 }
 
 
@@ -452,114 +444,154 @@ OopMapCache::~OopMapCache() {
   // Deallocate oop maps that are allocated out-of-line
   flush();
   // Deallocate array
-  NOT_PRODUCT(_total_memory_usage -= sizeof(OopMapCache) + (sizeof(OopMapCacheEntry) * _size);)
-  FREE_C_HEAP_ARRAY(OopMapCacheEntry, _array);
+  FREE_C_HEAP_ARRAY(OopMapCacheEntry*, _array);
 }
 
 OopMapCacheEntry* OopMapCache::entry_at(int i) const {
-  return &_array[i % _size];
+  return (OopMapCacheEntry*)OrderAccess::load_ptr_acquire(&(_array[i % _size]));
+}
+
+bool OopMapCache::put_at(int i, OopMapCacheEntry* entry, OopMapCacheEntry* old) {
+  return Atomic::cmpxchg_ptr (entry, &_array[i % _size], old) == old;
 }
 
 void OopMapCache::flush() {
-  for (int i = 0; i < _size; i++) _array[i].flush();
+  for (int i = 0; i < _size; i++) {
+    OopMapCacheEntry* entry = _array[i];
+    if (entry != NULL) {
+      _array[i] = NULL;  // no barrier, only called in OopMapCache destructor
+      entry->flush();
+      FREE_C_HEAP_OBJ(entry);
+    }
+  }
 }
 
 void OopMapCache::flush_obsolete_entries() {
-  for (int i = 0; i < _size; i++)
-    if (!_array[i].is_empty() && _array[i].method()->is_old()) {
+  assert(SafepointSynchronize::is_at_safepoint(), "called by RedefineClasses in a safepoint");
+  for (int i = 0; i < _size; i++) {
+    OopMapCacheEntry* entry = _array[i];
+    if (entry != NULL && !entry->is_empty() && entry->method()->is_old()) {
       // Cache entry is occupied by an old redefined method and we don't want
       // to pin it down so flush the entry.
       if (log_is_enabled(Debug, redefine, class, oopmap)) {
         ResourceMark rm;
-        log_debug(redefine, class, oopmap)
+        log_debug(redefine, class, interpreter, oopmap)
           ("flush: %s(%s): cached entry @%d",
-           _array[i].method()->name()->as_C_string(), _array[i].method()->signature()->as_C_string(), i);
+           entry->method()->name()->as_C_string(), entry->method()->signature()->as_C_string(), i);
       }
-      _array[i].flush();
+      _array[i] = NULL;
+      entry->flush();
+      FREE_C_HEAP_OBJ(entry);
     }
+  }
 }
 
+// Called by GC for thread root scan during a safepoint only.  The other interpreted frame oopmaps
+// are generated locally and not cached.
 void OopMapCache::lookup(const methodHandle& method,
                          int bci,
-                         InterpreterOopMap* entry_for) const {
-  MutexLocker x(&_mut);
-
-  OopMapCacheEntry* entry = NULL;
+                         InterpreterOopMap* entry_for) {
+  assert(SafepointSynchronize::is_at_safepoint(), "called by GC in a safepoint");
   int probe = hash_value_for(method, bci);
+  int i;
+  OopMapCacheEntry* entry = NULL;
+
+  if (log_is_enabled(Debug, interpreter, oopmap)) {
+    static int count = 0;
+    ResourceMark rm;
+    log_debug(interpreter, oopmap)
+          ("%d - Computing oopmap at bci %d for %s at hash %d", ++count, bci,
+           method()->name_and_sig_as_C_string(), probe);
+  }
 
   // Search hashtable for match
-  int i;
   for(i = 0; i < _probe_depth; i++) {
     entry = entry_at(probe + i);
-    if (entry->match(method, bci)) {
+    if (entry != NULL && !entry->is_empty() && entry->match(method, bci)) {
       entry_for->resource_copy(entry);
       assert(!entry_for->is_empty(), "A non-empty oop map should be returned");
+      log_debug(interpreter, oopmap)("- found at hash %d", probe + i);
       return;
     }
   }
 
-  if (TraceOopMapGeneration) {
-    static int count = 0;
-    ResourceMark rm;
-    tty->print("%d - Computing oopmap at bci %d for ", ++count, bci);
-    method->print_value(); tty->cr();
-  }
-
   // Entry is not in hashtable.
-  // Compute entry and return it
+  // Compute entry
+
+  OopMapCacheEntry* tmp = NEW_C_HEAP_OBJ(OopMapCacheEntry, mtClass);
+  tmp->initialize();
+  tmp->fill(method, bci);
+  entry_for->resource_copy(tmp);
 
   if (method->should_not_be_cached()) {
     // It is either not safe or not a good idea to cache this Method*
     // at this time. We give the caller of lookup() a copy of the
     // interesting info via parameter entry_for, but we don't add it to
     // the cache. See the gory details in Method*.cpp.
-    compute_one_oop_map(method, bci, entry_for);
+    FREE_C_HEAP_OBJ(tmp);
     return;
   }
 
   // First search for an empty slot
   for(i = 0; i < _probe_depth; i++) {
-    entry  = entry_at(probe + i);
-    if (entry->is_empty()) {
-      entry->fill(method, bci);
-      entry_for->resource_copy(entry);
-      assert(!entry_for->is_empty(), "A non-empty oop map should be returned");
-      return;
+    entry = entry_at(probe + i);
+    if (entry == NULL) {
+      if (put_at(probe + i, tmp, NULL)) {
+        assert(!entry_for->is_empty(), "A non-empty oop map should be returned");
+        return;
+      }
     }
   }
 
-  if (TraceOopMapGeneration) {
-    ResourceMark rm;
-    tty->print_cr("*** collision in oopmap cache - flushing item ***");
-  }
+  log_debug(interpreter, oopmap)("*** collision in oopmap cache - flushing item ***");
 
   // No empty slot (uncommon case). Use (some approximation of a) LRU algorithm
-  //entry_at(probe + _probe_depth - 1)->flush();
-  //for(i = _probe_depth - 1; i > 0; i--) {
-  //  // Coping entry[i] = entry[i-1];
-  //  OopMapCacheEntry *to   = entry_at(probe + i);
-  //  OopMapCacheEntry *from = entry_at(probe + i - 1);
-  //  to->copy(from);
-  // }
-
-  assert(method->is_method(), "gaga");
-
-  entry = entry_at(probe + 0);
-  entry->fill(method, bci);
-
-  // Copy the  newly cached entry to input parameter
-  entry_for->resource_copy(entry);
-
-  if (TraceOopMapGeneration) {
-    ResourceMark rm;
-    tty->print("Done with ");
-    method->print_value(); tty->cr();
+  // where the first entry in the collision array is replaced with the new one.
+  OopMapCacheEntry* old = entry_at(probe + 0);
+  if (put_at(probe + 0, tmp, old)) {
+    enqueue_for_cleanup(old);
+  } else {
+    enqueue_for_cleanup(tmp);
   }
-  assert(!entry_for->is_empty(), "A non-empty oop map should be returned");
 
+  assert(!entry_for->is_empty(), "A non-empty oop map should be returned");
   return;
 }
 
+void OopMapCache::enqueue_for_cleanup(OopMapCacheEntry* entry) {
+  bool success = false;
+  OopMapCacheEntry* head;
+  do {
+    head = _old_entries;
+    entry->_next = head;
+    success = Atomic::cmpxchg_ptr (entry, &_old_entries, head) == head;
+  } while (!success);
+
+  if (log_is_enabled(Debug, interpreter, oopmap)) {
+    ResourceMark rm;
+    log_debug(interpreter, oopmap)("enqueue %s at bci %d for cleanup",
+                          entry->method()->name_and_sig_as_C_string(), entry->bci());
+  }
+}
+
+// This is called after GC threads are done and nothing is accessing the old_entries
+// list, so no synchronization needed.
+void OopMapCache::cleanup_old_entries() {
+  OopMapCacheEntry* entry = _old_entries;
+  _old_entries = NULL;
+  while (entry != NULL) {
+    if (log_is_enabled(Debug, interpreter, oopmap)) {
+      ResourceMark rm;
+      log_debug(interpreter, oopmap)("cleanup entry %s at bci %d",
+                          entry->method()->name_and_sig_as_C_string(), entry->bci());
+    }
+    OopMapCacheEntry* next = entry->_next;
+    entry->flush();
+    FREE_C_HEAP_OBJ(entry);
+    entry = next;
+  }
+}
+
 void OopMapCache::compute_one_oop_map(const methodHandle& method, int bci, InterpreterOopMap* entry) {
   // Due to the invariants above it's tricky to allocate a temporary OopMapCacheEntry on the stack
   OopMapCacheEntry* tmp = NEW_C_HEAP_ARRAY(OopMapCacheEntry, 1, mtClass);

hotspot/src/share/vm/interpreter/oopMapCache.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1997, 2015, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2017, 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
@@ -144,17 +144,19 @@ class InterpreterOopMap: ResourceObj {
 };
 
 class OopMapCache : public CHeapObj<mtClass> {
+ static OopMapCacheEntry* volatile _old_entries;
  private:
   enum { _size        = 32,     // Use fixed size for now
          _probe_depth = 3       // probe depth in case of collisions
   };
 
-  OopMapCacheEntry* _array;
+  OopMapCacheEntry* volatile * _array;
 
   unsigned int hash_value_for(const methodHandle& method, int bci) const;
   OopMapCacheEntry* entry_at(int i) const;
+  bool put_at(int i, OopMapCacheEntry* entry, OopMapCacheEntry* old);
 
-  mutable Mutex _mut;
+  static void enqueue_for_cleanup(OopMapCacheEntry* entry);
 
   void flush();
 
@@ -167,13 +169,11 @@ class OopMapCache : public CHeapObj<mtClass> {
 
   // Returns the oopMap for (method, bci) in parameter "entry".
   // Returns false if an oop map was not found.
-  void lookup(const methodHandle& method, int bci, InterpreterOopMap* entry) const;
+  void lookup(const methodHandle& method, int bci, InterpreterOopMap* entry);
 
   // Compute an oop map without updating the cache or grabbing any locks (for debugging)
   static void compute_one_oop_map(const methodHandle& method, int bci, InterpreterOopMap* entry);
-
-  // Returns total no. of bytes allocated as part of OopMapCache's
-  static long memory_usage()                     PRODUCT_RETURN0;
+  static void cleanup_old_entries();
 };
 
 #endif // SHARE_VM_INTERPRETER_OOPMAPCACHE_HPP

hotspot/src/share/vm/logging/logTag.hpp

@@ -74,6 +74,7 @@
   LOG_TAG(iklass) \
   LOG_TAG(init) \
   LOG_TAG(inlining) \
+  LOG_TAG(interpreter) \
   LOG_TAG(itables) \
   LOG_TAG(jit) \
   LOG_TAG(jni) \

hotspot/src/share/vm/oops/method.cpp

@@ -214,26 +214,14 @@ int Method::fast_exception_handler_bci_for(const methodHandle& mh, Klass* ex_kla
 }
 
 void Method::mask_for(int bci, InterpreterOopMap* mask) {
-
-  Thread* myThread    = Thread::current();
-  methodHandle h_this(myThread, this);
-#if defined(ASSERT) && !INCLUDE_JVMCI
-  bool has_capability = myThread->is_VM_thread() ||
-                        myThread->is_ConcurrentGC_thread() ||
-                        myThread->is_GC_task_thread();
-
-  if (!has_capability) {
-    if (!VerifyStack && !VerifyLastFrame) {
-      // verify stack calls this outside VM thread
-      warning("oopmap should only be accessed by the "
-              "VM, GC task or CMS threads (or during debugging)");
-      InterpreterOopMap local_mask;
-      method_holder()->mask_for(h_this, bci, &local_mask);
-      local_mask.print();
-    }
+  methodHandle h_this(Thread::current(), this);
+  // Only GC uses the OopMapCache during thread stack root scanning
+  // any other uses generate an oopmap but do not save it in the cache.
+  if (Universe::heap()->is_gc_active()) {
+    method_holder()->mask_for(h_this, bci, mask);
+  } else {
+    OopMapCache::compute_one_oop_map(h_this, bci, mask);
   }
-#endif
-  method_holder()->mask_for(h_this, bci, mask);
   return;
 }
 

hotspot/src/share/vm/runtime/globals.hpp

@@ -715,9 +715,6 @@ public:
   product(bool, PrintVMQWaitTime, false,                                    \
           "Print out the waiting time in VM operation queue")               \
                                                                             \
-  develop(bool, TraceOopMapGeneration, false,                               \
-          "Show OopMapGeneration")                                          \
-                                                                            \
   product(bool, MethodFlushing, true,                                       \
           "Reclamation of zombie and not-entrant methods")                  \
                                                                             \

hotspot/src/share/vm/runtime/memprofiler.cpp

@@ -129,7 +129,7 @@ void MemProfiler::do_trace() {
   fprintf(_log_fp, UINTX_FORMAT_W(6) "," UINTX_FORMAT_W(6) ",%6ld\n",
           handles_memory_usage / K,
           resource_memory_usage / K,
-          OopMapCache::memory_usage() / K);
+          0L);
   fflush(_log_fp);
 }
 

hotspot/src/share/vm/runtime/vframe.cpp

@@ -396,14 +396,7 @@ StackValueCollection* interpretedVFrame::expressions() const {
 StackValueCollection* interpretedVFrame::stack_data(bool expressions) const {
 
   InterpreterOopMap oop_mask;
-  // oopmap for current bci
-  if ((TraceDeoptimization && Verbose) JVMCI_ONLY( || PrintDeoptimizationDetails)) {
-    methodHandle m_h(Thread::current(), method());
-    OopMapCache::compute_one_oop_map(m_h, bci(), &oop_mask);
-  } else {
-    method()->mask_for(bci(), &oop_mask);
-  }
-
+  method()->mask_for(bci(), &oop_mask);
   const int mask_len = oop_mask.number_of_entries();
 
   // If the method is native, method()->max_locals() is not telling the truth.