8014910: deadlock between JVM/TI ClassPrepare event handler and CompilerThread
Summary: Revert changes in JDK-8008962
Reviewed-by: coleenp, sspitsyn
--- a/hotspot/src/share/vm/ci/ciEnv.cpp Mon Oct 21 17:26:46 2013 -0700
+++ b/hotspot/src/share/vm/ci/ciEnv.cpp Tue Oct 22 14:29:02 2013 -0700
@@ -483,8 +483,7 @@
{
// We have to lock the cpool to keep the oop from being resolved
// while we are accessing it.
- oop cplock = cpool->lock();
- ObjectLocker ol(cplock, THREAD, cplock != NULL);
+ MonitorLockerEx ml(cpool->lock());
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 Mon Oct 21 17:26:46 2013 -0700
+++ b/hotspot/src/share/vm/oops/constantPool.cpp Tue Oct 22 14:29:02 2013 -0700
@@ -40,7 +40,6 @@
#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) {
@@ -70,6 +69,7 @@
// 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,6 +95,9 @@
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 {
@@ -151,6 +154,9 @@
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"));
}
}
@@ -161,23 +167,7 @@
set_resolved_reference_length(
resolved_references() != NULL ? resolved_references()->length() : 0);
set_resolved_references(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;
- }
+ set_lock(NULL);
}
int ConstantPool::cp_to_object_index(int cp_index) {
@@ -211,9 +201,7 @@
Symbol* name = NULL;
Handle loader;
- {
- oop cplock = this_oop->lock();
- ObjectLocker ol(cplock , THREAD, cplock != NULL);
+ { MonitorLockerEx ml(this_oop->lock());
if (this_oop->tag_at(which).is_unresolved_klass()) {
if (this_oop->tag_at(which).is_unresolved_klass_in_error()) {
@@ -260,8 +248,7 @@
bool throw_orig_error = false;
{
- oop cplock = this_oop->lock();
- ObjectLocker ol(cplock, THREAD, cplock != NULL);
+ MonitorLockerEx ml(this_oop->lock());
// some other thread has beaten us and has resolved the class.
if (this_oop->tag_at(which).is_klass()) {
@@ -329,8 +316,7 @@
}
return k();
} else {
- oop cplock = this_oop->lock();
- ObjectLocker ol(cplock, THREAD, cplock != NULL);
+ MonitorLockerEx ml(this_oop->lock());
// Only updated constant pool - if it is resolved.
do_resolve = this_oop->tag_at(which).is_unresolved_klass();
if (do_resolve) {
@@ -600,8 +586,7 @@
int tag, TRAPS) {
ResourceMark rm;
Symbol* error = PENDING_EXCEPTION->klass()->name();
- oop cplock = this_oop->lock();
- ObjectLocker ol(cplock, THREAD, cplock != NULL); // lock cpool to change tag.
+ MonitorLockerEx ml(this_oop->lock()); // lock cpool to change tag.
int error_tag = (tag == JVM_CONSTANT_MethodHandle) ?
JVM_CONSTANT_MethodHandleInError : JVM_CONSTANT_MethodTypeInError;
@@ -762,8 +747,7 @@
if (cache_index >= 0) {
// Cache the oop here also.
Handle result_handle(THREAD, result_oop);
- oop cplock = this_oop->lock();
- ObjectLocker ol(cplock, THREAD, cplock != NULL); // don't know if we really need this
+ MonitorLockerEx ml(this_oop->lock()); // 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 Mon Oct 21 17:26:46 2013 -0700
+++ b/hotspot/src/share/vm/oops/constantPool.hpp Tue Oct 22 14:29:02 2013 -0700
@@ -111,6 +111,7 @@
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); }
@@ -843,17 +844,8 @@
void set_resolved_reference_length(int length) { _saved._resolved_reference_length = length; }
int resolved_reference_length() const { return _saved._resolved_reference_length; }
-
- // 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();
+ void set_lock(Monitor* lock) { _lock = lock; }
+ Monitor* lock() { return _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 Mon Oct 21 17:26:46 2013 -0700
+++ b/hotspot/src/share/vm/oops/cpCache.cpp Tue Oct 22 14:29:02 2013 -0700
@@ -284,8 +284,7 @@
// the lock, so that when the losing writer returns, he can use the linked
// cache entry.
- oop cplock = cpool->lock();
- ObjectLocker ol(cplock, Thread::current(), cplock != NULL);
+ MonitorLockerEx ml(cpool->lock());
if (!is_f1_null()) {
return;
}
--- a/hotspot/src/share/vm/oops/instanceKlass.cpp Mon Oct 21 17:26:46 2013 -0700
+++ b/hotspot/src/share/vm/oops/instanceKlass.cpp Tue Oct 22 14:29:02 2013 -0700
@@ -498,13 +498,27 @@
oop InstanceKlass::init_lock() const {
// return the init lock from the mirror
- return java_lang_Class::init_lock(java_mirror());
+ oop lock = java_lang_Class::init_lock(java_mirror());
+ 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();
+ java_lang_Class::set_init_lock(java_mirror(), NULL);
+ assert(!is_not_initialized(), "class must be initialized now");
}
void InstanceKlass::eager_initialize_impl(instanceKlassHandle this_oop) {
EXCEPTION_MARK;
oop init_lock = this_oop->init_lock();
- ObjectLocker ol(init_lock, THREAD);
+ ObjectLocker ol(init_lock, THREAD, init_lock != NULL);
// abort if someone beat us to the initialization
if (!this_oop->is_not_initialized()) return; // note: not equivalent to is_initialized()
@@ -523,6 +537,7 @@
} 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);
@@ -649,7 +664,7 @@
// verification & rewriting
{
oop init_lock = this_oop->init_lock();
- ObjectLocker ol(init_lock, THREAD);
+ ObjectLocker ol(init_lock, THREAD, init_lock != NULL);
// rewritten will have been set if loader constraint error found
// on an earlier link attempt
// don't verify or rewrite if already rewritten
@@ -772,7 +787,7 @@
// Step 1
{
oop init_lock = this_oop->init_lock();
- ObjectLocker ol(init_lock, THREAD);
+ ObjectLocker ol(init_lock, THREAD, init_lock != NULL);
Thread *self = THREAD; // it's passed the current thread
@@ -920,8 +935,9 @@
void InstanceKlass::set_initialization_state_and_notify_impl(instanceKlassHandle this_oop, ClassState state, TRAPS) {
oop init_lock = this_oop->init_lock();
- ObjectLocker ol(init_lock, THREAD);
+ ObjectLocker ol(init_lock, THREAD, init_lock != NULL);
this_oop->set_init_state(state);
+ this_oop->fence_and_clear_init_lock();
ol.notify_all(CHECK);
}
--- a/hotspot/src/share/vm/oops/instanceKlass.hpp Mon Oct 21 17:26:46 2013 -0700
+++ b/hotspot/src/share/vm/oops/instanceKlass.hpp Tue Oct 22 14:29:02 2013 -0700
@@ -1023,6 +1023,7 @@
// It has to be an object not a Mutex because it's held through java calls.
oop init_lock() const;
private:
+ void fence_and_clear_init_lock();
// Static methods that are used to implement member methods where an exposed this pointer
// is needed due to possible GCs
--- a/hotspot/src/share/vm/prims/jvmtiEnv.cpp Mon Oct 21 17:26:46 2013 -0700
+++ b/hotspot/src/share/vm/prims/jvmtiEnv.cpp Tue Oct 22 14:29:02 2013 -0700
@@ -259,8 +259,7 @@
// bytes to the InstanceKlass here because they have not been
// validated and we're not at a safepoint.
constantPoolHandle constants(current_thread, ikh->constants());
- oop cplock = constants->lock();
- ObjectLocker ol(cplock, current_thread, cplock != NULL); // lock constant pool while we query it
+ MonitorLockerEx ml(constants->lock()); // lock constant pool while we query it
JvmtiClassFileReconstituter reconstituter(ikh);
if (reconstituter.get_error() != JVMTI_ERROR_NONE) {
@@ -2418,8 +2417,7 @@
instanceKlassHandle ikh(thread, k_oop);
constantPoolHandle constants(thread, ikh->constants());
- oop cplock = constants->lock();
- ObjectLocker ol(cplock, thread, cplock != NULL); // lock constant pool while we query it
+ MonitorLockerEx ml(constants->lock()); // lock constant pool while we query it
JvmtiConstantPoolReconstituter reconstituter(ikh);
if (reconstituter.get_error() != JVMTI_ERROR_NONE) {