Use r/w Mutex to control access to jvmtiEnv event information.
We were previously relying on the mutator_lock_ to provide
synchronization when accessing control information for some jvmti
events. This was not fully thread safe. To fix this problem we
instead use a ReaderWriterMutex to control access to this data.
Refactorings to allow all methods to have appropriate lock annotations
will follow in later CL (see bug).
Performance Impact:
Tested performance by running main piece of test 993 10 times.
Printing redirected to /dev/null.
Total of ~270 breakpoints are executed.
Before: 2.62 seconds / run
After: 2.72 seconds / run
Test: ./test.py --host -j50
Test: ./art/tools/run-prebuilt-libjdwp-tests.sh --debug
Bug: 62821960
Bug: 67958496
Bug: 67962439
Change-Id: Ib2935c2e10bc2113e8430b49a9a7b76144e4235e
diff --git a/openjdkjvmti/OpenjdkJvmTi.cc b/openjdkjvmti/OpenjdkJvmTi.cc
index b30d45a..c2584e6 100644
--- a/openjdkjvmti/OpenjdkJvmTi.cc
+++ b/openjdkjvmti/OpenjdkJvmTi.cc
@@ -1676,7 +1676,8 @@
ArtJvmTiEnv::ArtJvmTiEnv(art::JavaVMExt* runtime, EventHandler* event_handler)
: art_vm(runtime),
local_data(nullptr),
- capabilities() {
+ capabilities(),
+ event_info_mutex_("jvmtiEnv_EventInfoMutex") {
object_tag_table = std::unique_ptr<ObjectTagTable>(new ObjectTagTable(event_handler, this));
functions = &gJvmtiInterface;
}
diff --git a/openjdkjvmti/art_jvmti.h b/openjdkjvmti/art_jvmti.h
index ad405e8..3edefaf 100644
--- a/openjdkjvmti/art_jvmti.h
+++ b/openjdkjvmti/art_jvmti.h
@@ -43,6 +43,7 @@
#include "base/logging.h"
#include "base/macros.h"
#include "base/strlcpy.h"
+#include "base/mutex.h"
#include "events.h"
#include "java_vm_ext.h"
#include "jni_env_ext.h"
@@ -59,6 +60,9 @@
class ObjectTagTable;
+// Make sure that the DEFAULT_MUTEX_ACQUIRED_AFTER macro works.
+using art::Locks;
+
// A structure that is a jvmtiEnv with additional information for the runtime.
struct ArtJvmTiEnv : public jvmtiEnv {
art::JavaVMExt* art_vm;
@@ -77,12 +81,15 @@
// or by putting a list in the ClassExt of a field's DeclaringClass.
// TODO Maybe just have an extension to let one put a watch on every field, that would probably be
// good enough maybe since you probably want either a few or all/almost all of them.
- std::unordered_set<art::ArtField*> access_watched_fields;
- std::unordered_set<art::ArtField*> modify_watched_fields;
+ std::unordered_set<art::ArtField*> access_watched_fields GUARDED_BY(event_info_mutex_);
+ std::unordered_set<art::ArtField*> modify_watched_fields GUARDED_BY(event_info_mutex_);
// Set of breakpoints is unique to each jvmtiEnv.
- std::unordered_set<Breakpoint> breakpoints;
- std::unordered_set<const art::ShadowFrame*> notify_frames;
+ std::unordered_set<Breakpoint> breakpoints GUARDED_BY(event_info_mutex_);
+ std::unordered_set<const art::ShadowFrame*> notify_frames GUARDED_BY(event_info_mutex_);
+
+ // RW lock to protect access to all of the event data.
+ art::ReaderWriterMutex event_info_mutex_ DEFAULT_MUTEX_ACQUIRED_AFTER;
ArtJvmTiEnv(art::JavaVMExt* runtime, EventHandler* event_handler);
diff --git a/openjdkjvmti/events-inl.h b/openjdkjvmti/events-inl.h
index ab8e6de..3973e94 100644
--- a/openjdkjvmti/events-inl.h
+++ b/openjdkjvmti/events-inl.h
@@ -21,6 +21,7 @@
#include <type_traits>
#include <tuple>
+#include "base/mutex-inl.h"
#include "events.h"
#include "jni_internal.h"
#include "nativehelper/ScopedLocalRef.h"
@@ -276,6 +277,7 @@
jthread jni_thread ATTRIBUTE_UNUSED,
jmethodID jmethod,
jlocation location) const {
+ art::ReaderMutexLock lk(art::Thread::Current(), env->event_info_mutex_);
art::ArtMethod* method = art::jni::DecodeArtMethod(jmethod);
return ShouldDispatchOnThread<ArtJvmtiEvent::kBreakpoint>(env, thread) &&
env->breakpoints.find({method, location}) != env->breakpoints.end();
@@ -292,6 +294,7 @@
const art::ShadowFrame* frame) const {
// Search for the frame. Do this before checking if we need to send the event so that we don't
// have to deal with use-after-free or the frames being reallocated later.
+ art::WriterMutexLock lk(art::Thread::Current(), env->event_info_mutex_);
return env->notify_frames.erase(frame) != 0 &&
ShouldDispatchOnThread<ArtJvmtiEvent::kFramePop>(env, thread);
}
@@ -313,6 +316,7 @@
jfieldID field,
char type_char ATTRIBUTE_UNUSED,
jvalue val ATTRIBUTE_UNUSED) const {
+ art::ReaderMutexLock lk(art::Thread::Current(), env->event_info_mutex_);
return ShouldDispatchOnThread<ArtJvmtiEvent::kFieldModification>(env, thread) &&
env->modify_watched_fields.find(
art::jni::DecodeArtField(field)) != env->modify_watched_fields.end();
@@ -329,6 +333,7 @@
jclass field_klass ATTRIBUTE_UNUSED,
jobject object ATTRIBUTE_UNUSED,
jfieldID field) const {
+ art::ReaderMutexLock lk(art::Thread::Current(), env->event_info_mutex_);
return ShouldDispatchOnThread<ArtJvmtiEvent::kFieldAccess>(env, thread) &&
env->access_watched_fields.find(
art::jni::DecodeArtField(field)) != env->access_watched_fields.end();
diff --git a/openjdkjvmti/ti_breakpoint.cc b/openjdkjvmti/ti_breakpoint.cc
index f5116a8..75c8027 100644
--- a/openjdkjvmti/ti_breakpoint.cc
+++ b/openjdkjvmti/ti_breakpoint.cc
@@ -36,6 +36,7 @@
#include "art_jvmti.h"
#include "art_method-inl.h"
#include "base/enums.h"
+#include "base/mutex-inl.h"
#include "dex_file_annotations.h"
#include "events-inl.h"
#include "jni_internal.h"
@@ -62,6 +63,7 @@
}
void BreakpointUtil::RemoveBreakpointsInClass(ArtJvmTiEnv* env, art::mirror::Class* klass) {
+ art::WriterMutexLock lk(art::Thread::Current(), env->event_info_mutex_);
std::vector<Breakpoint> to_remove;
for (const Breakpoint& b : env->breakpoints) {
if (b.GetMethod()->GetDeclaringClass() == klass) {
@@ -83,6 +85,7 @@
// Need to get mutator_lock_ so we can find the interface version of any default methods.
art::ScopedObjectAccess soa(art::Thread::Current());
art::ArtMethod* art_method = art::jni::DecodeArtMethod(method)->GetCanonicalMethod();
+ art::WriterMutexLock lk(art::Thread::Current(), env->event_info_mutex_);
if (location < 0 || static_cast<uint32_t>(location) >=
art_method->GetCodeItem()->insns_size_in_code_units_) {
return ERR(INVALID_LOCATION);
@@ -102,8 +105,9 @@
}
// Need to get mutator_lock_ so we can find the interface version of any default methods.
art::ScopedObjectAccess soa(art::Thread::Current());
- auto pos = env->breakpoints.find(
- /* Breakpoint */ {art::jni::DecodeArtMethod(method)->GetCanonicalMethod(), location});
+ art::ArtMethod* art_method = art::jni::DecodeArtMethod(method)->GetCanonicalMethod();
+ art::WriterMutexLock lk(art::Thread::Current(), env->event_info_mutex_);
+ auto pos = env->breakpoints.find(/* Breakpoint */ {art_method, location});
if (pos == env->breakpoints.end()) {
return ERR(NOT_FOUND);
}
diff --git a/openjdkjvmti/ti_field.cc b/openjdkjvmti/ti_field.cc
index c45b926..b869183 100644
--- a/openjdkjvmti/ti_field.cc
+++ b/openjdkjvmti/ti_field.cc
@@ -189,6 +189,7 @@
jvmtiError FieldUtil::SetFieldModificationWatch(jvmtiEnv* jenv, jclass klass, jfieldID field) {
ArtJvmTiEnv* env = ArtJvmTiEnv::AsArtJvmTiEnv(jenv);
+ art::WriterMutexLock lk(art::Thread::Current(), env->event_info_mutex_);
if (klass == nullptr) {
return ERR(INVALID_CLASS);
}
@@ -205,6 +206,7 @@
jvmtiError FieldUtil::ClearFieldModificationWatch(jvmtiEnv* jenv, jclass klass, jfieldID field) {
ArtJvmTiEnv* env = ArtJvmTiEnv::AsArtJvmTiEnv(jenv);
+ art::WriterMutexLock lk(art::Thread::Current(), env->event_info_mutex_);
if (klass == nullptr) {
return ERR(INVALID_CLASS);
}
@@ -221,6 +223,7 @@
jvmtiError FieldUtil::SetFieldAccessWatch(jvmtiEnv* jenv, jclass klass, jfieldID field) {
ArtJvmTiEnv* env = ArtJvmTiEnv::AsArtJvmTiEnv(jenv);
+ art::WriterMutexLock lk(art::Thread::Current(), env->event_info_mutex_);
if (klass == nullptr) {
return ERR(INVALID_CLASS);
}
@@ -237,6 +240,7 @@
jvmtiError FieldUtil::ClearFieldAccessWatch(jvmtiEnv* jenv, jclass klass, jfieldID field) {
ArtJvmTiEnv* env = ArtJvmTiEnv::AsArtJvmTiEnv(jenv);
+ art::WriterMutexLock lk(art::Thread::Current(), env->event_info_mutex_);
if (klass == nullptr) {
return ERR(INVALID_CLASS);
}
diff --git a/openjdkjvmti/ti_field.h b/openjdkjvmti/ti_field.h
index 8a229ed..3cf29f0 100644
--- a/openjdkjvmti/ti_field.h
+++ b/openjdkjvmti/ti_field.h
@@ -35,6 +35,8 @@
#include "jni.h"
#include "jvmti.h"
+#include "art_jvmti.h"
+
namespace openjdkjvmti {
class FieldUtil {
@@ -61,10 +63,14 @@
jfieldID field,
jboolean* is_synthetic_ptr);
- static jvmtiError SetFieldModificationWatch(jvmtiEnv* env, jclass klass, jfieldID field);
- static jvmtiError ClearFieldModificationWatch(jvmtiEnv* env, jclass klass, jfieldID field);
- static jvmtiError SetFieldAccessWatch(jvmtiEnv* env, jclass klass, jfieldID field);
- static jvmtiError ClearFieldAccessWatch(jvmtiEnv* env, jclass klass, jfieldID field);
+ static jvmtiError SetFieldModificationWatch(jvmtiEnv* env, jclass klass, jfieldID field)
+ REQUIRES(!ArtJvmTiEnv::event_info_mutex_);
+ static jvmtiError ClearFieldModificationWatch(jvmtiEnv* env, jclass klass, jfieldID field)
+ REQUIRES(!ArtJvmTiEnv::event_info_mutex_);
+ static jvmtiError SetFieldAccessWatch(jvmtiEnv* env, jclass klass, jfieldID field)
+ REQUIRES(!ArtJvmTiEnv::event_info_mutex_);
+ static jvmtiError ClearFieldAccessWatch(jvmtiEnv* env, jclass klass, jfieldID field)
+ REQUIRES(!ArtJvmTiEnv::event_info_mutex_);
};
} // namespace openjdkjvmti
diff --git a/openjdkjvmti/ti_stack.cc b/openjdkjvmti/ti_stack.cc
index e0c1399..23cf659 100644
--- a/openjdkjvmti/ti_stack.cc
+++ b/openjdkjvmti/ti_stack.cc
@@ -1024,9 +1024,12 @@
method,
visitor.GetDexPc());
}
- // Mark shadow frame as needs_notify_pop_
- shadow_frame->SetNotifyPop(true);
- tienv->notify_frames.insert(shadow_frame);
+ {
+ art::WriterMutexLock lk(self, tienv->event_info_mutex_);
+ // Mark shadow frame as needs_notify_pop_
+ shadow_frame->SetNotifyPop(true);
+ tienv->notify_frames.insert(shadow_frame);
+ }
// Make sure can we will go to the interpreter and use the shadow frames.
if (needs_instrument) {
art::Runtime::Current()->GetInstrumentation()->InstrumentThreadStack(target);