Revert^6 "Ensure that OSR still is possible with jvmti"
The instrumentation uninstall could set methods to non-debuggable
boot.oat code. This could cause events to be missed due to methods
being inlined. We needed to change the path so that we would only have
the JIT/interpreter replace methods. We do this by adding a new
callback that can be used to determine if a method needs to be
debuggable and being more careful about replacing code when this is
true.
This reverts commit 5f3005c8844d851d7d218b88b5f90d6c9083ce24.
This unreverts commit b9ad26d1ed9146b89555d4333021f44eeb831f05.
Reason for revert: Fixed issue causing CTS version of test 993 failure.
Test: cts-tradefed run cts-dev CtsJvmtiRunTest993HostTestCases
Test: ./test.py --host -j50 --all -t 993
Test: ./test.py --host
Test: while ./test/run-test --host --jit 1935; do; done
Test: while ./test/run-test --host --jit --jvmti-redefine-stress 1935; do; done
Test: am start --attach-agent -n com.example.android.displayingbitmaps/.ui.ImageGridActivity
Run blur filter.
Bug: 76226464
Bug: 77306669
Change-Id: I5068201a03f7613787c66981405499b6499c24e1
diff --git a/openjdkjvmti/deopt_manager.cc b/openjdkjvmti/deopt_manager.cc
index 6d84ffa..a6f1207 100644
--- a/openjdkjvmti/deopt_manager.cc
+++ b/openjdkjvmti/deopt_manager.cc
@@ -53,20 +53,29 @@
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) {
return !manager_->MethodHasBreakpoints(method);
}
+bool JvmtiMethodInspectionCallback::MethodNeedsDebugVersion(
+ art::ArtMethod* method ATTRIBUTE_UNUSED) {
+ return true;
+}
+
DeoptManager::DeoptManager()
: deoptimization_status_lock_("JVMTI_DeoptimizationStatusLock",
static_cast<art::LockLevel>(
@@ -75,7 +84,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 +133,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 +167,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 +210,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 {