Sweep interpreter caches using checkpoint during GC

Interpreter caches are mutator thread-local caches. Currently, the way
GC sweeps these caches breaks that invariant and requires
synchronization for correct behavior.

In this CL we take a different approach of sweeping the caches
during GC using checkpoint (or in a stop-the-world pause),
eliminating the need for synchronization.

Bug: 235988901
Test: art/test/testrunner/testrunner.py --host --debug --interpreter
Change-Id: I8e8bbfc2d424bdce9c1c8c662782febe18678193

14 files changed, 42 insertions, 30 deletions

diff --git a/openjdkjvmti/ti_heap.cc b/openjdkjvmti/ti_heap.cc
index 2a1d44207a..01864cd312 100644
--- a/openjdkjvmti/ti_heap.cc
+++ b/openjdkjvmti/ti_heap.cc
@@ -1851,7 +1851,9 @@ static void ReplaceWeakRoots(art::Thread* self,
     const ObjectMap& map_;
   };
   ReplaceWeaksVisitor rwv(map);
-  art::Runtime::Current()->SweepSystemWeaks(&rwv);
+  art::Runtime* runtime = art::Runtime::Current();
+  runtime->SweepSystemWeaks(&rwv);
+  runtime->GetThreadList()->SweepInterpreterCaches(&rwv);
   // Re-add the object tags. At this point all weak-references to the old_obj_ptr are gone.
   event_handler->ForEachEnv(self, [&](ArtJvmTiEnv* env) {
     // Cannot have REQUIRES(art::Locks::mutator_lock_) since ForEachEnv doesn't require it.
diff --git a/runtime/gc/collector/concurrent_copying.cc b/runtime/gc/collector/concurrent_copying.cc
index f3c61e3ef4..ba9d48a1bd 100644
--- a/runtime/gc/collector/concurrent_copying.cc
+++ b/runtime/gc/collector/concurrent_copying.cc
@@ -1745,6 +1745,10 @@ class ConcurrentCopying::DisableMarkingCheckpoint : public Closure {
            thread->IsSuspended() ||
            thread->GetState() == ThreadState::kWaitingPerformingGc)
         << thread->GetState() << " thread " << thread << " self " << self;
+    // We sweep interpreter caches here so that it can be done after all
+    // reachable objects are marked and the mutators can sweep their caches
+    // without synchronization.
+    thread->SweepInterpreterCache(concurrent_copying_);
     // Disable the thread-local is_gc_marking flag.
     // Note a thread that has just started right before this checkpoint may have already this flag
     // set to false, which is ok.
diff --git a/runtime/gc/collector/mark_compact.cc b/runtime/gc/collector/mark_compact.cc
index 3b6dffbcd1..a14bfd5fba 100644
--- a/runtime/gc/collector/mark_compact.cc
+++ b/runtime/gc/collector/mark_compact.cc
@@ -1688,6 +1688,9 @@ void MarkCompact::PreCompactionPhase() {
     std::list<Thread*> thread_list = runtime->GetThreadList()->GetList();
     for (Thread* thread : thread_list) {
       thread->VisitRoots(this, kVisitRootFlagAllRoots);
+      // Interpreter cache is thread-local so it needs to be swept either in a
+      // checkpoint, or a stop-the-world pause.
+      thread->SweepInterpreterCache(this);
       thread->AdjustTlab(black_objs_slide_diff_);
     }
   }
diff --git a/runtime/gc/collector/mark_sweep.cc b/runtime/gc/collector/mark_sweep.cc
index bd5ce37b2c..8e3899adb1 100644
--- a/runtime/gc/collector/mark_sweep.cc
+++ b/runtime/gc/collector/mark_sweep.cc
@@ -340,6 +340,8 @@ void MarkSweep::ReclaimPhase() {
   Thread* const self = Thread::Current();
   // Process the references concurrently.
   ProcessReferences(self);
+  // There is no need to sweep interpreter caches as this GC doesn't move
+  // objects and hence would be a nop.
   SweepSystemWeaks(self);
   Runtime* const runtime = Runtime::Current();
   runtime->AllowNewSystemWeaks();
@@ -1127,7 +1129,9 @@ void MarkSweep::VerifySystemWeaks() {
   TimingLogger::ScopedTiming t(__FUNCTION__, GetTimings());
   // Verify system weaks, uses a special object visitor which returns the input object.
   VerifySystemWeakVisitor visitor(this);
-  Runtime::Current()->SweepSystemWeaks(&visitor);
+  Runtime* runtime = Runtime::Current();
+  runtime->SweepSystemWeaks(&visitor);
+  runtime->GetThreadList()->SweepInterpreterCaches(&visitor);
 }
 
 class MarkSweep::CheckpointMarkThreadRoots : public Closure, public RootVisitor {
diff --git a/runtime/gc/collector/mark_sweep.h b/runtime/gc/collector/mark_sweep.h
index 6af7c54600..12fd7f9995 100644
--- a/runtime/gc/collector/mark_sweep.h
+++ b/runtime/gc/collector/mark_sweep.h
@@ -181,7 +181,7 @@ class MarkSweep : public GarbageCollector {
       REQUIRES_SHARED(Locks::heap_bitmap_lock_, Locks::mutator_lock_);
 
   void VerifySystemWeaks()
-      REQUIRES_SHARED(Locks::mutator_lock_, Locks::heap_bitmap_lock_);
+      REQUIRES(Locks::mutator_lock_) REQUIRES_SHARED(Locks::heap_bitmap_lock_);
 
   // Verify that an object is live, either in a live bitmap or in the allocation stack.
   void VerifyIsLive(const mirror::Object* obj)
diff --git a/runtime/gc/collector/semi_space.cc b/runtime/gc/collector/semi_space.cc
index 53b060483f..acd4807a4f 100644
--- a/runtime/gc/collector/semi_space.cc
+++ b/runtime/gc/collector/semi_space.cc
@@ -500,7 +500,9 @@ void SemiSpace::MarkRoots() {
 
 void SemiSpace::SweepSystemWeaks() {
   TimingLogger::ScopedTiming t(__FUNCTION__, GetTimings());
-  Runtime::Current()->SweepSystemWeaks(this);
+  Runtime* runtime = Runtime::Current();
+  runtime->SweepSystemWeaks(this);
+  runtime->GetThreadList()->SweepInterpreterCaches(this);
 }
 
 bool SemiSpace::ShouldSweepSpace(space::ContinuousSpace* space) const {
diff --git a/runtime/gc/collector/semi_space.h b/runtime/gc/collector/semi_space.h
index 245ea10558..6d3ac0846e 100644
--- a/runtime/gc/collector/semi_space.h
+++ b/runtime/gc/collector/semi_space.h
@@ -143,7 +143,7 @@ class SemiSpace : public GarbageCollector {
   void SweepLargeObjects(bool swap_bitmaps) REQUIRES(Locks::heap_bitmap_lock_);
 
   void SweepSystemWeaks()
-      REQUIRES_SHARED(Locks::heap_bitmap_lock_, Locks::mutator_lock_);
+      REQUIRES_SHARED(Locks::heap_bitmap_lock_) REQUIRES(Locks::mutator_lock_);
 
   void VisitRoots(mirror::Object*** roots, size_t count, const RootInfo& info) override
       REQUIRES(Locks::mutator_lock_, Locks::heap_bitmap_lock_);
diff --git a/runtime/interpreter/interpreter_cache-inl.h b/runtime/interpreter/interpreter_cache-inl.h
index 269f5fa9ab..804d382877 100644
--- a/runtime/interpreter/interpreter_cache-inl.h
+++ b/runtime/interpreter/interpreter_cache-inl.h
@@ -35,13 +35,9 @@ inline bool InterpreterCache::Get(Thread* self, const void* key, /* out */ size_
 
 inline void InterpreterCache::Set(Thread* self, const void* key, size_t value) {
   DCHECK(self->GetInterpreterCache() == this) << "Must be called from owning thread";
-
-  // For simplicity, only update the cache if weak ref accesses are enabled. If
-  // they are disabled, this means the CC GC could be processing the cache, and
-  // reading it concurrently.
-  if (!gUseReadBarrier || self->GetWeakRefAccessEnabled()) {
-    data_[IndexOf(key)] = Entry{key, value};
-  }
+  // Simple store works here as the cache is always read/written by the owning
+  // thread only (or in a stop-the-world pause).
+  data_[IndexOf(key)] = Entry{key, value};
 }
 
 }  // namespace art
diff --git a/runtime/runtime.cc b/runtime/runtime.cc
index 6e583afe82..255295a8ed 100644
--- a/runtime/runtime.cc
+++ b/runtime/runtime.cc
@@ -803,7 +803,6 @@ void Runtime::SweepSystemWeaks(IsMarkedVisitor* visitor) {
     // from mutators. See b/32167580.
     GetJit()->GetCodeCache()->SweepRootTables(visitor);
   }
-  Thread::SweepInterpreterCaches(visitor);
 
   // All other generic system-weak holders.
   for (gc::AbstractSystemWeakHolder* holder : system_weak_holders_) {
diff --git a/runtime/runtime.h b/runtime/runtime.h
index a932718ab1..3cc689ecfa 100644
--- a/runtime/runtime.h
+++ b/runtime/runtime.h
@@ -459,8 +459,7 @@ class Runtime {
 
   // Sweep system weaks, the system weak is deleted if the visitor return null. Otherwise, the
   // system weak is updated to be the visitor's returned value.
-  void SweepSystemWeaks(IsMarkedVisitor* visitor)
-      REQUIRES_SHARED(Locks::mutator_lock_);
+  void SweepSystemWeaks(IsMarkedVisitor* visitor) REQUIRES_SHARED(Locks::mutator_lock_);
 
   // Walk all reflective objects and visit their targets as well as any method/fields held by the
   // runtime threads that are marked as being reflective.
diff --git a/runtime/thread.cc b/runtime/thread.cc
index 1d6b961c18..8e8e45f725 100644
--- a/runtime/thread.cc
+++ b/runtime/thread.cc
@@ -4458,9 +4458,6 @@ void Thread::VisitRoots(RootVisitor* visitor) {
 
 static void SweepCacheEntry(IsMarkedVisitor* visitor, const Instruction* inst, size_t* value)
     REQUIRES_SHARED(Locks::mutator_lock_) {
-  // WARNING: The interpreter will not modify the cache while this method is running in GC.
-  //          However, ClearAllInterpreterCaches can still run if any dex file is closed.
-  //          Therefore the cache entry can be nulled at any point through this method.
   if (inst == nullptr) {
     return;
   }
@@ -4509,16 +4506,12 @@ static void SweepCacheEntry(IsMarkedVisitor* visitor, const Instruction* inst, s
       // New opcode is using the cache. We need to explicitly handle it in this method.
       DCHECK(false) << "Unhandled opcode " << inst->Opcode();
   }
-};
+}
 
-void Thread::SweepInterpreterCaches(IsMarkedVisitor* visitor) {
-  MutexLock mu(Thread::Current(), *Locks::thread_list_lock_);
-  Runtime::Current()->GetThreadList()->ForEach([visitor](Thread* thread) {
-    Locks::mutator_lock_->AssertSharedHeld(Thread::Current());
-    for (InterpreterCache::Entry& entry : thread->GetInterpreterCache()->GetArray()) {
-      SweepCacheEntry(visitor, reinterpret_cast<const Instruction*>(entry.first), &entry.second);
-    }
-  });
+void Thread::SweepInterpreterCache(IsMarkedVisitor* visitor) {
+  for (InterpreterCache::Entry& entry : GetInterpreterCache()->GetArray()) {
+    SweepCacheEntry(visitor, reinterpret_cast<const Instruction*>(entry.first), &entry.second);
+  }
 }
 
 // FIXME: clang-r433403 reports the below function exceeds frame size limit.
diff --git a/runtime/thread.h b/runtime/thread.h
index b218e20b90..18a00b80bb 100644
--- a/runtime/thread.h
+++ b/runtime/thread.h
@@ -736,6 +736,9 @@ class Thread {
     return tlsPtr_.frame_id_to_shadow_frame != nullptr;
   }
 
+  // This is done by GC using a checkpoint (or in a stop-the-world pause).
+  void SweepInterpreterCache(IsMarkedVisitor* visitor) REQUIRES_SHARED(Locks::mutator_lock_);
+
   void VisitRoots(RootVisitor* visitor, VisitRootFlags flags)
       REQUIRES_SHARED(Locks::mutator_lock_);
 
@@ -1609,9 +1612,6 @@ class Thread {
   template <bool kPrecise>
   void VisitRoots(RootVisitor* visitor) REQUIRES_SHARED(Locks::mutator_lock_);
 
-  static void SweepInterpreterCaches(IsMarkedVisitor* visitor)
-      REQUIRES_SHARED(Locks::mutator_lock_);
-
   static bool IsAotCompiler();
 
   void ReleaseLongJumpContextInternal();
diff --git a/runtime/thread_list.cc b/runtime/thread_list.cc
index 6b23c349ef..f57ce76816 100644
--- a/runtime/thread_list.cc
+++ b/runtime/thread_list.cc
@@ -1413,6 +1413,13 @@ void ThreadList::VisitReflectiveTargets(ReflectiveValueVisitor *visitor) const {
   }
 }
 
+void ThreadList::SweepInterpreterCaches(IsMarkedVisitor* visitor) const {
+  MutexLock mu(Thread::Current(), *Locks::thread_list_lock_);
+  for (const auto& thread : list_) {
+    thread->SweepInterpreterCache(visitor);
+  }
+}
+
 uint32_t ThreadList::AllocThreadId(Thread* self) {
   MutexLock mu(self, *Locks::allocated_thread_ids_lock_);
   for (size_t i = 0; i < allocated_ids_.size(); ++i) {
diff --git a/runtime/thread_list.h b/runtime/thread_list.h
index 29b0c52186..17a53fa58d 100644
--- a/runtime/thread_list.h
+++ b/runtime/thread_list.h
@@ -167,6 +167,9 @@ class ThreadList {
 
   void VisitReflectiveTargets(ReflectiveValueVisitor* visitor) const REQUIRES(Locks::mutator_lock_);
 
+  void SweepInterpreterCaches(IsMarkedVisitor* visitor) const
+      REQUIRES(Locks::mutator_lock_, !Locks::thread_list_lock_);
+
   // Return a copy of the thread list.
   std::list<Thread*> GetList() REQUIRES(Locks::thread_list_lock_) {
     return list_;