Scan dirty cards separate from aging them

Currently we scan objects corresponding to dirty cards while atomically
aging them. This could lead to following situation:

Mutator			GC-thread
-------			---------
1) dirty card
			a) age card
			b) scan ref
2) store reference

Although the write-barrier first stores the reference and then dirties
the corresponding card. However, it may reorder and lead to the above
situation.

This CL separates aging the cards and scanning them for dirty objects.
These two steps are done such that the mutator stack-scanning checkpoint
is between the two operations, ensuring that any in-progress write
barrier when cards were aged is finished before scanning the cards.

Bug: 288315519
Test: atest --rerun-until-failure 100 art-run-test-658-fp-read-barrier
Change-Id: Ia6f1c01905ba950047be528e3905809fe7a62df9

3 files changed, 35 insertions, 89 deletions

diff --git a/runtime/gc/collector/immune_spaces.h b/runtime/gc/collector/immune_spaces.h
index 5a8441a67a..950c35b27a 100644
--- a/runtime/gc/collector/immune_spaces.h
+++ b/runtime/gc/collector/immune_spaces.h
@@ -43,6 +43,7 @@ class ImmuneSpaces {
  public:
   ImmuneSpaces() {}
   void Reset();
+  bool IsEmpty() const { return spaces_.empty(); }
 
   // Add a continuous space to the immune spaces set.
   void AddSpace(space::ContinuousSpace* space) REQUIRES(Locks::heap_bitmap_lock_);
diff --git a/runtime/gc/collector/mark_compact.cc b/runtime/gc/collector/mark_compact.cc
index c3c08b12b8..768547dac0 100644
--- a/runtime/gc/collector/mark_compact.cc
+++ b/runtime/gc/collector/mark_compact.cc
@@ -562,27 +562,28 @@ void MarkCompact::AddLinearAllocSpaceData(uint8_t* begin, size_t len) {
                                          is_shared);
 }
 
-void MarkCompact::BindAndResetBitmaps() {
-  // TODO: We need to hold heap_bitmap_lock_ only for populating immune_spaces.
-  // The card-table and mod-union-table processing can be done without it. So
-  // change the logic below. Note that the bitmap clearing would require the
-  // lock.
+void MarkCompact::PrepareCardTableForMarking(bool clear_alloc_space_cards) {
   TimingLogger::ScopedTiming t(__FUNCTION__, GetTimings());
   accounting::CardTable* const card_table = heap_->GetCardTable();
+  // immune_spaces_ is emptied in InitializePhase() before marking starts. This
+  // function is invoked twice during marking. We only need to populate immune_spaces_
+  // once per GC cycle. And when it's done (below), all the immune spaces are
+  // added to it. We can never have partially filled immune_spaces_.
+  bool update_immune_spaces = immune_spaces_.IsEmpty();
   // Mark all of the spaces we never collect as immune.
   for (const auto& space : GetHeap()->GetContinuousSpaces()) {
     if (space->GetGcRetentionPolicy() == space::kGcRetentionPolicyNeverCollect ||
         space->GetGcRetentionPolicy() == space::kGcRetentionPolicyFullCollect) {
       CHECK(space->IsZygoteSpace() || space->IsImageSpace());
-      immune_spaces_.AddSpace(space);
+      if (update_immune_spaces) {
+        immune_spaces_.AddSpace(space);
+      }
       accounting::ModUnionTable* table = heap_->FindModUnionTableFromSpace(space);
       if (table != nullptr) {
         table->ProcessCards();
       } else {
-        // Keep cards aged if we don't have a mod-union table since we may need
+        // Keep cards aged if we don't have a mod-union table since we need
         // to scan them in future GCs. This case is for app images.
-        // TODO: We could probably scan the objects right here to avoid doing
-        // another scan through the card-table.
         card_table->ModifyCardsAtomic(
             space->Begin(),
             space->End(),
@@ -593,7 +594,7 @@ void MarkCompact::BindAndResetBitmaps() {
             },
             /* card modified visitor */ VoidFunctor());
       }
-    } else {
+    } else if (clear_alloc_space_cards) {
       CHECK(!space->IsZygoteSpace());
       CHECK(!space->IsImageSpace());
       // The card-table corresponding to bump-pointer and non-moving space can
@@ -606,6 +607,16 @@ void MarkCompact::BindAndResetBitmaps() {
         non_moving_space_ = space;
         non_moving_space_bitmap_ = space->GetMarkBitmap();
       }
+    } else {
+      card_table->ModifyCardsAtomic(
+          space->Begin(),
+          space->End(),
+          [](uint8_t card) {
+            return (card == gc::accounting::CardTable::kCardDirty) ?
+                       gc::accounting::CardTable::kCardAged :
+                       gc::accounting::CardTable::kCardClean;
+          },
+          /* card modified visitor */ VoidFunctor());
     }
   }
 }
@@ -3796,26 +3807,6 @@ void MarkCompact::MarkReachableObjects() {
   ProcessMarkStack();
 }
 
-class MarkCompact::CardModifiedVisitor {
- public:
-  explicit CardModifiedVisitor(MarkCompact* const mark_compact,
-                               accounting::ContinuousSpaceBitmap* const bitmap,
-                               accounting::CardTable* const card_table)
-      : visitor_(mark_compact), bitmap_(bitmap), card_table_(card_table) {}
-
-  void operator()(uint8_t* card, uint8_t expected_value, [[maybe_unused]] uint8_t new_value) const {
-    if (expected_value == accounting::CardTable::kCardDirty) {
-      uintptr_t start = reinterpret_cast<uintptr_t>(card_table_->AddrFromCard(card));
-      bitmap_->VisitMarkedRange(start, start + accounting::CardTable::kCardSize, visitor_);
-    }
-  }
-
- private:
-  ScanObjectVisitor visitor_;
-  accounting::ContinuousSpaceBitmap* bitmap_;
-  accounting::CardTable* const card_table_;
-};
-
 void MarkCompact::ScanDirtyObjects(bool paused, uint8_t minimum_age) {
   accounting::CardTable* card_table = heap_->GetCardTable();
   for (const auto& space : heap_->GetContinuousSpaces()) {
@@ -3835,58 +3826,8 @@ void MarkCompact::ScanDirtyObjects(bool paused, uint8_t minimum_age) {
       UNREACHABLE();
     }
     TimingLogger::ScopedTiming t(name, GetTimings());
-    ScanObjectVisitor visitor(this);
-    const bool is_immune_space = space->IsZygoteSpace() || space->IsImageSpace();
-    if (paused) {
-      DCHECK_EQ(minimum_age, gc::accounting::CardTable::kCardDirty);
-      // We can clear the card-table for any non-immune space.
-      if (is_immune_space) {
-        card_table->Scan</*kClearCard*/false>(space->GetMarkBitmap(),
-                                              space->Begin(),
-                                              space->End(),
-                                              visitor,
-                                              minimum_age);
-      } else {
-        card_table->Scan</*kClearCard*/true>(space->GetMarkBitmap(),
-                                             space->Begin(),
-                                             space->End(),
-                                             visitor,
-                                             minimum_age);
-      }
-    } else {
-      DCHECK_EQ(minimum_age, gc::accounting::CardTable::kCardAged);
-      accounting::ModUnionTable* table = heap_->FindModUnionTableFromSpace(space);
-      if (table) {
-        table->ProcessCards();
-        card_table->Scan</*kClearCard*/false>(space->GetMarkBitmap(),
-                                              space->Begin(),
-                                              space->End(),
-                                              visitor,
-                                              minimum_age);
-      } else {
-        CardModifiedVisitor card_modified_visitor(this, space->GetMarkBitmap(), card_table);
-        // For the alloc spaces we should age the dirty cards and clear the rest.
-        // For image and zygote-space without mod-union-table, age the dirty
-        // cards but keep the already aged cards unchanged.
-        // In either case, visit the objects on the cards that were changed from
-        // dirty to aged.
-        if (is_immune_space) {
-          card_table->ModifyCardsAtomic(space->Begin(),
-                                        space->End(),
-                                        [](uint8_t card) {
-                                          return (card == gc::accounting::CardTable::kCardClean)
-                                                  ? card
-                                                  : gc::accounting::CardTable::kCardAged;
-                                        },
-                                        card_modified_visitor);
-        } else {
-          card_table->ModifyCardsAtomic(space->Begin(),
-                                        space->End(),
-                                        AgeCardVisitor(),
-                                        card_modified_visitor);
-        }
-      }
-    }
+    card_table->Scan</*kClearCard*/ false>(
+        space->GetMarkBitmap(), space->Begin(), space->End(), ScanObjectVisitor(this), minimum_age);
   }
 }
 
@@ -3910,6 +3851,10 @@ void MarkCompact::MarkRoots(VisitRootFlags flags) {
 void MarkCompact::PreCleanCards() {
   TimingLogger::ScopedTiming t(__FUNCTION__, GetTimings());
   CHECK(!Locks::mutator_lock_->IsExclusiveHeld(thread_running_gc_));
+  // Age the card-table before thread stack scanning checkpoint in MarkRoots()
+  // as it ensures that there are no in-progress write barriers which started
+  // prior to aging the card-table.
+  PrepareCardTableForMarking(/*clear_alloc_space_cards*/ false);
   MarkRoots(static_cast<VisitRootFlags>(kVisitRootFlagClearRootLog | kVisitRootFlagNewRoots));
   RecursiveMarkDirtyObjects(/*paused*/ false, accounting::CardTable::kCardDirty - 1);
 }
@@ -3930,7 +3875,7 @@ void MarkCompact::MarkingPhase() {
   TimingLogger::ScopedTiming t(__FUNCTION__, GetTimings());
   DCHECK_EQ(thread_running_gc_, Thread::Current());
   WriterMutexLock mu(thread_running_gc_, *Locks::heap_bitmap_lock_);
-  BindAndResetBitmaps();
+  PrepareCardTableForMarking(/*clear_alloc_space_cards*/ true);
   MarkZygoteLargeObjects();
   MarkRoots(
         static_cast<VisitRootFlags>(kVisitRootFlagAllRoots | kVisitRootFlagStartLoggingNewRoots));
diff --git a/runtime/gc/collector/mark_compact.h b/runtime/gc/collector/mark_compact.h
index d73f40d436..4e9780c970 100644
--- a/runtime/gc/collector/mark_compact.h
+++ b/runtime/gc/collector/mark_compact.h
@@ -290,10 +290,10 @@ class MarkCompact final : public GarbageCollector {
   // GC cycle.
   ALWAYS_INLINE mirror::Object* PostCompactBlackObjAddr(mirror::Object* old_ref) const
       REQUIRES_SHARED(Locks::mutator_lock_);
-  // Identify immune spaces and reset card-table, mod-union-table, and mark
-  // bitmaps.
-  void BindAndResetBitmaps() REQUIRES_SHARED(Locks::mutator_lock_)
-      REQUIRES(Locks::heap_bitmap_lock_);
+  // Clears (for alloc spaces in the beginning of marking phase) or ages the
+  // card table. Also, identifies immune spaces and mark bitmap.
+  void PrepareCardTableForMarking(bool clear_alloc_space_cards)
+      REQUIRES_SHARED(Locks::mutator_lock_) REQUIRES(Locks::heap_bitmap_lock_);
 
   // Perform one last round of marking, identifying roots from dirty cards
   // during a stop-the-world (STW) pause.
@@ -767,8 +767,8 @@ class MarkCompact final : public GarbageCollector {
   class VerifyRootMarkedVisitor;
   class ScanObjectVisitor;
   class CheckpointMarkThreadRoots;
-  template<size_t kBufferSize> class ThreadRootsVisitor;
-  class CardModifiedVisitor;
+  template <size_t kBufferSize>
+  class ThreadRootsVisitor;
   class RefFieldsVisitor;
   template <bool kCheckBegin, bool kCheckEnd> class RefsUpdateVisitor;
   class ArenaPoolPageUpdater;