Remove Deoptimization code from class transformation.
Since we removed the current_method from the compiled code we don't
need to deoptimize all frames anymore.
This is a partial revert of commit dba614810.
Bug: 32369913
Bug: 33630159
Test: ART_TEST_TRACE=true \
ART_TEST_JIT=true \
ART_TEST_INTERPRETER=true mma -j40 test-art-host
Change-Id: I44a6dd89e1d96bd8c82c2c24a2f42fef023a80be
diff --git a/runtime/art_method.cc b/runtime/art_method.cc
index ec789f5..61ff417 100644
--- a/runtime/art_method.cc
+++ b/runtime/art_method.cc
@@ -55,6 +55,17 @@
extern "C" void art_quick_invoke_static_stub(ArtMethod*, uint32_t*, uint32_t, Thread*, JValue*,
const char*);
+ArtMethod* ArtMethod::GetNonObsoleteMethod() {
+ DCHECK_EQ(kRuntimePointerSize, Runtime::Current()->GetClassLinker()->GetImagePointerSize());
+ if (LIKELY(!IsObsolete())) {
+ return this;
+ } else if (IsDirect()) {
+ return &GetDeclaringClass()->GetDirectMethodsSlice(kRuntimePointerSize)[GetMethodIndex()];
+ } else {
+ return GetDeclaringClass()->GetVTableEntry(GetMethodIndex(), kRuntimePointerSize);
+ }
+}
+
ArtMethod* ArtMethod::GetSingleImplementation(PointerSize pointer_size) {
DCHECK(!IsNative());
if (!IsAbstract()) {
diff --git a/runtime/art_method.h b/runtime/art_method.h
index f145d7c..e4db2c7 100644
--- a/runtime/art_method.h
+++ b/runtime/art_method.h
@@ -570,6 +570,8 @@
ALWAYS_INLINE ArtMethod* GetInterfaceMethodIfProxy(PointerSize pointer_size)
REQUIRES_SHARED(Locks::mutator_lock_);
+ ArtMethod* GetNonObsoleteMethod() REQUIRES_SHARED(Locks::mutator_lock_);
+
// May cause thread suspension due to class resolution.
bool EqualParameters(Handle<mirror::ObjectArray<mirror::Class>> params)
REQUIRES_SHARED(Locks::mutator_lock_);
diff --git a/runtime/class_linker-inl.h b/runtime/class_linker-inl.h
index 65178ed..e928344 100644
--- a/runtime/class_linker-inl.h
+++ b/runtime/class_linker-inl.h
@@ -99,7 +99,7 @@
if (UNLIKELY(resolved_type == nullptr)) {
StackHandleScope<2> hs(Thread::Current());
ObjPtr<mirror::Class> declaring_class = referrer->GetDeclaringClass();
- Handle<mirror::DexCache> dex_cache(hs.NewHandle(declaring_class->GetDexCache()));
+ Handle<mirror::DexCache> dex_cache(hs.NewHandle(referrer->GetDexCache()));
Handle<mirror::ClassLoader> class_loader(hs.NewHandle(declaring_class->GetClassLoader()));
const DexFile& dex_file = *dex_cache->GetDexFile();
resolved_type = ResolveType(dex_file, type_idx, dex_cache, class_loader);
@@ -146,7 +146,7 @@
if (UNLIKELY(resolved_method == nullptr)) {
ObjPtr<mirror::Class> declaring_class = referrer->GetDeclaringClass();
StackHandleScope<2> hs(self);
- Handle<mirror::DexCache> h_dex_cache(hs.NewHandle(declaring_class->GetDexCache()));
+ Handle<mirror::DexCache> h_dex_cache(hs.NewHandle(referrer->GetDexCache()));
Handle<mirror::ClassLoader> h_class_loader(hs.NewHandle(declaring_class->GetClassLoader()));
const DexFile* dex_file = h_dex_cache->GetDexFile();
resolved_method = ResolveMethod<kResolveMode>(*dex_file,
diff --git a/runtime/instrumentation.cc b/runtime/instrumentation.cc
index 3005c2a..f11e2cb 100644
--- a/runtime/instrumentation.cc
+++ b/runtime/instrumentation.cc
@@ -558,10 +558,8 @@
}
Instrumentation::InstrumentationLevel Instrumentation::GetCurrentInstrumentationLevel() const {
- if (interpreter_stubs_installed_ && interpret_only_) {
+ if (interpreter_stubs_installed_) {
return InstrumentationLevel::kInstrumentWithInterpreter;
- } else if (interpreter_stubs_installed_) {
- return InstrumentationLevel::kInstrumentWithInterpreterAndJit;
} else if (entry_exit_stubs_installed_) {
return InstrumentationLevel::kInstrumentWithInstrumentationStubs;
} else {
@@ -570,11 +568,8 @@
}
bool Instrumentation::RequiresInstrumentationInstallation(InstrumentationLevel new_level) const {
- // We need to reinstall instrumentation if we go to a different level or if the current level is
- // kInstrumentWithInterpreterAndJit since that level does not force all code to always use the
- // interpreter and so we might have started running optimized code again.
- return new_level == InstrumentationLevel::kInstrumentWithInterpreterAndJit ||
- GetCurrentInstrumentationLevel() != new_level;
+ // We need to reinstall instrumentation if we go to a different level.
+ return GetCurrentInstrumentationLevel() != new_level;
}
void Instrumentation::ConfigureStubs(const char* key, InstrumentationLevel desired_level) {
@@ -605,7 +600,7 @@
Locks::mutator_lock_->AssertExclusiveHeld(self);
Locks::thread_list_lock_->AssertNotHeld(self);
if (requested_level > InstrumentationLevel::kInstrumentNothing) {
- if (requested_level >= InstrumentationLevel::kInstrumentWithInterpreterAndJit) {
+ if (requested_level == InstrumentationLevel::kInstrumentWithInterpreter) {
interpreter_stubs_installed_ = true;
entry_exit_stubs_installed_ = true;
} else {
@@ -881,14 +876,6 @@
return !deoptimization_enabled_ && !interpreter_stubs_installed_;
}
-// TODO we don't check deoptimization_enabled_ because currently there isn't really any support for
-// multiple users of instrumentation. Since this is just a temporary state anyway pending work to
-// ensure that the current_method doesn't get kept across suspend points this should be okay.
-// TODO Remove once b/33630159 is resolved.
-void Instrumentation::ReJitEverything(const char* key) {
- ConfigureStubs(key, InstrumentationLevel::kInstrumentWithInterpreterAndJit);
-}
-
void Instrumentation::DeoptimizeEverything(const char* key) {
CHECK(deoptimization_enabled_);
ConfigureStubs(key, InstrumentationLevel::kInstrumentWithInterpreter);
diff --git a/runtime/instrumentation.h b/runtime/instrumentation.h
index 39606f7..01071a5 100644
--- a/runtime/instrumentation.h
+++ b/runtime/instrumentation.h
@@ -133,9 +133,6 @@
enum class InstrumentationLevel {
kInstrumentNothing, // execute without instrumentation
kInstrumentWithInstrumentationStubs, // execute with instrumentation entry/exit stubs
- kInstrumentWithInterpreterAndJit, // execute with interpreter initially and later the JIT
- // (if it is enabled). This level is special in that it
- // always requires re-instrumentation.
kInstrumentWithInterpreter // execute with interpreter
};
@@ -166,13 +163,6 @@
}
bool ShouldNotifyMethodEnterExitEvents() const REQUIRES_SHARED(Locks::mutator_lock_);
- // Executes everything with the interpreter/jit (if available).
- void ReJitEverything(const char* key)
- REQUIRES(Locks::mutator_lock_, Roles::uninterruptible_)
- REQUIRES(!Locks::thread_list_lock_,
- !Locks::classlinker_classes_lock_,
- !deoptimized_methods_lock_);
-
// Executes everything with interpreter.
void DeoptimizeEverything(const char* key)
REQUIRES(Locks::mutator_lock_, Roles::uninterruptible_)
diff --git a/runtime/openjdkjvmti/ti_redefine.cc b/runtime/openjdkjvmti/ti_redefine.cc
index b76d74a..da4757f 100644
--- a/runtime/openjdkjvmti/ti_redefine.cc
+++ b/runtime/openjdkjvmti/ti_redefine.cc
@@ -75,9 +75,7 @@
StackVisitor::StackWalkKind::kIncludeInlinedFrames),
allocator_(allocator),
obsoleted_methods_(obsoleted_methods),
- obsolete_maps_(obsolete_maps),
- is_runtime_frame_(false) {
- }
+ obsolete_maps_(obsolete_maps) { }
~ObsoleteMethodStackVisitor() OVERRIDE {}
@@ -100,21 +98,7 @@
bool VisitFrame() OVERRIDE REQUIRES(art::Locks::mutator_lock_) {
art::ArtMethod* old_method = GetMethod();
- // TODO REMOVE once either current_method doesn't stick around through suspend points or deopt
- // works through runtime methods.
- bool prev_was_runtime_frame_ = is_runtime_frame_;
- is_runtime_frame_ = old_method->IsRuntimeMethod();
if (obsoleted_methods_.find(old_method) != obsoleted_methods_.end()) {
- // The check below works since when we deoptimize we set shadow frames for all frames until a
- // native/runtime transition and for those set the return PC to a function that will complete
- // the deoptimization. This does leave us with the unfortunate side-effect that frames just
- // below runtime frames cannot be deoptimized at the moment.
- // TODO REMOVE once either current_method doesn't stick around through suspend points or deopt
- // works through runtime methods.
- // TODO b/33616143
- if (!IsShadowFrame() && prev_was_runtime_frame_) {
- LOG(FATAL) << "Deoptimization failed due to runtime method in stack. See b/33616143";
- }
// We cannot ensure that the right dex file is used in inlined frames so we don't support
// redefining them.
DCHECK(!IsInInlinedFrame()) << "Inlined frames are not supported when using redefinition";
@@ -163,9 +147,6 @@
// values in this map must be added to the obsolete_methods_ (and obsolete_dex_caches_) fields of
// the redefined classes ClassExt by the caller.
std::unordered_map<art::ArtMethod*, art::ArtMethod*>* obsolete_maps_;
- // TODO REMOVE once either current_method doesn't stick around through suspend points or deopt
- // works through runtime methods.
- bool is_runtime_frame_;
};
jvmtiError Redefiner::IsModifiableClass(jvmtiEnv* env ATTRIBUTE_UNUSED,
@@ -508,18 +489,6 @@
}
}
-// TODO It should be possible to only deoptimize the specific obsolete methods.
-// TODO ReJitEverything can (sort of) fail. In certain cases it will skip deoptimizing some frames.
-// If one of these frames is an obsolete method we have a problem. b/33616143
-// TODO This shouldn't be necessary once we can ensure that the current method is not kept in
-// registers across suspend points.
-// TODO Pending b/33630159
-void Redefiner::EnsureObsoleteMethodsAreDeoptimized() {
- art::ScopedAssertNoThreadSuspension nts("Deoptimizing everything!");
- art::instrumentation::Instrumentation* i = runtime_->GetInstrumentation();
- i->ReJitEverything("libOpenJkdJvmti - Class Redefinition");
-}
-
bool Redefiner::ClassRedefinition::CheckClass() {
// TODO Might just want to put it in a ObjPtr and NoSuspend assert.
art::StackHandleScope<1> hs(driver_->self_);
@@ -875,15 +844,6 @@
redef.UpdateClass(klass, holder.GetNewDexCache(cnt), holder.GetOriginalDexFileBytes(cnt));
cnt++;
}
- // Ensure that obsolete methods are deoptimized. This is needed since optimized methods may have
- // pointers to their ArtMethod's stashed in registers that they then use to attempt to hit the
- // DexCache. (b/33630159)
- // TODO This can fail (leave some methods optimized) near runtime methods (including
- // quick-to-interpreter transition function).
- // TODO We probably don't need this at all once we have a way to ensure that the
- // current_art_method is never stashed in a (physical) register by the JIT and lost to the
- // stack-walker.
- EnsureObsoleteMethodsAreDeoptimized();
// TODO Verify the new Class.
// TODO Shrink the obsolete method maps if possible?
// TODO find appropriate class loader.
diff --git a/runtime/openjdkjvmti/ti_redefine.h b/runtime/openjdkjvmti/ti_redefine.h
index 85df6e1..fc7a3b3 100644
--- a/runtime/openjdkjvmti/ti_redefine.h
+++ b/runtime/openjdkjvmti/ti_redefine.h
@@ -239,14 +239,6 @@
REQUIRES_SHARED(art::Locks::mutator_lock_);
void ReleaseAllDexFiles() REQUIRES_SHARED(art::Locks::mutator_lock_);
- // Ensure that obsolete methods are deoptimized. This is needed since optimized methods may have
- // pointers to their ArtMethods stashed in registers that they then use to attempt to hit the
- // DexCache.
- void EnsureObsoleteMethodsAreDeoptimized()
- REQUIRES(art::Locks::mutator_lock_)
- REQUIRES(!art::Locks::thread_list_lock_,
- !art::Locks::classlinker_classes_lock_);
-
void RecordFailure(jvmtiError result, const std::string& class_sig, const std::string& error_msg);
void RecordFailure(jvmtiError result, const std::string& error_msg) {
RecordFailure(result, "NO CLASS", error_msg);
diff --git a/runtime/trace.cc b/runtime/trace.cc
index 2add955..3a9975a 100644
--- a/runtime/trace.cc
+++ b/runtime/trace.cc
@@ -905,6 +905,9 @@
void Trace::LogMethodTraceEvent(Thread* thread, ArtMethod* method,
instrumentation::Instrumentation::InstrumentationEvent event,
uint32_t thread_clock_diff, uint32_t wall_clock_diff) {
+ // Ensure we always use the non-obsolete version of the method so that entry/exit events have the
+ // same pointer value.
+ method = method->GetNonObsoleteMethod();
// Advance cur_offset_ atomically.
int32_t new_offset;
int32_t old_offset = 0;
diff --git a/test/916-obsolete-jit/src/Main.java b/test/916-obsolete-jit/src/Main.java
index 1b03200..f4c45fc 100644
--- a/test/916-obsolete-jit/src/Main.java
+++ b/test/916-obsolete-jit/src/Main.java
@@ -116,26 +116,17 @@
doTest(new Transform(), new TestWatcher());
}
- // TODO Workaround to (1) inability to ensure that current_method is not put into a register by
- // the JIT and/or (2) inability to deoptimize frames near runtime functions.
- // TODO Fix one/both of these issues.
- public static void doCall(Runnable r) {
- r.run();
- }
-
private static boolean interpreting = true;
private static boolean retry = false;
public static void doTest(Transform t, TestWatcher w) {
// Get the methods that need to be optimized.
Method say_hi_method;
- Method do_call_method;
// Figure out if we can even JIT at all.
final boolean has_jit = hasJit();
try {
say_hi_method = Transform.class.getDeclaredMethod(
"sayHi", Runnable.class, Consumer.class);
- do_call_method = Main.class.getDeclaredMethod("doCall", Runnable.class);
} catch (Exception e) {
System.out.println("Unable to find methods!");
e.printStackTrace();
@@ -144,9 +135,7 @@
// Makes sure the stack is the way we want it for the test and does the redefinition. It will
// set the retry boolean to true if we need to go around again due to a bad stack.
Runnable do_redefinition = () -> {
- if (has_jit &&
- (Main.isInterpretedFunction(say_hi_method, true) ||
- Main.isInterpretedFunction(do_call_method, false))) {
+ if (has_jit && Main.isInterpretedFunction(say_hi_method, true)) {
// Try again. We are not running the right jitted methods/cannot redefine them now.
retry = true;
} else {
@@ -161,7 +150,6 @@
do {
// Run ensureJitCompiled here since it might get GCd
ensureJitCompiled(Transform.class, "sayHi");
- ensureJitCompiled(Main.class, "doCall");
// Clear output.
w.clear();
// Try and redefine.
diff --git a/test/916-obsolete-jit/src/Transform.java b/test/916-obsolete-jit/src/Transform.java
index f4dcf09..9c9adbc 100644
--- a/test/916-obsolete-jit/src/Transform.java
+++ b/test/916-obsolete-jit/src/Transform.java
@@ -29,13 +29,7 @@
reporter.accept("Pre Start private method call");
Start(reporter);
reporter.accept("Post Start private method call");
- // TODO Revisit with b/33616143
- // TODO Uncomment this once either b/33630159 or b/33616143 are resolved.
- // r.run();
- // TODO This doCall function is a very temporary fix until we get either deoptimization near
- // runtime frames working, forcing current method to be always read from the stack or both
- // working.
- Main.doCall(r);
+ r.run();
reporter.accept("Pre Finish private method call");
Finish(reporter);
reporter.accept("Post Finish private method call");
diff --git a/test/919-obsolete-fields/src/Main.java b/test/919-obsolete-fields/src/Main.java
index 1d893f1..ffb9897 100644
--- a/test/919-obsolete-fields/src/Main.java
+++ b/test/919-obsolete-fields/src/Main.java
@@ -120,13 +120,6 @@
doTest(new Transform(w), w);
}
- // TODO Workaround to (1) inability to ensure that current_method is not put into a register by
- // the JIT and/or (2) inability to deoptimize frames near runtime functions.
- // TODO Fix one/both of these issues.
- public static void doCall(Runnable r) {
- r.run();
- }
-
private static boolean interpreting = true;
private static boolean retry = false;
diff --git a/test/919-obsolete-fields/src/Transform.java b/test/919-obsolete-fields/src/Transform.java
index abd1d19..c8e3cbd 100644
--- a/test/919-obsolete-fields/src/Transform.java
+++ b/test/919-obsolete-fields/src/Transform.java
@@ -34,12 +34,7 @@
reporter.accept("Pre Start private method call");
Start();
reporter.accept("Post Start private method call");
- // TODO Revist with b/33616143
- // TODO Uncomment this
- // r.run();
- // TODO This is a very temporary fix until we get either deoptimization near runtime frames
- // working, forcing current method to be always read from the stack or both working.
- Main.doCall(r);
+ r.run();
reporter.accept("Pre Finish private method call");
Finish();
reporter.accept("Post Finish private method call");