Merge "Remove Deoptimization code from class transformation."

12 files changed, 26 insertions, 111 deletions

diff --git a/runtime/art_method.cc b/runtime/art_method.cc
index ec789f51ef..61ff41742b 100644
--- a/runtime/art_method.cc
+++ b/runtime/art_method.cc
@@ -55,6 +55,17 @@ extern "C" void art_quick_invoke_stub(ArtMethod*, uint32_t*, uint32_t, Thread*,
 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 f145d7c416..e4db2c7324 100644
--- a/runtime/art_method.h
+++ b/runtime/art_method.h
@@ -570,6 +570,8 @@ class ArtMethod FINAL {
   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 65178ed066..e928344fb6 100644
--- a/runtime/class_linker-inl.h
+++ b/runtime/class_linker-inl.h
@@ -99,7 +99,7 @@ inline mirror::Class* ClassLinker::ResolveType(dex::TypeIndex type_idx, ArtMetho
   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 @@ inline ArtMethod* ClassLinker::ResolveMethod(Thread* self,
   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 3005c2a2f1..f11e2cba10 100644
--- a/runtime/instrumentation.cc
+++ b/runtime/instrumentation.cc
@@ -558,10 +558,8 @@ void Instrumentation::RemoveListener(InstrumentationListener* listener, uint32_t
 }
 
 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 @@ Instrumentation::InstrumentationLevel Instrumentation::GetCurrentInstrumentation
 }
 
 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 @@ void Instrumentation::ConfigureStubs(const char* key, InstrumentationLevel desir
   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 @@ bool Instrumentation::ShouldNotifyMethodEnterExitEvents() const {
   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 39606f73f6..01071a541f 100644
--- a/runtime/instrumentation.h
+++ b/runtime/instrumentation.h
@@ -133,9 +133,6 @@ class Instrumentation {
   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 @@ class Instrumentation {
   }
   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 b76d74ae79..da4757f50f 100644
--- a/runtime/openjdkjvmti/ti_redefine.cc
+++ b/runtime/openjdkjvmti/ti_redefine.cc
@@ -75,9 +75,7 @@ class ObsoleteMethodStackVisitor : public art::StackVisitor {
                        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 @@ class ObsoleteMethodStackVisitor : public art::StackVisitor {
 
   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 @@ class ObsoleteMethodStackVisitor : public art::StackVisitor {
   // 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 @@ void Redefiner::ClassRedefinition::FillObsoleteMethodMap(
   }
 }
 
-// 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 @@ jvmtiError Redefiner::Run() {
     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 85df6e1024..fc7a3b3dec 100644
--- a/runtime/openjdkjvmti/ti_redefine.h
+++ b/runtime/openjdkjvmti/ti_redefine.h
@@ -239,14 +239,6 @@ class Redefiner {
       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 2add955f8e..3a9975a4e2 100644
--- a/runtime/trace.cc
+++ b/runtime/trace.cc
@@ -905,6 +905,9 @@ void Trace::FlushBuf() {
 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 1b03200ba5..f4c45fc032 100644
--- a/test/916-obsolete-jit/src/Main.java
+++ b/test/916-obsolete-jit/src/Main.java
@@ -116,26 +116,17 @@ public class Main {
     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 @@ public class Main {
     // 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 @@ public class Main {
     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 f4dcf09dc6..9c9adbc22d 100644
--- a/test/916-obsolete-jit/src/Transform.java
+++ b/test/916-obsolete-jit/src/Transform.java
@@ -29,13 +29,7 @@ class Transform {
     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 1d893f125a..ffb9897236 100644
--- a/test/919-obsolete-fields/src/Main.java
+++ b/test/919-obsolete-fields/src/Main.java
@@ -120,13 +120,6 @@ public class Main {
     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 abd1d19b66..c8e3cbd934 100644
--- a/test/919-obsolete-fields/src/Transform.java
+++ b/test/919-obsolete-fields/src/Transform.java
@@ -34,12 +34,7 @@ class Transform {
     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");