Cleanup around method inspection callbacks

IsMethodBeingInspected callback was used to prevent moving to compiled
code (JIT / OSR / Nterp) while a method has breakpoints or other
debugger related events enabled. We also use Instrumentation::IsDeoptimized
for similar purpose in some places (for ex: when determining if a method
needs to be deoptimized). Deoptimized methods list is a super set of
methods that have breakpoints on them. So we can replace most method
inspection callbacks to use IsDeoptimized check instead.

The one exception is when we decide if we need to OSR a method. Here we
need to check if the frame has any locals changed which isn't covered by
IsDeoptimized check and requires a runtime callback to see if any locals
have changed.

This CL:
1. Adds a new runtime callback called HaveLocalsChanged to check if any
   locals have changed and uses it to prevent OSRing such frames.
2. Removes IsMethodBeingInspected and replaces it with IsDeoptimized /
   HaveLocalsChanged as required.

Bug: 206029744
Test: art/test.py

Change-Id: Ie649cffaeba3d31746527e0ee326abe81284978d

7 files changed, 21 insertions, 26 deletions

diff --git a/openjdkjvmti/deopt_manager.cc b/openjdkjvmti/deopt_manager.cc
index 312a797b9d..03042eccfd 100644
--- a/openjdkjvmti/deopt_manager.cc
+++ b/openjdkjvmti/deopt_manager.cc
@@ -61,16 +61,13 @@
 
 namespace openjdkjvmti {
 
-// TODO We should make this much more selective in the future so we only return true when we
+// We could make this much more selective in the future so we only return true when we
 // actually care about the method at this time (ie active frames had locals changed). For now we
-// just assume that if anything has changed any frame's locals we care about all methods. If nothing
-// has we only care about methods with active breakpoints on them. In the future we should probably
-// rewrite all of this to instead do this at the ShadowFrame or thread granularity.
-bool JvmtiMethodInspectionCallback::IsMethodBeingInspected(art::ArtMethod* method) {
-  // In non-java-debuggable runtimes the breakpoint check would miss if we have breakpoints on
-  // methods that are inlined. Since these features are best effort in non-java-debuggable
-  // runtimes it is OK to be less precise. For debuggable runtimes, inlining is disabled.
-  return manager_->HaveLocalsChanged() || manager_->MethodHasBreakpoints(method);
+// just assume that if anything has changed any frame's locals we care about all methods. This only
+// impacts whether we are able to OSR or not so maybe not really important to maintain frame
+// specific information.
+bool JvmtiMethodInspectionCallback::HaveLocalsChanged() {
+  return manager_->HaveLocalsChanged();
 }
 
 DeoptManager::DeoptManager()
diff --git a/openjdkjvmti/deopt_manager.h b/openjdkjvmti/deopt_manager.h
index c0a788b1b5..9278bf1bc2 100644
--- a/openjdkjvmti/deopt_manager.h
+++ b/openjdkjvmti/deopt_manager.h
@@ -57,8 +57,7 @@ struct JvmtiMethodInspectionCallback : public art::MethodInspectionCallback {
  public:
   explicit JvmtiMethodInspectionCallback(DeoptManager* manager) : manager_(manager) {}
 
-  bool IsMethodBeingInspected(art::ArtMethod* method)
-      override REQUIRES_SHARED(art::Locks::mutator_lock_);
+  bool HaveLocalsChanged() override REQUIRES_SHARED(art::Locks::mutator_lock_);
 
  private:
   DeoptManager* manager_;
diff --git a/runtime/instrumentation.cc b/runtime/instrumentation.cc
index 91d0fae232..3b17bb5ee4 100644
--- a/runtime/instrumentation.cc
+++ b/runtime/instrumentation.cc
@@ -285,9 +285,7 @@ bool Instrumentation::InterpretOnly(ArtMethod* method) REQUIRES_SHARED(Locks::mu
   if (method->IsNative()) {
     return false;
   }
-  return InterpretOnly() ||
-         IsDeoptimized(method) ||
-         Runtime::Current()->GetRuntimeCallbacks()->IsMethodBeingInspected(method);
+  return InterpretOnly() || IsDeoptimized(method);
 }
 
 static bool CanUseAotCode(const void* quick_code)
diff --git a/runtime/jit/jit.cc b/runtime/jit/jit.cc
index 5d0430e7fb..91869c6175 100644
--- a/runtime/jit/jit.cc
+++ b/runtime/jit/jit.cc
@@ -282,9 +282,8 @@ bool Jit::CompileMethodInternal(ArtMethod* method,
     compilation_kind = CompilationKind::kOptimized;
   }
 
-  RuntimeCallbacks* cb = Runtime::Current()->GetRuntimeCallbacks();
   // Don't compile the method if it has breakpoints.
-  if (cb->IsMethodBeingInspected(method)) {
+  if (Runtime::Current()->GetInstrumentation()->IsDeoptimized(method)) {
     VLOG(jit) << "JIT not compiling " << method->PrettyMethod()
               << " due to not being safe to jit according to runtime-callbacks. For example, there"
               << " could be breakpoints in this method.";
@@ -571,8 +570,11 @@ bool Jit::MaybeDoOnStackReplacement(Thread* thread,
   // Before allowing the jump, make sure no code is actively inspecting the method to avoid
   // jumping from interpreter to OSR while e.g. single stepping. Note that we could selectively
   // disable OSR when single stepping, but that's currently hard to know at this point.
+  // Currently, HaveLocalsChanged is not frame specific. It is possible to make it frame specific
+  // to allow OSR of frames that don't have any locals changed but it isn't worth the additional
+  // complexity.
   if (Runtime::Current()->GetInstrumentation()->NeedsSlowInterpreterForMethod(thread, method) ||
-      Runtime::Current()->GetRuntimeCallbacks()->IsMethodBeingInspected(method)) {
+      Runtime::Current()->GetRuntimeCallbacks()->HaveLocalsChanged()) {
     return false;
   }
 
diff --git a/runtime/runtime_callbacks.cc b/runtime/runtime_callbacks.cc
index 753ac280e3..28c81a22c7 100644
--- a/runtime/runtime_callbacks.cc
+++ b/runtime/runtime_callbacks.cc
@@ -105,9 +105,9 @@ void RuntimeCallbacks::RemoveMethodInspectionCallback(MethodInspectionCallback*
   Remove(cb, &method_inspection_callbacks_);
 }
 
-bool RuntimeCallbacks::IsMethodBeingInspected(ArtMethod* m) {
+bool RuntimeCallbacks::HaveLocalsChanged() {
   for (MethodInspectionCallback* cb : COPY(method_inspection_callbacks_)) {
-    if (cb->IsMethodBeingInspected(m)) {
+    if (cb->HaveLocalsChanged()) {
       return true;
     }
   }
diff --git a/runtime/runtime_callbacks.h b/runtime/runtime_callbacks.h
index b1a7e55ae4..98584a8587 100644
--- a/runtime/runtime_callbacks.h
+++ b/runtime/runtime_callbacks.h
@@ -143,9 +143,8 @@ class MethodInspectionCallback {
  public:
   virtual ~MethodInspectionCallback() {}
 
-  // Returns true if the method is being inspected currently and the runtime should not modify it in
-  // potentially dangerous ways (i.e. replace with compiled version, JIT it, etc).
-  virtual bool IsMethodBeingInspected(ArtMethod* method) REQUIRES_SHARED(Locks::mutator_lock_) = 0;
+  // Returns true if any locals have changed. If any locals have changed we shouldn't OSR.
+  virtual bool HaveLocalsChanged() REQUIRES_SHARED(Locks::mutator_lock_) = 0;
 };
 
 // Callback to let something request to be notified when reflective objects are being visited and
@@ -225,9 +224,9 @@ class RuntimeCallbacks {
   void AddParkCallback(ParkCallback* cb) REQUIRES_SHARED(Locks::mutator_lock_);
   void RemoveParkCallback(ParkCallback* cb) REQUIRES_SHARED(Locks::mutator_lock_);
 
-  // Returns true if some MethodInspectionCallback indicates the method is being inspected/depended
-  // on by some code.
-  bool IsMethodBeingInspected(ArtMethod* method) REQUIRES_SHARED(Locks::mutator_lock_);
+  // Returns true if any locals have changed. This is used to prevent OSRing frames that have
+  // some locals changed.
+  bool HaveLocalsChanged() REQUIRES_SHARED(Locks::mutator_lock_);
 
   void AddMethodInspectionCallback(MethodInspectionCallback* cb)
       REQUIRES_SHARED(Locks::mutator_lock_);
diff --git a/test/common/runtime_state.cc b/test/common/runtime_state.cc
index f39bca1bcf..a900714f09 100644
--- a/test/common/runtime_state.cc
+++ b/test/common/runtime_state.cc
@@ -234,7 +234,7 @@ static void ForceJitCompiled(Thread* self,
 
   {
     ScopedObjectAccess soa(self);
-    if (Runtime::Current()->GetRuntimeCallbacks()->IsMethodBeingInspected(method)) {
+    if (Runtime::Current()->GetInstrumentation()->IsDeoptimized(method)) {
       std::string msg(method->PrettyMethod());
       msg += ": is not safe to jit!";
       ThrowIllegalStateException(msg.c_str());