Revert^3 "Ensure that OSR still is possible with jvmti"
This reverts commit 94e3dd79da6c94a6b024da776b34a87d59a6d53d.
Reason for revert: test 1935 is sporadically failing on bots.
Bug: 76226464
Change-Id: I42f6eaa51887701a2c88187abbc100e2ec3ef922
Test: None
diff --git a/openjdkjvmti/deopt_manager.cc b/openjdkjvmti/deopt_manager.cc
index 380d95c..6d84ffa 100644
--- a/openjdkjvmti/deopt_manager.cc
+++ b/openjdkjvmti/deopt_manager.cc
@@ -53,18 +53,14 @@
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 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);
+// 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;
}
bool JvmtiMethodInspectionCallback::IsMethodSafeToJit(art::ArtMethod* method) {
@@ -79,10 +75,7 @@
performing_deoptimization_(false),
global_deopt_count_(0),
deopter_count_(0),
- breakpoint_status_lock_("JVMTI_BreakpointStatusLock",
- static_cast<art::LockLevel>(art::LockLevel::kAbortLock + 1)),
- inspection_callback_(this),
- set_local_variable_called_(false) { }
+ inspection_callback_(this) { }
void DeoptManager::Setup() {
art::ScopedThreadStateChange stsc(art::Thread::Current(),
@@ -128,11 +121,14 @@
}
bool DeoptManager::MethodHasBreakpoints(art::ArtMethod* method) {
- art::MutexLock lk(art::Thread::Current(), breakpoint_status_lock_);
+ art::MutexLock lk(art::Thread::Current(), deoptimization_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;
}
@@ -162,23 +158,18 @@
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.
- breakpoint_status_lock_.ExclusiveUnlock(self);
- WaitForDeoptimizationToFinish(self);
- return;
- }
- breakpoint_status_[method] = 1;
- breakpoint_status_lock_.ExclusiveUnlock(self);
+ 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;
}
+ breakpoint_status_[method] = 1;
auto instrumentation = art::Runtime::Current()->GetInstrumentation();
if (instrumentation->IsForcedInterpretOnly()) {
// We are already interpreting everything so no need to do anything.
@@ -205,22 +196,17 @@
// 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!";
- breakpoint_status_[method] -= 1;
- is_last_breakpoint = (breakpoint_status_[method] == 0);
- }
+ DCHECK_GT(deopter_count_, 0u) << "unexpected deotpimization request";
+ DCHECK(MethodHasBreakpointsLocked(method)) << "Breakpoint on a method was removed without "
+ << "breakpoints present!";
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 (is_last_breakpoint) {
+ } else if (breakpoint_status_[method] == 0) {
if (UNLIKELY(is_default)) {
RemoveDeoptimizeAllMethodsLocked(self);
} else {
diff --git a/openjdkjvmti/deopt_manager.h b/openjdkjvmti/deopt_manager.h
index a38690c..a495b68 100644
--- a/openjdkjvmti/deopt_manager.h
+++ b/openjdkjvmti/deopt_manager.h
@@ -32,7 +32,6 @@
#ifndef ART_OPENJDKJVMTI_DEOPT_MANAGER_H_
#define ART_OPENJDKJVMTI_DEOPT_MANAGER_H_
-#include <atomic>
#include <unordered_map>
#include "jni.h"
@@ -108,17 +107,9 @@
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(breakpoint_status_lock_);
+ REQUIRES(deoptimization_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
@@ -165,20 +156,13 @@
// 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(breakpoint_status_lock_);
+ GUARDED_BY(deoptimization_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 b83310d..bf2e6cd 100644
--- a/openjdkjvmti/ti_method.cc
+++ b/openjdkjvmti/ti_method.cc
@@ -915,9 +915,6 @@
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 ec3e10e..84a148f 100644
--- a/runtime/instrumentation.cc
+++ b/runtime/instrumentation.cc
@@ -139,14 +139,10 @@
bool Instrumentation::NeedDebugVersionFor(ArtMethod* method) const
REQUIRES_SHARED(Locks::mutator_lock_) {
- art::Runtime* runtime = Runtime::Current();
- return runtime->IsJavaDebuggable() &&
+ return Runtime::Current()->IsJavaDebuggable() &&
!method->IsNative() &&
!method->IsProxyMethod() &&
- // 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));
+ Runtime::Current()->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 a685891..cdb8f6a 100644
--- a/test/1935-get-set-current-frame-jit/expected.txt
+++ b/test/1935-get-set-current-frame-jit/expected.txt
@@ -1,5 +1,7 @@
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 671493b..714a98a 100644
--- a/test/1935-get-set-current-frame-jit/src/Main.java
+++ b/test/1935-get-set-current-frame-jit/src/Main.java
@@ -21,7 +21,6 @@
import java.lang.reflect.Executable;
import java.lang.reflect.Method;
import java.nio.ByteBuffer;
-import java.time.Instant;
import java.util.concurrent.Semaphore;
import java.util.Arrays;
import java.util.Collection;
@@ -50,11 +49,9 @@
public static class IntRunner implements Runnable {
private volatile boolean continueBusyLoop;
private volatile boolean inBusyLoop;
- private final boolean expectOsr;
- public IntRunner(boolean expectOsr) {
+ public IntRunner() {
this.continueBusyLoop = true;
this.inBusyLoop = false;
- this.expectOsr = expectOsr;
}
public void run() {
int TARGET = 42;
@@ -62,22 +59,14 @@
while (continueBusyLoop) {
inBusyLoop = true;
}
- // Wait up to 300 seconds for OSR to kick in if we expect it. If we don't give up after only
- // 3 seconds.
- Instant osrDeadline = Instant.now().plusSeconds(expectOsr ? 600 : 3);
- do {
+ int i = 0;
+ while (Main.isInterpreted() && i < 10000) {
Main.ensureJitCompiled(IntRunner.class, "run");
- } while (hasJit() && !Main.isInOsrCode("run") && osrDeadline.compareTo(Instant.now()) > 0);
- // 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.
- 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.");
- }
+ i++;
}
+ // We shouldn't be doing OSR since we are using JVMTI and the get/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")));
reportValue(TARGET);
}
public void waitForBusyLoopStart() { while (!inBusyLoop) {} }
@@ -89,7 +78,7 @@
public static void runGet() throws Exception {
Method target = IntRunner.class.getDeclaredMethod("run");
// Get Int
- IntRunner int_runner = new IntRunner(true);
+ IntRunner int_runner = new IntRunner();
Thread target_get = new Thread(int_runner, "GetLocalInt - Target");
target_get.start();
int_runner.waitForBusyLoopStart();
@@ -119,7 +108,7 @@
public static void runSet() throws Exception {
Method target = IntRunner.class.getDeclaredMethod("run");
// Set Int
- IntRunner int_runner = new IntRunner(false);
+ IntRunner int_runner = new IntRunner();
Thread target_set = new Thread(int_runner, "SetLocalInt - Target");
target_set.start();
int_runner.waitForBusyLoopStart();