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
diff --git a/openjdkjvmti/deopt_manager.cc b/openjdkjvmti/deopt_manager.cc
index 312a797..03042ec 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 c0a788b..9278bf1 100644
--- a/openjdkjvmti/deopt_manager.h
+++ b/openjdkjvmti/deopt_manager.h
@@ -57,8 +57,7 @@
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 91d0fae..3b17bb5 100644
--- a/runtime/instrumentation.cc
+++ b/runtime/instrumentation.cc
@@ -285,9 +285,7 @@
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 5d0430e..91869c6 100644
--- a/runtime/jit/jit.cc
+++ b/runtime/jit/jit.cc
@@ -282,9 +282,8 @@
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 @@
// 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 753ac28..28c81a2 100644
--- a/runtime/runtime_callbacks.cc
+++ b/runtime/runtime_callbacks.cc
@@ -105,9 +105,9 @@
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 b1a7e55..98584a8 100644
--- a/runtime/runtime_callbacks.h
+++ b/runtime/runtime_callbacks.h
@@ -143,9 +143,8 @@
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 @@
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 f39bca1..a900714 100644
--- a/test/common/runtime_state.cc
+++ b/test/common/runtime_state.cc
@@ -234,7 +234,7 @@
{
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());