From 59d9d668d4f4286813afe2b4e7c6db839222ce96 Mon Sep 17 00:00:00 2001 From: Sebastien Hertz Date: Tue, 19 Aug 2014 15:33:43 +0200 Subject: [PATCH] Reduce lock contention when debugging Uses a ReaderWriterMutex for the breakpoint lock to reduce contention during debugging session. Also adds missing thread safety annotations on fields and methods related to instrumentation and debugging. Bug: 16814665 Bug: 11667502 Change-Id: I056cdafa91109e0c83806c8d8df75c37ade0a354 --- runtime/base/mutex.cc | 4 ++-- runtime/base/mutex.h | 2 +- runtime/debugger.cc | 14 +++++++------- runtime/instrumentation.h | 35 ++++++++++++++++++----------------- runtime/thread_list.cc | 2 +- 5 files changed, 29 insertions(+), 28 deletions(-) diff --git a/runtime/base/mutex.cc b/runtime/base/mutex.cc index c0a865fd0..80fb9ea81 100644 --- a/runtime/base/mutex.cc +++ b/runtime/base/mutex.cc @@ -32,7 +32,7 @@ namespace art { Mutex* Locks::abort_lock_ = nullptr; Mutex* Locks::allocated_monitor_ids_lock_ = nullptr; Mutex* Locks::allocated_thread_ids_lock_ = nullptr; -Mutex* Locks::breakpoint_lock_ = nullptr; +ReaderWriterMutex* Locks::breakpoint_lock_ = nullptr; ReaderWriterMutex* Locks::classlinker_classes_lock_ = nullptr; ReaderWriterMutex* Locks::heap_bitmap_lock_ = nullptr; Mutex* Locks::logging_lock_ = nullptr; @@ -880,7 +880,7 @@ void Locks::Init() { UPDATE_CURRENT_LOCK_LEVEL(kBreakpointLock); DCHECK(breakpoint_lock_ == nullptr); - breakpoint_lock_ = new Mutex("breakpoint lock", current_lock_level); + breakpoint_lock_ = new ReaderWriterMutex("breakpoint lock", current_lock_level); UPDATE_CURRENT_LOCK_LEVEL(kClassLinkerClassesLock); DCHECK(classlinker_classes_lock_ == nullptr); diff --git a/runtime/base/mutex.h b/runtime/base/mutex.h index d898e49df..8d89f963a 100644 --- a/runtime/base/mutex.h +++ b/runtime/base/mutex.h @@ -562,7 +562,7 @@ class Locks { static Mutex* thread_list_lock_ ACQUIRED_AFTER(trace_lock_); // Guards breakpoints. - static Mutex* breakpoint_lock_ ACQUIRED_AFTER(thread_list_lock_); + static ReaderWriterMutex* breakpoint_lock_ ACQUIRED_AFTER(thread_list_lock_); // Guards lists of classes within the class linker. static ReaderWriterMutex* classlinker_classes_lock_ ACQUIRED_AFTER(breakpoint_lock_); diff --git a/runtime/debugger.cc b/runtime/debugger.cc index 6d2f21e1e..6f431c3c6 100644 --- a/runtime/debugger.cc +++ b/runtime/debugger.cc @@ -209,7 +209,7 @@ class Breakpoint { bool need_full_deoptimization_; }; -static std::ostream& operator<<(std::ostream& os, Breakpoint& rhs) +static std::ostream& operator<<(std::ostream& os, const Breakpoint& rhs) SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) { os << StringPrintf("Breakpoint[%s @%#x]", PrettyMethod(rhs.Method()).c_str(), rhs.DexPc()); return os; @@ -373,7 +373,7 @@ void SingleStepControl::Clear() { static bool IsBreakpoint(const mirror::ArtMethod* m, uint32_t dex_pc) LOCKS_EXCLUDED(Locks::breakpoint_lock_) SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) { - MutexLock mu(Thread::Current(), *Locks::breakpoint_lock_); + ReaderMutexLock mu(Thread::Current(), *Locks::breakpoint_lock_); for (size_t i = 0, e = gBreakpoints.size(); i < e; ++i) { if (gBreakpoints[i].DexPc() == dex_pc && gBreakpoints[i].Method() == m) { VLOG(jdwp) << "Hit breakpoint #" << i << ": " << gBreakpoints[i]; @@ -740,7 +740,7 @@ void Dbg::GoActive() { { // TODO: dalvik only warned if there were breakpoints left over. clear in Dbg::Disconnected? - MutexLock mu(Thread::Current(), *Locks::breakpoint_lock_); + ReaderMutexLock mu(Thread::Current(), *Locks::breakpoint_lock_); CHECK_EQ(gBreakpoints.size(), 0U); } @@ -3043,7 +3043,7 @@ static bool IsMethodPossiblyInlined(Thread* self, mirror::ArtMethod* m) } static const Breakpoint* FindFirstBreakpointForMethod(mirror::ArtMethod* m) - EXCLUSIVE_LOCKS_REQUIRED(Locks::breakpoint_lock_) SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) { + SHARED_LOCKS_REQUIRED(Locks::mutator_lock_, Locks::breakpoint_lock_) { for (Breakpoint& breakpoint : gBreakpoints) { if (breakpoint.Method() == m) { return &breakpoint; @@ -3054,7 +3054,7 @@ static const Breakpoint* FindFirstBreakpointForMethod(mirror::ArtMethod* m) // Sanity checks all existing breakpoints on the same method. static void SanityCheckExistingBreakpoints(mirror::ArtMethod* m, bool need_full_deoptimization) - EXCLUSIVE_LOCKS_REQUIRED(Locks::breakpoint_lock_) SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) { + SHARED_LOCKS_REQUIRED(Locks::mutator_lock_, Locks::breakpoint_lock_) { if (kIsDebugBuild) { for (const Breakpoint& breakpoint : gBreakpoints) { CHECK_EQ(need_full_deoptimization, breakpoint.NeedFullDeoptimization()); @@ -3079,7 +3079,7 @@ void Dbg::WatchLocation(const JDWP::JdwpLocation* location, DeoptimizationReques mirror::ArtMethod* m = FromMethodId(location->method_id); DCHECK(m != nullptr) << "No method for method id " << location->method_id; - MutexLock mu(self, *Locks::breakpoint_lock_); + WriterMutexLock mu(self, *Locks::breakpoint_lock_); const Breakpoint* const existing_breakpoint = FindFirstBreakpointForMethod(m); bool need_full_deoptimization; if (existing_breakpoint == nullptr) { @@ -3110,7 +3110,7 @@ void Dbg::WatchLocation(const JDWP::JdwpLocation* location, DeoptimizationReques // Uninstalls a breakpoint at the specified location. Also indicates through the deoptimization // request if we need to undeoptimize. void Dbg::UnwatchLocation(const JDWP::JdwpLocation* location, DeoptimizationRequest* req) { - MutexLock mu(Thread::Current(), *Locks::breakpoint_lock_); + WriterMutexLock mu(Thread::Current(), *Locks::breakpoint_lock_); mirror::ArtMethod* m = FromMethodId(location->method_id); DCHECK(m != nullptr) << "No method for method id " << location->method_id; bool need_full_deoptimization = false; diff --git a/runtime/instrumentation.h b/runtime/instrumentation.h index 21d11a5e1..d05cee5dc 100644 --- a/runtime/instrumentation.h +++ b/runtime/instrumentation.h @@ -177,7 +177,8 @@ class Instrumentation { EXCLUSIVE_LOCKS_REQUIRED(Locks::mutator_lock_) LOCKS_EXCLUDED(Locks::thread_list_lock_, Locks::classlinker_classes_lock_); - InterpreterHandlerTable GetInterpreterHandlerTable() const { + InterpreterHandlerTable GetInterpreterHandlerTable() const + SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) { return interpreter_handler_table_; } @@ -220,31 +221,31 @@ class Instrumentation { return instrumentation_stubs_installed_; } - bool HasMethodEntryListeners() const { + bool HasMethodEntryListeners() const SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) { return have_method_entry_listeners_; } - bool HasMethodExitListeners() const { + bool HasMethodExitListeners() const SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) { return have_method_exit_listeners_; } - bool HasDexPcListeners() const { + bool HasDexPcListeners() const SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) { return have_dex_pc_listeners_; } - bool HasFieldReadListeners() const { + bool HasFieldReadListeners() const SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) { return have_field_read_listeners_; } - bool HasFieldWriteListeners() const { + bool HasFieldWriteListeners() const SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) { return have_field_write_listeners_; } - bool HasExceptionCaughtListeners() const { + bool HasExceptionCaughtListeners() const SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) { return have_exception_caught_listeners_; } - bool IsActive() const { + bool IsActive() const SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) { return have_dex_pc_listeners_ || have_method_entry_listeners_ || have_method_exit_listeners_ || have_field_read_listeners_ || have_field_write_listeners_ || have_exception_caught_listeners_ || have_method_unwind_listeners_; @@ -343,7 +344,7 @@ class Instrumentation { LOCKS_EXCLUDED(Locks::thread_list_lock_, Locks::classlinker_classes_lock_, deoptimized_methods_lock_); - void UpdateInterpreterHandlerTable() { + void UpdateInterpreterHandlerTable() EXCLUSIVE_LOCKS_REQUIRED(Locks::mutator_lock_) { interpreter_handler_table_ = IsActive() ? kAlternativeHandlerTable : kMainHandlerTable; } @@ -404,30 +405,30 @@ class Instrumentation { // Do we have any listeners for method entry events? Short-cut to avoid taking the // instrumentation_lock_. - bool have_method_entry_listeners_; + bool have_method_entry_listeners_ GUARDED_BY(Locks::mutator_lock_); // Do we have any listeners for method exit events? Short-cut to avoid taking the // instrumentation_lock_. - bool have_method_exit_listeners_; + bool have_method_exit_listeners_ GUARDED_BY(Locks::mutator_lock_); // Do we have any listeners for method unwind events? Short-cut to avoid taking the // instrumentation_lock_. - bool have_method_unwind_listeners_; + bool have_method_unwind_listeners_ GUARDED_BY(Locks::mutator_lock_); // Do we have any listeners for dex move events? Short-cut to avoid taking the // instrumentation_lock_. - bool have_dex_pc_listeners_; + bool have_dex_pc_listeners_ GUARDED_BY(Locks::mutator_lock_); // Do we have any listeners for field read events? Short-cut to avoid taking the // instrumentation_lock_. - bool have_field_read_listeners_; + bool have_field_read_listeners_ GUARDED_BY(Locks::mutator_lock_); // Do we have any listeners for field write events? Short-cut to avoid taking the // instrumentation_lock_. - bool have_field_write_listeners_; + bool have_field_write_listeners_ GUARDED_BY(Locks::mutator_lock_); // Do we have any exception caught listeners? Short-cut to avoid taking the instrumentation_lock_. - bool have_exception_caught_listeners_; + bool have_exception_caught_listeners_ GUARDED_BY(Locks::mutator_lock_); // The event listeners, written to with the mutator_lock_ exclusively held. std::list method_entry_listeners_ GUARDED_BY(Locks::mutator_lock_); @@ -451,7 +452,7 @@ class Instrumentation { // Current interpreter handler table. This is updated each time the thread state flags are // modified. - InterpreterHandlerTable interpreter_handler_table_; + InterpreterHandlerTable interpreter_handler_table_ GUARDED_BY(Locks::mutator_lock_); // Greater than 0 if quick alloc entry points instrumented. // TODO: The access and changes to this is racy and should be guarded by a lock. diff --git a/runtime/thread_list.cc b/runtime/thread_list.cc index 7cf26e664..9d85ec36c 100644 --- a/runtime/thread_list.cc +++ b/runtime/thread_list.cc @@ -625,7 +625,7 @@ void ThreadList::SuspendAllForDebugger() { #endif AssertThreadsAreSuspended(self, self, debug_thread); - VLOG(threads) << *self << " SuspendAll complete"; + VLOG(threads) << *self << " SuspendAllForDebugger complete"; } void ThreadList::SuspendSelfForDebugger() { -- 2.11.0