8008962: NPG: Memory regression: One extra Monitor per ConstantPool
authoriklam
Thu, 25 Apr 2013 12:55:49 -0700
changeset 17078 a37993c37937
parent 17077 8607f7c33310
child 17079 45dbf28e160b
8008962: NPG: Memory regression: One extra Monitor per ConstantPool Summary: Re-use InstanceKlass::_init_lock locking ConstantPool as well. Reviewed-by: dholmes, coleenp, acorn
hotspot/src/share/vm/ci/ciEnv.cpp
hotspot/src/share/vm/oops/constantPool.cpp
hotspot/src/share/vm/oops/constantPool.hpp
hotspot/src/share/vm/oops/cpCache.cpp
hotspot/src/share/vm/oops/instanceKlass.cpp
hotspot/src/share/vm/oops/instanceKlass.hpp
hotspot/src/share/vm/prims/jvmtiEnv.cpp
--- a/hotspot/src/share/vm/ci/ciEnv.cpp	Thu Apr 25 03:58:53 2013 -0700
+++ b/hotspot/src/share/vm/ci/ciEnv.cpp	Thu Apr 25 12:55:49 2013 -0700
@@ -483,7 +483,8 @@
     {
       // We have to lock the cpool to keep the oop from being resolved
       // while we are accessing it.
-        MonitorLockerEx ml(cpool->lock());
+      oop cplock = cpool->lock();
+      ObjectLocker ol(cplock, THREAD, cplock != NULL);
       constantTag tag = cpool->tag_at(index);
       if (tag.is_klass()) {
         // The klass has been inserted into the constant pool
--- a/hotspot/src/share/vm/oops/constantPool.cpp	Thu Apr 25 03:58:53 2013 -0700
+++ b/hotspot/src/share/vm/oops/constantPool.cpp	Thu Apr 25 12:55:49 2013 -0700
@@ -40,6 +40,7 @@
 #include "runtime/init.hpp"
 #include "runtime/javaCalls.hpp"
 #include "runtime/signature.hpp"
+#include "runtime/synchronizer.hpp"
 #include "runtime/vframe.hpp"
 
 ConstantPool* ConstantPool::allocate(ClassLoaderData* loader_data, int length, TRAPS) {
@@ -69,7 +70,6 @@
 
   // only set to non-zero if constant pool is merged by RedefineClasses
   set_version(0);
-  set_lock(new Monitor(Monitor::nonleaf + 2, "A constant pool lock"));
 
   // initialize tag array
   int length = tags->length();
@@ -95,9 +95,6 @@
 void ConstantPool::release_C_heap_structures() {
   // walk constant pool and decrement symbol reference counts
   unreference_symbols();
-
-  delete _lock;
-  set_lock(NULL);
 }
 
 objArrayOop ConstantPool::resolved_references() const {
@@ -154,9 +151,6 @@
       ClassLoaderData* loader_data = pool_holder()->class_loader_data();
       set_resolved_references(loader_data->add_handle(refs_handle));
     }
-
-    // Also need to recreate the mutex.  Make sure this matches the constructor
-    set_lock(new Monitor(Monitor::nonleaf + 2, "A constant pool lock"));
   }
 }
 
@@ -167,7 +161,23 @@
   set_resolved_reference_length(
     resolved_references() != NULL ? resolved_references()->length() : 0);
   set_resolved_references(NULL);
-  set_lock(NULL);
+}
+
+oop ConstantPool::lock() {
+  if (_pool_holder) {
+    // We re-use the _pool_holder's init_lock to reduce footprint.
+    // Notes on deadlocks:
+    // [1] This lock is a Java oop, so it can be recursively locked by
+    //     the same thread without self-deadlocks.
+    // [2] Deadlock will happen if there is circular dependency between
+    //     the <clinit> of two Java classes. However, in this case,
+    //     the deadlock would have happened long before we reach
+    //     ConstantPool::lock(), so reusing init_lock does not
+    //     increase the possibility of deadlock.
+    return _pool_holder->init_lock();
+  } else {
+    return NULL;
+  }
 }
 
 int ConstantPool::cp_to_object_index(int cp_index) {
@@ -208,7 +218,9 @@
 
   Symbol* name = NULL;
   Handle       loader;
-  {  MonitorLockerEx ml(this_oop->lock());
+  {
+    oop cplock = this_oop->lock();
+    ObjectLocker ol(cplock , THREAD, cplock != NULL);
 
     if (this_oop->tag_at(which).is_unresolved_klass()) {
       if (this_oop->tag_at(which).is_unresolved_klass_in_error()) {
@@ -255,7 +267,8 @@
 
       bool throw_orig_error = false;
       {
-        MonitorLockerEx ml(this_oop->lock());
+        oop cplock = this_oop->lock();
+        ObjectLocker ol(cplock, THREAD, cplock != NULL);
 
         // some other thread has beaten us and has resolved the class.
         if (this_oop->tag_at(which).is_klass()) {
@@ -323,7 +336,8 @@
       }
       return k();
     } else {
-      MonitorLockerEx ml(this_oop->lock());
+      oop cplock = this_oop->lock();
+      ObjectLocker ol(cplock, THREAD, cplock != NULL);
       // Only updated constant pool - if it is resolved.
       do_resolve = this_oop->tag_at(which).is_unresolved_klass();
       if (do_resolve) {
@@ -619,7 +633,8 @@
                                      int tag, TRAPS) {
   ResourceMark rm;
   Symbol* error = PENDING_EXCEPTION->klass()->name();
-  MonitorLockerEx ml(this_oop->lock());  // lock cpool to change tag.
+  oop cplock = this_oop->lock();
+  ObjectLocker ol(cplock, THREAD, cplock != NULL);  // lock cpool to change tag.
 
   int error_tag = (tag == JVM_CONSTANT_MethodHandle) ?
            JVM_CONSTANT_MethodHandleInError : JVM_CONSTANT_MethodTypeInError;
@@ -780,7 +795,8 @@
   if (cache_index >= 0) {
     // Cache the oop here also.
     Handle result_handle(THREAD, result_oop);
-    MonitorLockerEx ml(this_oop->lock());  // don't know if we really need this
+    oop cplock = this_oop->lock();
+    ObjectLocker ol(cplock, THREAD, cplock != NULL);  // don't know if we really need this
     oop result = this_oop->resolved_references()->obj_at(cache_index);
     // Benign race condition:  resolved_references may already be filled in while we were trying to lock.
     // The important thing here is that all threads pick up the same result.
--- a/hotspot/src/share/vm/oops/constantPool.hpp	Thu Apr 25 03:58:53 2013 -0700
+++ b/hotspot/src/share/vm/oops/constantPool.hpp	Thu Apr 25 12:55:49 2013 -0700
@@ -111,7 +111,6 @@
     int                _version;
   } _saved;
 
-  Monitor*             _lock;
 
   void set_tags(Array<u1>* tags)               { _tags = tags; }
   void tag_at_put(int which, jbyte t)          { tags()->at_put(which, t); }
@@ -823,8 +822,17 @@
 
   void set_resolved_reference_length(int length) { _saved._resolved_reference_length = length; }
   int  resolved_reference_length() const  { return _saved._resolved_reference_length; }
-  void set_lock(Monitor* lock)            { _lock = lock; }
-  Monitor* lock()                         { return _lock; }
+
+  // lock() may return null -- constant pool updates may happen before this lock is
+  // initialized, because the _pool_holder has not been fully initialized and
+  // has not been registered into the system dictionary. In this case, no other
+  // thread can be modifying this constantpool, so no synchronization is
+  // necessary.
+  //
+  // Use cplock() like this:
+  //    oop cplock = cp->lock();
+  //    ObjectLocker ol(cplock , THREAD, cplock != NULL);
+  oop lock();
 
   // Decrease ref counts of symbols that are in the constant pool
   // when the holder class is unloaded
--- a/hotspot/src/share/vm/oops/cpCache.cpp	Thu Apr 25 03:58:53 2013 -0700
+++ b/hotspot/src/share/vm/oops/cpCache.cpp	Thu Apr 25 12:55:49 2013 -0700
@@ -266,7 +266,8 @@
   // the lock, so that when the losing writer returns, he can use the linked
   // cache entry.
 
-  MonitorLockerEx ml(cpool->lock());
+  oop cplock = cpool->lock();
+  ObjectLocker ol(cplock, Thread::current(), cplock != NULL);
   if (!is_f1_null()) {
     return;
   }
--- a/hotspot/src/share/vm/oops/instanceKlass.cpp	Thu Apr 25 03:58:53 2013 -0700
+++ b/hotspot/src/share/vm/oops/instanceKlass.cpp	Thu Apr 25 12:55:49 2013 -0700
@@ -419,25 +419,6 @@
   set_annotations(NULL);
 }
 
-volatile oop InstanceKlass::init_lock() const {
-  volatile oop lock = _init_lock;  // read once
-  assert((oop)lock != NULL || !is_not_initialized(), // initialized or in_error state
-         "only fully initialized state can have a null lock");
-  return lock;
-}
-
-// Set the initialization lock to null so the object can be GC'ed.  Any racing
-// threads to get this lock will see a null lock and will not lock.
-// That's okay because they all check for initialized state after getting
-// the lock and return.
-void InstanceKlass::fence_and_clear_init_lock() {
-  // make sure previous stores are all done, notably the init_state.
-  OrderAccess::storestore();
-  klass_oop_store(&_init_lock, NULL);
-  assert(!is_not_initialized(), "class must be initialized now");
-}
-
-
 bool InstanceKlass::should_be_initialized() const {
   return !is_initialized();
 }
@@ -474,7 +455,7 @@
 void InstanceKlass::eager_initialize_impl(instanceKlassHandle this_oop) {
   EXCEPTION_MARK;
   volatile oop init_lock = this_oop->init_lock();
-  ObjectLocker ol(init_lock, THREAD, init_lock != NULL);
+  ObjectLocker ol(init_lock, THREAD);
 
   // abort if someone beat us to the initialization
   if (!this_oop->is_not_initialized()) return;  // note: not equivalent to is_initialized()
@@ -493,7 +474,6 @@
   } else {
     // linking successfull, mark class as initialized
     this_oop->set_init_state (fully_initialized);
-    this_oop->fence_and_clear_init_lock();
     // trace
     if (TraceClassInitialization) {
       ResourceMark rm(THREAD);
@@ -620,7 +600,7 @@
   // verification & rewriting
   {
     volatile oop init_lock = this_oop->init_lock();
-    ObjectLocker ol(init_lock, THREAD, init_lock != NULL);
+    ObjectLocker ol(init_lock, THREAD);
     // rewritten will have been set if loader constraint error found
     // on an earlier link attempt
     // don't verify or rewrite if already rewritten
@@ -743,7 +723,7 @@
   // Step 1
   {
     volatile oop init_lock = this_oop->init_lock();
-    ObjectLocker ol(init_lock, THREAD, init_lock != NULL);
+    ObjectLocker ol(init_lock, THREAD);
 
     Thread *self = THREAD; // it's passed the current thread
 
@@ -891,9 +871,8 @@
 
 void InstanceKlass::set_initialization_state_and_notify_impl(instanceKlassHandle this_oop, ClassState state, TRAPS) {
   volatile oop init_lock = this_oop->init_lock();
-  ObjectLocker ol(init_lock, THREAD, init_lock != NULL);
+  ObjectLocker ol(init_lock, THREAD);
   this_oop->set_init_state(state);
-  this_oop->fence_and_clear_init_lock();
   ol.notify_all(CHECK);
 }
 
@@ -2860,7 +2839,7 @@
   st->print(BULLET"protection domain: "); ((InstanceKlass*)this)->protection_domain()->print_value_on(st); st->cr();
   st->print(BULLET"host class:        "); host_klass()->print_value_on_maybe_null(st); st->cr();
   st->print(BULLET"signers:           "); signers()->print_value_on(st);               st->cr();
-  st->print(BULLET"init_lock:         "); ((oop)_init_lock)->print_value_on(st);             st->cr();
+  st->print(BULLET"init_lock:         "); ((oop)_init_lock)->print_value_on(st);       st->cr();
   if (source_file_name() != NULL) {
     st->print(BULLET"source file:       ");
     source_file_name()->print_value_on(st);
--- a/hotspot/src/share/vm/oops/instanceKlass.hpp	Thu Apr 25 03:58:53 2013 -0700
+++ b/hotspot/src/share/vm/oops/instanceKlass.hpp	Thu Apr 25 12:55:49 2013 -0700
@@ -184,8 +184,9 @@
   oop             _protection_domain;
   // Class signers.
   objArrayOop     _signers;
-  // Initialization lock.  Must be one per class and it has to be a VM internal
-  // object so java code cannot lock it (like the mirror)
+  // Lock for (1) initialization; (2) access to the ConstantPool of this class.
+  // Must be one per class and it has to be a VM internal object so java code
+  // cannot lock it (like the mirror).
   // It has to be an object not a Mutex because it's held through java calls.
   volatile oop    _init_lock;
 
@@ -970,6 +971,7 @@
 #endif // INCLUDE_ALL_GCS
 
   u2 idnum_allocated_count() const      { return _idnum_allocated_count; }
+
 private:
   // initialization state
 #ifdef ASSERT
@@ -996,9 +998,10 @@
          { OrderAccess::release_store_ptr(&_methods_cached_itable_indices, indices); }
 
   // Lock during initialization
-  volatile oop init_lock() const;
-  void set_init_lock(oop value)      { klass_oop_store(&_init_lock, value); }
-  void fence_and_clear_init_lock();  // after fully_initialized
+public:
+  volatile oop init_lock() const     {return _init_lock; }
+private:
+  void set_init_lock(oop value) { klass_oop_store(&_init_lock, value); }
 
   // Offsets for memory management
   oop* adr_protection_domain() const { return (oop*)&this->_protection_domain;}
--- a/hotspot/src/share/vm/prims/jvmtiEnv.cpp	Thu Apr 25 03:58:53 2013 -0700
+++ b/hotspot/src/share/vm/prims/jvmtiEnv.cpp	Thu Apr 25 12:55:49 2013 -0700
@@ -259,7 +259,8 @@
       // bytes to the InstanceKlass here because they have not been
       // validated and we're not at a safepoint.
       constantPoolHandle  constants(current_thread, ikh->constants());
-      MonitorLockerEx ml(constants->lock());    // lock constant pool while we query it
+      oop cplock = constants->lock();
+      ObjectLocker ol(cplock, current_thread, cplock != NULL);    // lock constant pool while we query it
 
       JvmtiClassFileReconstituter reconstituter(ikh);
       if (reconstituter.get_error() != JVMTI_ERROR_NONE) {
@@ -2417,7 +2418,8 @@
 
   instanceKlassHandle ikh(thread, k_oop);
   constantPoolHandle  constants(thread, ikh->constants());
-  MonitorLockerEx ml(constants->lock());    // lock constant pool while we query it
+  oop cplock = constants->lock();
+  ObjectLocker ol(cplock, thread, cplock != NULL);    // lock constant pool while we query it
 
   JvmtiConstantPoolReconstituter reconstituter(ikh);
   if (reconstituter.get_error() != JVMTI_ERROR_NONE) {