From: Daniel Mihalyi Date: Mon, 18 Aug 2014 16:45:31 +0000 (+0200) Subject: Optimized instrumentation listener handling X-Git-Tag: android-x86-6.0-r1~145^2~2^2~285 X-Git-Url: http://git.osdn.net/view?a=commitdiff_plain;h=96add97815ba8418fb4e8c0fc08dbf9c7198f244;p=android-x86%2Fart.git Optimized instrumentation listener handling Some instrumentation listener lists may be modified while iterating over the list to deliver an instrumentation event. Therefore the previous implementation copied the list of listeners before starting the iteration. This new implementation only copies the list of instrumentation listeners when the list is changed. Instances of the list are reference counted using std::shared_ptr<>. Bug: 16814665 (cherry picked from commit ca1d06cfa2f2b8d2be4390644e126cb68cdbb5ba) Change-Id: Ib2e6b980de85b75f1c8f4a8825bdc7767154663a --- diff --git a/runtime/instrumentation.cc b/runtime/instrumentation.cc index 5988d71f4..47d35f350 100644 --- a/runtime/instrumentation.cc +++ b/runtime/instrumentation.cc @@ -210,7 +210,8 @@ static void InstrumentationInstallStack(Thread* thread, void* arg) LOG(INFO) << " Handling quick to interpreter transition. Frame " << GetFrameId(); } CHECK_LT(instrumentation_stack_depth_, instrumentation_stack_->size()); - const InstrumentationStackFrame& frame = instrumentation_stack_->at(instrumentation_stack_depth_); + const InstrumentationStackFrame& frame = + instrumentation_stack_->at(instrumentation_stack_depth_); CHECK(frame.interpreter_entry_); // This is an interpreter frame so method enter event must have been reported. However we // need to push a DEX pc into the dex_pcs_ list to match size of instrumentation stack. @@ -236,7 +237,8 @@ static void InstrumentationInstallStack(Thread* thread, void* arg) reached_existing_instrumentation_frames_ = true; CHECK_LT(instrumentation_stack_depth_, instrumentation_stack_->size()); - const InstrumentationStackFrame& frame = instrumentation_stack_->at(instrumentation_stack_depth_); + const InstrumentationStackFrame& frame = + instrumentation_stack_->at(instrumentation_stack_depth_); CHECK_EQ(m, frame.method_) << "Expected " << PrettyMethod(m) << ", Found " << PrettyMethod(frame.method_); return_pc = frame.return_pc_; @@ -329,7 +331,8 @@ static void InstrumentationRestoreStack(Thread* thread, void* arg) mirror::ArtMethod* m = GetMethod(); if (GetCurrentQuickFrame() == NULL) { if (kVerboseInstrumentation) { - LOG(INFO) << " Ignoring a shadow frame. Frame " << GetFrameId() << " Method=" << PrettyMethod(m); + LOG(INFO) << " Ignoring a shadow frame. Frame " << GetFrameId() + << " Method=" << PrettyMethod(m); } return true; // Ignore shadow frames. } @@ -410,19 +413,47 @@ void Instrumentation::AddListener(InstrumentationListener* listener, uint32_t ev have_method_unwind_listeners_ = true; } if ((events & kDexPcMoved) != 0) { - dex_pc_listeners_.push_back(listener); + std::list* modified; + if (have_dex_pc_listeners_) { + modified = new std::list(*dex_pc_listeners_.get()); + } else { + modified = new std::list(); + } + modified->push_back(listener); + dex_pc_listeners_.reset(modified); have_dex_pc_listeners_ = true; } if ((events & kFieldRead) != 0) { - field_read_listeners_.push_back(listener); + std::list* modified; + if (have_field_read_listeners_) { + modified = new std::list(*field_read_listeners_.get()); + } else { + modified = new std::list(); + } + modified->push_back(listener); + field_read_listeners_.reset(modified); have_field_read_listeners_ = true; } if ((events & kFieldWritten) != 0) { - field_write_listeners_.push_back(listener); + std::list* modified; + if (have_field_write_listeners_) { + modified = new std::list(*field_write_listeners_.get()); + } else { + modified = new std::list(); + } + modified->push_back(listener); + field_write_listeners_.reset(modified); have_field_write_listeners_ = true; } if ((events & kExceptionCaught) != 0) { - exception_caught_listeners_.push_back(listener); + std::list* modified; + if (have_exception_caught_listeners_) { + modified = new std::list(*exception_caught_listeners_.get()); + } else { + modified = new std::list(); + } + modified->push_back(listener); + exception_caught_listeners_.reset(modified); have_exception_caught_listeners_ = true; } UpdateInterpreterHandlerTable(); @@ -432,51 +463,78 @@ void Instrumentation::RemoveListener(InstrumentationListener* listener, uint32_t Locks::mutator_lock_->AssertExclusiveHeld(Thread::Current()); if ((events & kMethodEntered) != 0) { - bool contains = std::find(method_entry_listeners_.begin(), method_entry_listeners_.end(), - listener) != method_entry_listeners_.end(); - if (contains) { + if (have_method_entry_listeners_) { method_entry_listeners_.remove(listener); + have_method_entry_listeners_ = !method_entry_listeners_.empty(); } - have_method_entry_listeners_ = method_entry_listeners_.size() > 0; } if ((events & kMethodExited) != 0) { - bool contains = std::find(method_exit_listeners_.begin(), method_exit_listeners_.end(), - listener) != method_exit_listeners_.end(); - if (contains) { + if (have_method_exit_listeners_) { method_exit_listeners_.remove(listener); + have_method_exit_listeners_ = !method_exit_listeners_.empty(); } - have_method_exit_listeners_ = method_exit_listeners_.size() > 0; } if ((events & kMethodUnwind) != 0) { - method_unwind_listeners_.remove(listener); + if (have_method_unwind_listeners_) { + method_unwind_listeners_.remove(listener); + have_method_unwind_listeners_ = !method_unwind_listeners_.empty(); + } } if ((events & kDexPcMoved) != 0) { - bool contains = std::find(dex_pc_listeners_.begin(), dex_pc_listeners_.end(), - listener) != dex_pc_listeners_.end(); - if (contains) { - dex_pc_listeners_.remove(listener); + if (have_dex_pc_listeners_) { + std::list* modified = + new std::list(*dex_pc_listeners_.get()); + modified->remove(listener); + have_dex_pc_listeners_ = !modified->empty(); + if (have_dex_pc_listeners_) { + dex_pc_listeners_.reset(modified); + } else { + dex_pc_listeners_.reset(); + delete modified; + } } - have_dex_pc_listeners_ = dex_pc_listeners_.size() > 0; } if ((events & kFieldRead) != 0) { - bool contains = std::find(field_read_listeners_.begin(), field_read_listeners_.end(), - listener) != field_read_listeners_.end(); - if (contains) { - field_read_listeners_.remove(listener); + if (have_dex_pc_listeners_) { + std::list* modified = + new std::list(*field_read_listeners_.get()); + modified->remove(listener); + have_field_read_listeners_ = !modified->empty(); + if (have_field_read_listeners_) { + field_read_listeners_.reset(modified); + } else { + field_read_listeners_.reset(); + delete modified; + } } - have_field_read_listeners_ = field_read_listeners_.size() > 0; } if ((events & kFieldWritten) != 0) { - bool contains = std::find(field_write_listeners_.begin(), field_write_listeners_.end(), - listener) != field_write_listeners_.end(); - if (contains) { - field_write_listeners_.remove(listener); + if (have_field_write_listeners_) { + std::list* modified = + new std::list(*field_write_listeners_.get()); + modified->remove(listener); + have_field_write_listeners_ = !modified->empty(); + if (have_field_write_listeners_) { + field_write_listeners_.reset(modified); + } else { + field_write_listeners_.reset(); + delete modified; + } } - have_field_write_listeners_ = field_write_listeners_.size() > 0; } if ((events & kExceptionCaught) != 0) { - exception_caught_listeners_.remove(listener); - have_exception_caught_listeners_ = exception_caught_listeners_.size() > 0; + if (have_exception_caught_listeners_) { + std::list* modified = + new std::list(*exception_caught_listeners_.get()); + modified->remove(listener); + have_exception_caught_listeners_ = !modified->empty(); + if (have_exception_caught_listeners_) { + exception_caught_listeners_.reset(modified); + } else { + exception_caught_listeners_.reset(); + delete modified; + } + } } UpdateInterpreterHandlerTable(); } @@ -689,7 +747,8 @@ void Instrumentation::Deoptimize(mirror::ArtMethod* method) { { WriterMutexLock mu(self, deoptimized_methods_lock_); bool has_not_been_deoptimized = AddDeoptimizedMethod(method); - CHECK(has_not_been_deoptimized) << "Method " << PrettyMethod(method) << " is already deoptimized"; + CHECK(has_not_been_deoptimized) << "Method " << PrettyMethod(method) + << " is already deoptimized"; } if (!interpreter_stubs_installed_) { UpdateEntrypoints(method, GetQuickInstrumentationEntryPoint(), GetPortableToInterpreterBridge(), @@ -858,33 +917,33 @@ void Instrumentation::MethodUnwindEvent(Thread* thread, mirror::Object* this_obj void Instrumentation::DexPcMovedEventImpl(Thread* thread, mirror::Object* this_object, mirror::ArtMethod* method, uint32_t dex_pc) const { - // TODO: STL copy-on-write collection? The copy below is due to the debug listener having an - // action where it can remove itself as a listener and break the iterator. The copy only works - // around the problem and in general we may have to move to something like reference counting to - // ensure listeners are deleted correctly. - std::list copy(dex_pc_listeners_); - for (InstrumentationListener* listener : copy) { - listener->DexPcMoved(thread, this_object, method, dex_pc); + if (HasDexPcListeners()) { + std::shared_ptr> original(dex_pc_listeners_); + for (InstrumentationListener* listener : *original.get()) { + listener->DexPcMoved(thread, this_object, method, dex_pc); + } } } void Instrumentation::FieldReadEventImpl(Thread* thread, mirror::Object* this_object, mirror::ArtMethod* method, uint32_t dex_pc, mirror::ArtField* field) const { - // TODO: same comment than DexPcMovedEventImpl. - std::list copy(field_read_listeners_); - for (InstrumentationListener* listener : copy) { - listener->FieldRead(thread, this_object, method, dex_pc, field); + if (HasFieldReadListeners()) { + std::shared_ptr> original(field_read_listeners_); + for (InstrumentationListener* listener : *original.get()) { + listener->FieldRead(thread, this_object, method, dex_pc, field); + } } } void Instrumentation::FieldWriteEventImpl(Thread* thread, mirror::Object* this_object, mirror::ArtMethod* method, uint32_t dex_pc, mirror::ArtField* field, const JValue& field_value) const { - // TODO: same comment than DexPcMovedEventImpl. - std::list copy(field_write_listeners_); - for (InstrumentationListener* listener : copy) { - listener->FieldWritten(thread, this_object, method, dex_pc, field, field_value); + if (HasFieldWriteListeners()) { + std::shared_ptr> original(field_write_listeners_); + for (InstrumentationListener* listener : *original.get()) { + listener->FieldWritten(thread, this_object, method, dex_pc, field, field_value); + } } } @@ -896,11 +955,10 @@ void Instrumentation::ExceptionCaughtEvent(Thread* thread, const ThrowLocation& DCHECK_EQ(thread->GetException(nullptr), exception_object); bool is_exception_reported = thread->IsExceptionReportedToInstrumentation(); thread->ClearException(); - // TODO: The copy below is due to the debug listener having an action where it can remove - // itself as a listener and break the iterator. The copy only works around the problem. - std::list copy(exception_caught_listeners_); - for (InstrumentationListener* listener : copy) { - listener->ExceptionCaught(thread, throw_location, catch_method, catch_dex_pc, exception_object); + std::shared_ptr> original(exception_caught_listeners_); + for (InstrumentationListener* listener : *original.get()) { + listener->ExceptionCaught(thread, throw_location, catch_method, catch_dex_pc, + exception_object); } thread->SetException(throw_location, exception_object); thread->SetExceptionReportedToInstrumentation(is_exception_reported); diff --git a/runtime/instrumentation.h b/runtime/instrumentation.h index 66c6b388d..21d11a5e1 100644 --- a/runtime/instrumentation.h +++ b/runtime/instrumentation.h @@ -433,10 +433,14 @@ class Instrumentation { std::list method_entry_listeners_ GUARDED_BY(Locks::mutator_lock_); std::list method_exit_listeners_ GUARDED_BY(Locks::mutator_lock_); std::list method_unwind_listeners_ GUARDED_BY(Locks::mutator_lock_); - std::list dex_pc_listeners_ GUARDED_BY(Locks::mutator_lock_); - std::list field_read_listeners_ GUARDED_BY(Locks::mutator_lock_); - std::list field_write_listeners_ GUARDED_BY(Locks::mutator_lock_); - std::list exception_caught_listeners_ GUARDED_BY(Locks::mutator_lock_); + std::shared_ptr> dex_pc_listeners_ + GUARDED_BY(Locks::mutator_lock_); + std::shared_ptr> field_read_listeners_ + GUARDED_BY(Locks::mutator_lock_); + std::shared_ptr> field_write_listeners_ + GUARDED_BY(Locks::mutator_lock_); + std::shared_ptr> exception_caught_listeners_ + GUARDED_BY(Locks::mutator_lock_); // The set of methods being deoptimized (by the debugger) which must be executed with interpreter // only.