Ensure that OSR still is possible with jvmti
We were previously disabling OSR by always claiming to be interested
in every method. This could cause slowdown on some methods. To fix
this we correctly only claim to be interested in methods if we might
hit breakpoints from an invoke, have modified locals, or have forced
the function to the interpreter.
There was also a minor bug in the instrumentation removal code that
caused prebuilt compiled code to be used when it should have been
ignored.
In the future we should get the granularity down to single frames but
that is currently not possible.
Test: ./test.py --host -j50 --all -t 993
Test: ./test.py --host -j50
Test: am start --attach-agent -n com.example.android.displayingbitmaps/.ui.ImageGridActivity
Run blur filter.
Bug: 76226464
Change-Id: I6a7aa0c6353372aebe01613b66841543614f3054
diff --git a/openjdkjvmti/deopt_manager.cc b/openjdkjvmti/deopt_manager.cc
index 6d84ffa..380d95c 100644
--- a/openjdkjvmti/deopt_manager.cc
+++ b/openjdkjvmti/deopt_manager.cc
@@ -53,14 +53,18 @@
namespace openjdkjvmti {
// TODO We should make this much more selective in the future so we only return true when we
-// actually care about the method (i.e. had locals changed, have breakpoints, etc.). For now though
-// we can just assume that we care we are loaded at all.
-//
-// Even if we don't keep track of this at the method level we might want to keep track of it at the
-// level of enabled capabilities.
-bool JvmtiMethodInspectionCallback::IsMethodBeingInspected(
- art::ArtMethod* method ATTRIBUTE_UNUSED) {
- return true;
+// 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) {
+ // Non-java-debuggable runtimes we need to assume that any method might not be debuggable and
+ // therefore potentially being inspected (due to inlines). If we are debuggable we rely hard on
+ // inlining not being done since we don't keep track of which methods get inlined where and simply
+ // look to see if the method is breakpointed.
+ return !art::Runtime::Current()->IsJavaDebuggable() ||
+ manager_->HaveLocalsChanged() ||
+ manager_->MethodHasBreakpoints(method);
}
bool JvmtiMethodInspectionCallback::IsMethodSafeToJit(art::ArtMethod* method) {
@@ -75,7 +79,10 @@
performing_deoptimization_(false),
global_deopt_count_(0),
deopter_count_(0),
- inspection_callback_(this) { }
+ breakpoint_status_lock_("JVMTI_BreakpointStatusLock",
+ static_cast<art::LockLevel>(art::LockLevel::kAbortLock + 1)),
+ inspection_callback_(this),
+ set_local_variable_called_(false) { }
void DeoptManager::Setup() {
art::ScopedThreadStateChange stsc(art::Thread::Current(),
@@ -121,14 +128,11 @@
}
bool DeoptManager::MethodHasBreakpoints(art::ArtMethod* method) {
- art::MutexLock lk(art::Thread::Current(), deoptimization_status_lock_);
+ art::MutexLock lk(art::Thread::Current(), breakpoint_status_lock_);
return MethodHasBreakpointsLocked(method);
}
bool DeoptManager::MethodHasBreakpointsLocked(art::ArtMethod* method) {
- if (deopter_count_ == 0) {
- return false;
- }
auto elem = breakpoint_status_.find(method);
return elem != breakpoint_status_.end() && elem->second != 0;
}
@@ -158,18 +162,23 @@
art::ScopedThreadSuspension sts(self, art::kSuspended);
deoptimization_status_lock_.ExclusiveLock(self);
+ {
+ breakpoint_status_lock_.ExclusiveLock(self);
- DCHECK_GT(deopter_count_, 0u) << "unexpected deotpimization request";
+ DCHECK_GT(deopter_count_, 0u) << "unexpected deotpimization request";
- if (MethodHasBreakpointsLocked(method)) {
- // Don't need to do anything extra.
- breakpoint_status_[method]++;
- // Another thread might be deoptimizing the very method we just added new breakpoints for. Wait
- // for any deopts to finish before moving on.
- WaitForDeoptimizationToFinish(self);
- return;
+ if (MethodHasBreakpointsLocked(method)) {
+ // Don't need to do anything extra.
+ breakpoint_status_[method]++;
+ // Another thread might be deoptimizing the very method we just added new breakpoints for.
+ // Wait for any deopts to finish before moving on.
+ breakpoint_status_lock_.ExclusiveUnlock(self);
+ WaitForDeoptimizationToFinish(self);
+ return;
+ }
+ breakpoint_status_[method] = 1;
+ breakpoint_status_lock_.ExclusiveUnlock(self);
}
- breakpoint_status_[method] = 1;
auto instrumentation = art::Runtime::Current()->GetInstrumentation();
if (instrumentation->IsForcedInterpretOnly()) {
// We are already interpreting everything so no need to do anything.
@@ -196,17 +205,22 @@
// need but since that is very heavy we will instead just use a condition variable to make sure we
// don't race with ourselves.
deoptimization_status_lock_.ExclusiveLock(self);
+ bool is_last_breakpoint;
+ {
+ art::MutexLock mu(self, breakpoint_status_lock_);
- DCHECK_GT(deopter_count_, 0u) << "unexpected deotpimization request";
- DCHECK(MethodHasBreakpointsLocked(method)) << "Breakpoint on a method was removed without "
- << "breakpoints present!";
+ DCHECK_GT(deopter_count_, 0u) << "unexpected deotpimization request";
+ DCHECK(MethodHasBreakpointsLocked(method)) << "Breakpoint on a method was removed without "
+ << "breakpoints present!";
+ breakpoint_status_[method] -= 1;
+ is_last_breakpoint = (breakpoint_status_[method] == 0);
+ }
auto instrumentation = art::Runtime::Current()->GetInstrumentation();
- breakpoint_status_[method] -= 1;
if (UNLIKELY(instrumentation->IsForcedInterpretOnly())) {
// We don't need to do anything since we are interpreting everything anyway.
deoptimization_status_lock_.ExclusiveUnlock(self);
return;
- } else if (breakpoint_status_[method] == 0) {
+ } else if (is_last_breakpoint) {
if (UNLIKELY(is_default)) {
RemoveDeoptimizeAllMethodsLocked(self);
} else {
diff --git a/openjdkjvmti/deopt_manager.h b/openjdkjvmti/deopt_manager.h
index a495b68..a38690c 100644
--- a/openjdkjvmti/deopt_manager.h
+++ b/openjdkjvmti/deopt_manager.h
@@ -32,6 +32,7 @@
#ifndef ART_OPENJDKJVMTI_DEOPT_MANAGER_H_
#define ART_OPENJDKJVMTI_DEOPT_MANAGER_H_
+#include <atomic>
#include <unordered_map>
#include "jni.h"
@@ -107,9 +108,17 @@
static DeoptManager* Get();
+ bool HaveLocalsChanged() const {
+ return set_local_variable_called_.load();
+ }
+
+ void SetLocalsUpdated() {
+ set_local_variable_called_.store(true);
+ }
+
private:
bool MethodHasBreakpointsLocked(art::ArtMethod* method)
- REQUIRES(deoptimization_status_lock_);
+ REQUIRES(breakpoint_status_lock_);
// Wait until nothing is currently in the middle of deoptimizing/undeoptimizing something. This is
// needed to ensure that everything is synchronized since threads need to drop the
@@ -156,13 +165,20 @@
// Number of users of deoptimization there currently are.
uint32_t deopter_count_ GUARDED_BY(deoptimization_status_lock_);
+ // A mutex that just protects the breakpoint-status map. This mutex should always be at the
+ // bottom of the lock hierarchy. Nothing more should be locked if we hold this.
+ art::Mutex breakpoint_status_lock_ ACQUIRED_BEFORE(art::Locks::abort_lock_);
// A map from methods to the number of breakpoints in them from all envs.
std::unordered_map<art::ArtMethod*, uint32_t> breakpoint_status_
- GUARDED_BY(deoptimization_status_lock_);
+ GUARDED_BY(breakpoint_status_lock_);
// The MethodInspectionCallback we use to tell the runtime if we care about particular methods.
JvmtiMethodInspectionCallback inspection_callback_;
+ // Set to true if anything calls SetLocalVariables on any thread since we need to be careful about
+ // OSR after this.
+ std::atomic<bool> set_local_variable_called_;
+
// Helper for setting up/tearing-down for deoptimization.
friend class ScopedDeoptimizationContext;
};
diff --git a/openjdkjvmti/ti_method.cc b/openjdkjvmti/ti_method.cc
index bf2e6cd..b83310d 100644
--- a/openjdkjvmti/ti_method.cc
+++ b/openjdkjvmti/ti_method.cc
@@ -915,6 +915,9 @@
if (depth < 0) {
return ERR(ILLEGAL_ARGUMENT);
}
+ // Make sure that we know not to do any OSR anymore.
+ // TODO We should really keep track of this at the Frame granularity.
+ DeoptManager::Get()->SetLocalsUpdated();
art::Thread* self = art::Thread::Current();
// Suspend JIT since it can get confused if we deoptimize methods getting jitted.
art::jit::ScopedJitSuspend suspend_jit;
diff --git a/runtime/instrumentation.cc b/runtime/instrumentation.cc
index 84a148f..ec3e10e 100644
--- a/runtime/instrumentation.cc
+++ b/runtime/instrumentation.cc
@@ -139,10 +139,14 @@
bool Instrumentation::NeedDebugVersionFor(ArtMethod* method) const
REQUIRES_SHARED(Locks::mutator_lock_) {
- return Runtime::Current()->IsJavaDebuggable() &&
+ art::Runtime* runtime = Runtime::Current();
+ return runtime->IsJavaDebuggable() &&
!method->IsNative() &&
!method->IsProxyMethod() &&
- Runtime::Current()->GetRuntimeCallbacks()->IsMethodBeingInspected(method);
+ // If we don't have a jit this can push us to the pre-compiled version of methods which is
+ // not something we want since we are debuggable.
+ (UNLIKELY(runtime->GetJit() == nullptr) ||
+ runtime->GetRuntimeCallbacks()->IsMethodBeingInspected(method));
}
void Instrumentation::InstallStubsForMethod(ArtMethod* method) {
diff --git a/test/1935-get-set-current-frame-jit/expected.txt b/test/1935-get-set-current-frame-jit/expected.txt
index cdb8f6a..a685891 100644
--- a/test/1935-get-set-current-frame-jit/expected.txt
+++ b/test/1935-get-set-current-frame-jit/expected.txt
@@ -1,7 +1,5 @@
JNI_OnLoad called
From GetLocalInt(), value is 42
-isInOsrCode? false
Value is '42'
Setting TARGET to 1337
-isInOsrCode? false
Value is '1337'
diff --git a/test/1935-get-set-current-frame-jit/src/Main.java b/test/1935-get-set-current-frame-jit/src/Main.java
index 714a98a..aad4cd7 100644
--- a/test/1935-get-set-current-frame-jit/src/Main.java
+++ b/test/1935-get-set-current-frame-jit/src/Main.java
@@ -49,9 +49,11 @@
public static class IntRunner implements Runnable {
private volatile boolean continueBusyLoop;
private volatile boolean inBusyLoop;
- public IntRunner() {
+ private final boolean expectOsr;
+ public IntRunner(boolean expectOsr) {
this.continueBusyLoop = true;
this.inBusyLoop = false;
+ this.expectOsr = expectOsr;
}
public void run() {
int TARGET = 42;
@@ -64,9 +66,16 @@
Main.ensureJitCompiled(IntRunner.class, "run");
i++;
}
- // We shouldn't be doing OSR since we are using JVMTI and the get/set prevents OSR.
+ // We shouldn't be doing OSR since we are using JVMTI and the set prevents OSR.
// Set local will also push us to interpreter but the get local may remain in compiled code.
- System.out.println("isInOsrCode? " + (hasJit() && Main.isInOsrCode("run")));
+ if (hasJit()) {
+ boolean inOsr = Main.isInOsrCode("run");
+ if (expectOsr && !inOsr) {
+ throw new Error("Expected to be in OSR but was not.");
+ } else if (!expectOsr && inOsr) {
+ throw new Error("Expected not to be in OSR but was.");
+ }
+ }
reportValue(TARGET);
}
public void waitForBusyLoopStart() { while (!inBusyLoop) {} }
@@ -78,7 +87,7 @@
public static void runGet() throws Exception {
Method target = IntRunner.class.getDeclaredMethod("run");
// Get Int
- IntRunner int_runner = new IntRunner();
+ IntRunner int_runner = new IntRunner(true);
Thread target_get = new Thread(int_runner, "GetLocalInt - Target");
target_get.start();
int_runner.waitForBusyLoopStart();
@@ -108,7 +117,7 @@
public static void runSet() throws Exception {
Method target = IntRunner.class.getDeclaredMethod("run");
// Set Int
- IntRunner int_runner = new IntRunner();
+ IntRunner int_runner = new IntRunner(false);
Thread target_set = new Thread(int_runner, "SetLocalInt - Target");
target_set.start();
int_runner.waitForBusyLoopStart();
