Cleanup Instrumentation::IsActive

Instrumentation::IsActive is used to check if nterp should be disabled
because we have certain listeners that are processed only when running
in interpreter. Earlier this check included all listeners. However
- method entry / exit / unwind are handled by installing instrumentation
  stubs, so nterp need not be disabled.
- exception events are handled in nterp too (see
  art_quick_deliver_exception)
- dex_pc_listeners these are handled only in interpreter but the methods
  that need these events are deoptimized.

This CL removes these listeners from IsActive and renames the IsActive.

Change-Id: Ie2e936207405d5c9057129b64ac382fcf9f0cbfb

10 files changed, 55 insertions, 36 deletions

diff --git a/runtime/debugger.h b/runtime/debugger.h
index 65dc13d439..a79add9b8e 100644
--- a/runtime/debugger.h
+++ b/runtime/debugger.h
@@ -28,6 +28,7 @@
 #include "base/locks.h"
 #include "base/logging.h"
 #include "jni.h"
+#include "runtime.h"
 #include "runtime_callbacks.h"
 #include "thread.h"
 #include "thread_state.h"
@@ -64,7 +65,10 @@ class Dbg {
   // the deoptimized frames.
   static bool IsForcedInterpreterNeededForException(Thread* thread)
       REQUIRES_SHARED(Locks::mutator_lock_) {
-    if (LIKELY(!thread->HasDebuggerShadowFrames())) {
+    // A quick check to avoid walking the stack. If there are no shadow frames or no method
+    // that needs to be deoptimized we can safely continue with optimized code.
+    if (LIKELY(!thread->HasDebuggerShadowFrames() &&
+               Runtime::Current()->GetInstrumentation()->IsDeoptimizedMethodsEmpty())) {
       return false;
     }
     return IsForcedInterpreterNeededForExceptionImpl(thread);
diff --git a/runtime/instrumentation.cc b/runtime/instrumentation.cc
index b4abf00cf6..a9a88109e2 100644
--- a/runtime/instrumentation.cc
+++ b/runtime/instrumentation.cc
@@ -943,11 +943,7 @@ void Instrumentation::UpdateStubs() {
     InstallStubsClassVisitor visitor(this);
     runtime->GetClassLinker()->VisitClasses(&visitor);
     // Restore stack only if there is no method currently deoptimized.
-    bool empty;
-    {
-      ReaderMutexLock mu(self, *GetDeoptimizedMethodsLock());
-      empty = IsDeoptimizedMethodsEmpty();  // Avoid lock violation.
-    }
+    bool empty = IsDeoptimizedMethodsEmpty();
     if (empty) {
       MutexLock mu(self, *Locks::thread_list_lock_);
       bool no_remaining_deopts = true;
@@ -1146,7 +1142,7 @@ bool Instrumentation::RemoveDeoptimizedMethod(ArtMethod* method) {
   return true;
 }
 
-bool Instrumentation::IsDeoptimizedMethodsEmpty() const {
+bool Instrumentation::IsDeoptimizedMethodsEmptyLocked() const {
   return deoptimized_methods_.empty();
 }
 
@@ -1190,7 +1186,7 @@ void Instrumentation::Undeoptimize(ArtMethod* method) {
     bool found_and_erased = RemoveDeoptimizedMethod(method);
     CHECK(found_and_erased) << "Method " << ArtMethod::PrettyMethod(method)
         << " is not deoptimized";
-    empty = IsDeoptimizedMethodsEmpty();
+    empty = IsDeoptimizedMethodsEmptyLocked();
   }
 
   // Restore code and possibly stack only if we did not deoptimize everything.
@@ -1214,6 +1210,11 @@ void Instrumentation::Undeoptimize(ArtMethod* method) {
   }
 }
 
+bool Instrumentation::IsDeoptimizedMethodsEmpty() const {
+  ReaderMutexLock mu(Thread::Current(), *GetDeoptimizedMethodsLock());
+  return deoptimized_methods_.empty();
+}
+
 bool Instrumentation::IsDeoptimized(ArtMethod* method) {
   DCHECK(method != nullptr);
   ReaderMutexLock mu(Thread::Current(), *GetDeoptimizedMethodsLock());
@@ -1229,7 +1230,7 @@ void Instrumentation::DisableDeoptimization(const char* key) {
     ArtMethod* method;
     {
       ReaderMutexLock mu(Thread::Current(), *GetDeoptimizedMethodsLock());
-      if (IsDeoptimizedMethodsEmpty()) {
+      if (IsDeoptimizedMethodsEmptyLocked()) {
         break;
       }
       method = BeginDeoptimizedMethod();
diff --git a/runtime/instrumentation.h b/runtime/instrumentation.h
index e53ee82a45..572586eefc 100644
--- a/runtime/instrumentation.h
+++ b/runtime/instrumentation.h
@@ -260,6 +260,11 @@ class Instrumentation {
   bool IsDeoptimized(ArtMethod* method)
       REQUIRES(!GetDeoptimizedMethodsLock()) REQUIRES_SHARED(Locks::mutator_lock_);
 
+  // Indicates if any method needs to be deoptimized. This is used to avoid walking the stack to
+  // determine if a deoptimization is required.
+  bool IsDeoptimizedMethodsEmpty() const
+      REQUIRES(!GetDeoptimizedMethodsLock()) REQUIRES_SHARED(Locks::mutator_lock_);
+
   // Enable method tracing by installing instrumentation entry/exit stubs or interpreter.
   void EnableMethodTracing(const char* key,
                            bool needs_interpreter = kDeoptimizeForAccurateMethodEntryExitListeners)
@@ -371,12 +376,11 @@ class Instrumentation {
     return have_exception_handled_listeners_;
   }
 
-  bool IsActive() const REQUIRES_SHARED(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_thrown_listeners_ || have_method_unwind_listeners_ ||
-        have_branch_listeners_ || have_watched_frame_pop_listeners_ ||
-        have_exception_handled_listeners_;
+  bool NeedsSlowInterpreterForListeners() const REQUIRES_SHARED(Locks::mutator_lock_) {
+    return have_field_read_listeners_ ||
+           have_field_write_listeners_ ||
+           have_watched_frame_pop_listeners_ ||
+           have_exception_handled_listeners_;
   }
 
   // Inform listeners that a method has been entered. A dex PC is provided as we may install
@@ -619,7 +623,7 @@ class Instrumentation {
       REQUIRES_SHARED(Locks::mutator_lock_) REQUIRES(GetDeoptimizedMethodsLock());
   ArtMethod* BeginDeoptimizedMethod()
       REQUIRES_SHARED(Locks::mutator_lock_, GetDeoptimizedMethodsLock());
-  bool IsDeoptimizedMethodsEmpty() const
+  bool IsDeoptimizedMethodsEmptyLocked() const
       REQUIRES_SHARED(Locks::mutator_lock_, GetDeoptimizedMethodsLock());
   void UpdateMethodsCodeImpl(ArtMethod* method, const void* new_code)
       REQUIRES_SHARED(Locks::mutator_lock_) REQUIRES(!GetDeoptimizedMethodsLock());
diff --git a/runtime/instrumentation_test.cc b/runtime/instrumentation_test.cc
index 9373751863..b0a81b6472 100644
--- a/runtime/instrumentation_test.cc
+++ b/runtime/instrumentation_test.cc
@@ -466,7 +466,7 @@ TEST_F(InstrumentationTest, NoInstrumentation) {
 
   EXPECT_FALSE(instr->AreExitStubsInstalled());
   EXPECT_FALSE(instr->AreAllMethodsDeoptimized());
-  EXPECT_FALSE(instr->IsActive());
+  EXPECT_FALSE(instr->NeedsSlowInterpreterForListeners());
   EXPECT_FALSE(instr->ShouldNotifyMethodEnterExitEvents());
 
 
@@ -478,7 +478,6 @@ TEST_F(InstrumentationTest, NoInstrumentation) {
   EXPECT_FALSE(instr->HasFieldWriteListeners());
   EXPECT_FALSE(instr->HasMethodEntryListeners());
   EXPECT_FALSE(instr->HasMethodExitListeners());
-  EXPECT_FALSE(instr->IsActive());
 }
 
 // Test instrumentation listeners for each event.
diff --git a/runtime/interpreter/interpreter.cc b/runtime/interpreter/interpreter.cc
index 4339120fa1..dc7e23ec06 100644
--- a/runtime/interpreter/interpreter.cc
+++ b/runtime/interpreter/interpreter.cc
@@ -476,13 +476,16 @@ void EnterInterpreterFromDeoptimize(Thread* self,
     const uint32_t dex_pc = shadow_frame->GetDexPC();
     uint32_t new_dex_pc = dex_pc;
     if (UNLIKELY(self->IsExceptionPending())) {
-      // If we deoptimize from the QuickExceptionHandler, we already reported the exception to
-      // the instrumentation. To prevent from reporting it a second time, we simply pass a
-      // null Instrumentation*.
-      const instrumentation::Instrumentation* const instrumentation =
-          frame_cnt == 0 ? nullptr : Runtime::Current()->GetInstrumentation();
-      new_dex_pc = MoveToExceptionHandler(
-          self, *shadow_frame, instrumentation) ? shadow_frame->GetDexPC() : dex::kDexNoIndex;
+      // If we deoptimize from the QuickExceptionHandler, we already reported the exception throw
+      // event to the instrumentation. Skip throw listeners for the first frame. The deopt check
+      // should happen after the throw listener is called as throw listener can trigger a
+      // deoptimization.
+      new_dex_pc = MoveToExceptionHandler(self,
+                                          *shadow_frame,
+                                          /* skip_listeners= */ false,
+                                          /* skip_throw_listener= */ frame_cnt == 0) ?
+                       shadow_frame->GetDexPC() :
+                       dex::kDexNoIndex;
     } else if (!from_code) {
       // Deoptimization is not called from code directly.
       const Instruction* instr = &accessor.InstructionAt(dex_pc);
diff --git a/runtime/interpreter/interpreter_common.cc b/runtime/interpreter/interpreter_common.cc
index cafe74714e..c8a87c1d75 100644
--- a/runtime/interpreter/interpreter_common.cc
+++ b/runtime/interpreter/interpreter_common.cc
@@ -150,11 +150,14 @@ bool SendMethodExitEvents(Thread* self,
 // behavior.
 bool MoveToExceptionHandler(Thread* self,
                             ShadowFrame& shadow_frame,
-                            const instrumentation::Instrumentation* instrumentation) {
+                            bool skip_listeners,
+                            bool skip_throw_listener) {
   self->VerifyStack();
   StackHandleScope<2> hs(self);
   Handle<mirror::Throwable> exception(hs.NewHandle(self->GetException()));
-  if (instrumentation != nullptr &&
+  const instrumentation::Instrumentation* instrumentation =
+      Runtime::Current()->GetInstrumentation();
+  if (!skip_throw_listener &&
       instrumentation->HasExceptionThrownListeners() &&
       self->IsExceptionThrownByCurrentMethod(exception.Get())) {
     // See b/65049545 for why we don't need to check to see if the exception has changed.
@@ -169,7 +172,7 @@ bool MoveToExceptionHandler(Thread* self,
   uint32_t found_dex_pc = shadow_frame.GetMethod()->FindCatchBlock(
       hs.NewHandle(exception->GetClass()), shadow_frame.GetDexPC(), &clear_exception);
   if (found_dex_pc == dex::kDexNoIndex) {
-    if (instrumentation != nullptr) {
+    if (!skip_listeners) {
       if (shadow_frame.NeedsNotifyPop()) {
         instrumentation->WatchedFramePopped(self, shadow_frame);
         if (shadow_frame.GetForcePopFrame()) {
@@ -189,12 +192,12 @@ bool MoveToExceptionHandler(Thread* self,
     return shadow_frame.GetForcePopFrame();
   } else {
     shadow_frame.SetDexPC(found_dex_pc);
-    if (instrumentation != nullptr && instrumentation->HasExceptionHandledListeners()) {
+    if (!skip_listeners && instrumentation->HasExceptionHandledListeners()) {
       self->ClearException();
       instrumentation->ExceptionHandledEvent(self, exception.Get());
       if (UNLIKELY(self->IsExceptionPending())) {
         // Exception handled event threw an exception. Try to find the handler for this one.
-        return MoveToExceptionHandler(self, shadow_frame, instrumentation);
+        return MoveToExceptionHandler(self, shadow_frame, skip_listeners, skip_throw_listener);
       } else if (!clear_exception) {
         self->SetException(exception.Get());
       }
diff --git a/runtime/interpreter/interpreter_common.h b/runtime/interpreter/interpreter_common.h
index e9b78f3259..95e3bd521e 100644
--- a/runtime/interpreter/interpreter_common.h
+++ b/runtime/interpreter/interpreter_common.h
@@ -706,8 +706,8 @@ static inline int32_t DoSparseSwitch(const Instruction* inst, const ShadowFrame&
 // TODO We might wish to reconsider how we cause some events to be ignored.
 bool MoveToExceptionHandler(Thread* self,
                             ShadowFrame& shadow_frame,
-                            const instrumentation::Instrumentation* instrumentation)
-    REQUIRES_SHARED(Locks::mutator_lock_);
+                            bool skip_listeners,
+                            bool skip_throw_listener) REQUIRES_SHARED(Locks::mutator_lock_);
 
 NO_RETURN void UnexpectedOpcode(const Instruction* inst, const ShadowFrame& shadow_frame)
   __attribute__((cold))
diff --git a/runtime/interpreter/interpreter_switch_impl-inl.h b/runtime/interpreter/interpreter_switch_impl-inl.h
index f58fb954dd..d95c507698 100644
--- a/runtime/interpreter/interpreter_switch_impl-inl.h
+++ b/runtime/interpreter/interpreter_switch_impl-inl.h
@@ -95,8 +95,11 @@ class InstructionHandler {
     }
     bool skip_event = shadow_frame_.GetSkipNextExceptionEvent();
     shadow_frame_.SetSkipNextExceptionEvent(false);
-    if (!MoveToExceptionHandler(Self(), shadow_frame_, skip_event ? nullptr : Instrumentation())) {
-      /* Structured locking is to be enforced for abnormal termination, too. */
+    if (!MoveToExceptionHandler(Self(),
+                                shadow_frame_,
+                                /* skip_listeners= */ skip_event,
+                                /* skip_throw_listener= */ skip_event)) {
+      // Structured locking is to be enforced for abnormal termination, too.
       DoMonitorCheckOnExit<do_assignability_check>(Self(), &shadow_frame_);
       ctx_->result = JValue(); /* Handled in caller. */
       ExitInterpreterLoop();
diff --git a/runtime/interpreter/mterp/nterp.cc b/runtime/interpreter/mterp/nterp.cc
index 34c971c109..89e6a134e1 100644
--- a/runtime/interpreter/mterp/nterp.cc
+++ b/runtime/interpreter/mterp/nterp.cc
@@ -46,7 +46,7 @@ bool CanRuntimeUseNterp() REQUIRES_SHARED(Locks::mutator_lock_) {
   return IsNterpSupported() &&
       !instr->InterpretOnly() &&
       !runtime->IsAotCompiler() &&
-      !runtime->GetInstrumentation()->IsActive() &&
+      !runtime->GetInstrumentation()->NeedsSlowInterpreterForListeners() &&
       // An async exception has been thrown. We need to go to the switch interpreter. nterp doesn't
       // know how to deal with these so we could end up never dealing with it if we are in an
       // infinite loop.
diff --git a/runtime/thread.cc b/runtime/thread.cc
index 4b5ec18657..8b2a4117e9 100644
--- a/runtime/thread.cc
+++ b/runtime/thread.cc
@@ -3672,7 +3672,9 @@ void Thread::QuickDeliverException() {
 
   ReadBarrier::MaybeAssertToSpaceInvariant(exception.Ptr());
 
-  // This is a real exception: let the instrumentation know about it.
+  // This is a real exception: let the instrumentation know about it. Exception throw listener
+  // could set a breakpoint or install listeners that might require a deoptimization. Hence the
+  // deoptimization check needs to happen after calling the listener.
   instrumentation::Instrumentation* instrumentation = Runtime::Current()->GetInstrumentation();
   if (instrumentation->HasExceptionThrownListeners() &&
       IsExceptionThrownByCurrentMethod(exception)) {