Revert^2 "Reduce pauses for weak reference access"

This reverts commit 07cbc5ba4f117ea74faecffe14ffc0ce8aa7ee0e.

PS1 is identical to aosp/1933038 .
PS2 applies a small fix for the CMS build.

Original commit message:

Remove the "preserve references" machinery. Instead let the reference
processor inform GetReferent about its state, so that it can leverage
knowledge about whether reachable memory has in fact been completely
marked.

Restructure the ReferenceProcessor interface by adding Setup to
ensure that ReferenceProcessor fields are properly set up before
we disable the normal fast path through GetReferent.

For the CC collector, forward essentially all SoftReferences as
part of normal marking, so we don't stall weak reference access
for those.

Note briefly in the log if we encounter References that are only
reachable from finalizers.

SS and MS collectors are only minimally updated to keep them working.

We now block in GetReferent only for the hopefully very brief period of
marking objects that were initially missed as a result of a mutator
collector race. This should hopefully eliminate multi-millisecond delays
here. For 2043-reference-pauses from aosp/1952438, it reduces blocking
from over 100 msecs to under 1 on host. This is mostly due to the
better SoftReference treatment; 100 msec pauses in GetReferent()
were never near-typical.

We iteratively mark through SoftReferences now. Previously we could
mistakenly clear SoftReferences discovered while marking from the top
level ones. (Lokesh pointed this out.) To make this work, we change
ForwardSoftReferences to actually remove References from the queue,
as the comment always said it did.

This also somewhat prepares us for a much less complete solution for
pauses to access WeakGlobalRefs or other "system weaks".

This fixes a memory ordering issue for the per-thread weak reference
access flags used with the CC collector. I think the issue is still
there for the CMS collector. That requires further discussion.

Bug: 190867430
Bug: 189738006
Bug: 211784084

Test: Build and boot aosp & Treehugger; aosp/195243

Change-Id: I9aa38da6c87555302243bd6c7d460747277ba8e7

1 files changed, 150 insertions, 113 deletions

diff --git a/runtime/gc/reference_processor.cc b/runtime/gc/reference_processor.cc
index e34d140db4..5e41ee4ef8 100644
--- a/runtime/gc/reference_processor.cc
+++ b/runtime/gc/reference_processor.cc
@@ -32,6 +32,7 @@
 #include "reflection.h"
 #include "scoped_thread_state_change-inl.h"
 #include "task_processor.h"
+#include "thread-inl.h"
 #include "thread_pool.h"
 #include "well_known_classes.h"
 
@@ -42,7 +43,6 @@ static constexpr bool kAsyncReferenceQueueAdd = false;
 
 ReferenceProcessor::ReferenceProcessor()
     : collector_(nullptr),
-      preserving_references_(false),
       condition_("reference processor condition", *Locks::reference_processor_lock_) ,
       soft_reference_queue_(Locks::reference_queue_soft_references_lock_),
       weak_reference_queue_(Locks::reference_queue_weak_references_lock_),
@@ -89,17 +89,20 @@ void ReferenceProcessor::BroadcastForSlowPath(Thread* self) {
 
 ObjPtr<mirror::Object> ReferenceProcessor::GetReferent(Thread* self,
                                                        ObjPtr<mirror::Reference> reference) {
-  if (!kUseReadBarrier || self->GetWeakRefAccessEnabled()) {
-    // Under read barrier / concurrent copying collector, it's not safe to call GetReferent() when
-    // weak ref access is disabled as the call includes a read barrier which may push a ref onto the
-    // mark stack and interfere with termination of marking.
-    const ObjPtr<mirror::Object> referent = reference->GetReferent();
-    // If the referent is null then it is already cleared, we can just return null since there is no
-    // scenario where it becomes non-null during the reference processing phase.
-    if (UNLIKELY(!SlowPathEnabled()) || referent == nullptr) {
-      return referent;
-    }
+  auto slow_path_required = [this, self]() REQUIRES_SHARED(Locks::mutator_lock_) {
+    return kUseReadBarrier ? !self->GetWeakRefAccessEnabled() : SlowPathEnabled();
+  };
+  if (!slow_path_required()) {
+    return reference->GetReferent();
+  }
+  // If the referent is null then it is already cleared, we can just return null since there is no
+  // scenario where it becomes non-null during the reference processing phase.
+  // A read barrier may be unsafe here, and we use the result only when it's null or marked.
+  ObjPtr<mirror::Object> referent = reference->template GetReferent<kWithoutReadBarrier>();
+  if (referent.IsNull()) {
+    return referent;
   }
+
   bool started_trace = false;
   uint64_t start_millis;
   auto finish_trace = [](uint64_t start_millis) {
@@ -112,50 +115,47 @@ ObjPtr<mirror::Object> ReferenceProcessor::GetReferent(Thread* self,
   };
 
   MutexLock mu(self, *Locks::reference_processor_lock_);
-  while ((!kUseReadBarrier && SlowPathEnabled()) ||
-         (kUseReadBarrier && !self->GetWeakRefAccessEnabled())) {
-    ObjPtr<mirror::Object> referent = reference->GetReferent<kWithoutReadBarrier>();
-    // If the referent became cleared, return it. Don't need barrier since thread roots can't get
-    // updated until after we leave the function due to holding the mutator lock.
-    if (referent == nullptr) {
-      if (started_trace) {
-        finish_trace(start_millis);
+  // Keeping reference_processor_lock_ blocks the broadcast when we try to reenable the fast path.
+  while (slow_path_required()) {
+    DCHECK(collector_ != nullptr);
+    constexpr bool kOtherReadBarrier = kUseReadBarrier && !kUseBakerReadBarrier;
+    if (UNLIKELY(reference->IsFinalizerReferenceInstance()
+                 || rp_state_ == RpState::kStarting /* too early to determine mark state */
+                 || (kOtherReadBarrier && reference->IsPhantomReferenceInstance()))) {
+      // Odd cases in which it doesn't hurt to just wait, or the wait is likely to be very brief.
+
+      // Check and run the empty checkpoint before blocking so the empty checkpoint will work in the
+      // presence of threads blocking for weak ref access.
+      self->CheckEmptyCheckpointFromWeakRefAccess(Locks::reference_processor_lock_);
+      if (!started_trace) {
+        ATraceBegin("GetReferent blocked");
+        started_trace = true;
+        start_millis = MilliTime();
       }
-      return nullptr;
+      condition_.WaitHoldingLocks(self);
+      continue;
     }
-    // Try to see if the referent is already marked by using the is_marked_callback. We can return
-    // it to the mutator as long as the GC is not preserving references.
-    if (LIKELY(collector_ != nullptr)) {
-      // If it's null it means not marked, but it could become marked if the referent is reachable
-      // by finalizer referents. So we cannot return in this case and must block. Otherwise, we
-      // can return it to the mutator as long as the GC is not preserving references, in which
-      // case only black nodes can be safely returned. If the GC is preserving references, the
-      // mutator could take a white field from a grey or white node and move it somewhere else
-      // in the heap causing corruption since this field would get swept.
-      // Use the cached referent instead of calling GetReferent since other threads could call
-      // Reference.clear() after we did the null check resulting in a null pointer being
-      // incorrectly passed to IsMarked. b/33569625
-      ObjPtr<mirror::Object> forwarded_ref = collector_->IsMarked(referent.Ptr());
-      if (forwarded_ref != nullptr) {
-        // Non null means that it is marked.
-        if (!preserving_references_ ||
-           (LIKELY(!reference->IsFinalizerReferenceInstance()) && reference->IsUnprocessed())) {
-          if (started_trace) {
-            finish_trace(start_millis);
-          }
-          return forwarded_ref;
-        }
-      }
+    DCHECK(!reference->IsPhantomReferenceInstance());
+
+    if (rp_state_ == RpState::kInitClearingDone) {
+      // Reachable references have their final referent values.
+      break;
     }
-    // Check and run the empty checkpoint before blocking so the empty checkpoint will work in the
-    // presence of threads blocking for weak ref access.
-    self->CheckEmptyCheckpointFromWeakRefAccess(Locks::reference_processor_lock_);
-    if (!started_trace) {
-      ATraceBegin("GetReferent blocked");
-      started_trace = true;
-      start_millis = MilliTime();
+    // Although reference processing is not done, we can always predict the correct return value
+    // based on the current mark state. No additional marking from finalizers has been done, since
+    // we hold reference_processor_lock_, which is required to advance to kInitClearingDone.
+    DCHECK(rp_state_ == RpState::kInitMarkingDone);
+    // Re-load and re-check referent, since the current one may have been read before we acquired
+    // reference_lock. In particular a Reference.clear() call may have intervened. (b/33569625)
+    referent = reference->GetReferent<kWithoutReadBarrier>();
+    ObjPtr<mirror::Object> forwarded_ref =
+        referent.IsNull() ? nullptr : collector_->IsMarked(referent.Ptr());
+    // Either the referent was marked, and forwarded_ref is the correct return value, or it
+    // was not, and forwarded_ref == null, which is again the correct return value.
+    if (started_trace) {
+      finish_trace(start_millis);
     }
-    condition_.WaitHoldingLocks(self);
+    return forwarded_ref;
   }
   if (started_trace) {
     finish_trace(start_millis);
@@ -163,36 +163,64 @@ ObjPtr<mirror::Object> ReferenceProcessor::GetReferent(Thread* self,
   return reference->GetReferent();
 }
 
-void ReferenceProcessor::StartPreservingReferences(Thread* self) {
-  MutexLock mu(self, *Locks::reference_processor_lock_);
-  preserving_references_ = true;
+// Forward SoftReferences. Can be done before we disable Reference access. Only
+// invoked if we are not clearing SoftReferences.
+uint32_t ReferenceProcessor::ForwardSoftReferences(TimingLogger* timings) {
+  TimingLogger::ScopedTiming split(
+      concurrent_ ? "ForwardSoftReferences" : "(Paused)ForwardSoftReferences", timings);
+  // We used to argue that we should be smarter about doing this conditionally, but it's unclear
+  // that's actually better than the more predictable strategy of basically only clearing
+  // SoftReferences just before we would otherwise run out of memory.
+  uint32_t non_null_refs = soft_reference_queue_.ForwardSoftReferences(collector_);
+  if (ATraceEnabled()) {
+    static constexpr size_t kBufSize = 80;
+    char buf[kBufSize];
+    snprintf(buf, kBufSize, "Marking for %" PRIu32 " SoftReferences", non_null_refs);
+    ATraceBegin(buf);
+    collector_->ProcessMarkStack();
+    ATraceEnd();
+  } else {
+    collector_->ProcessMarkStack();
+  }
+  return non_null_refs;
 }
 
-void ReferenceProcessor::StopPreservingReferences(Thread* self) {
+void ReferenceProcessor::Setup(Thread* self,
+                               collector::GarbageCollector* collector,
+                               bool concurrent,
+                               bool clear_soft_references) {
+  DCHECK(collector != nullptr);
   MutexLock mu(self, *Locks::reference_processor_lock_);
-  preserving_references_ = false;
-  // We are done preserving references, some people who are blocked may see a marked referent.
-  condition_.Broadcast(self);
+  collector_ = collector;
+  rp_state_ = RpState::kStarting;
+  concurrent_ = concurrent;
+  clear_soft_references_ = clear_soft_references;
 }
 
 // Process reference class instances and schedule finalizations.
-void ReferenceProcessor::ProcessReferences(bool concurrent,
-                                           TimingLogger* timings,
-                                           bool clear_soft_references,
-                                           collector::GarbageCollector* collector) {
-  TimingLogger::ScopedTiming t(concurrent ? __FUNCTION__ : "(Paused)ProcessReferences", timings);
-  Thread* self = Thread::Current();
+// We advance rp_state_ to signal partial completion for the benefit of GetReferent.
+void ReferenceProcessor::ProcessReferences(Thread* self, TimingLogger* timings) {
+  TimingLogger::ScopedTiming t(concurrent_ ? __FUNCTION__ : "(Paused)ProcessReferences", timings);
+  if (!clear_soft_references_) {
+    // Forward any additional SoftReferences we discovered late, now that reference access has been
+    // inhibited.
+    while (!soft_reference_queue_.IsEmpty()) {
+      ForwardSoftReferences(timings);
+    }
+  }
   {
     MutexLock mu(self, *Locks::reference_processor_lock_);
-    collector_ = collector;
     if (!kUseReadBarrier) {
-      CHECK_EQ(SlowPathEnabled(), concurrent) << "Slow path must be enabled iff concurrent";
+      CHECK_EQ(SlowPathEnabled(), concurrent_) << "Slow path must be enabled iff concurrent";
     } else {
-      // Weak ref access is enabled at Zygote compaction by SemiSpace (concurrent == false).
-      CHECK_EQ(!self->GetWeakRefAccessEnabled(), concurrent);
+      // Weak ref access is enabled at Zygote compaction by SemiSpace (concurrent_ == false).
+      CHECK_EQ(!self->GetWeakRefAccessEnabled(), concurrent_);
     }
+    DCHECK(rp_state_ == RpState::kStarting);
+    rp_state_ = RpState::kInitMarkingDone;
+    condition_.Broadcast(self);
   }
-  if (kIsDebugBuild && collector->IsTransactionActive()) {
+  if (kIsDebugBuild && collector_->IsTransactionActive()) {
     // In transaction mode, we shouldn't enqueue any Reference to the queues.
     // See DelayReferenceReferent().
     DCHECK(soft_reference_queue_.IsEmpty());
@@ -200,75 +228,84 @@ void ReferenceProcessor::ProcessReferences(bool concurrent,
     DCHECK(finalizer_reference_queue_.IsEmpty());
     DCHECK(phantom_reference_queue_.IsEmpty());
   }
-  // Unless required to clear soft references with white references, preserve some white referents.
-  if (!clear_soft_references) {
-    TimingLogger::ScopedTiming split(concurrent ? "ForwardSoftReferences" :
-        "(Paused)ForwardSoftReferences", timings);
-    if (concurrent) {
-      StartPreservingReferences(self);
-    }
-    // TODO: Add smarter logic for preserving soft references. The behavior should be a conditional
-    // mark if the SoftReference is supposed to be preserved.
-    uint32_t non_null_refs = soft_reference_queue_.ForwardSoftReferences(collector);
-    if (ATraceEnabled()) {
-      static constexpr size_t kBufSize = 80;
-      char buf[kBufSize];
-      snprintf(buf, kBufSize, "Marking for %" PRIu32 " SoftReferences", non_null_refs);
-      ATraceBegin(buf);
-      collector->ProcessMarkStack();
-      ATraceEnd();
-    } else {
-      collector->ProcessMarkStack();
-    }
-    if (concurrent) {
-      StopPreservingReferences(self);
-    }
-  }
   // Clear all remaining soft and weak references with white referents.
-  soft_reference_queue_.ClearWhiteReferences(&cleared_references_, collector);
-  weak_reference_queue_.ClearWhiteReferences(&cleared_references_, collector);
+  // This misses references only reachable through finalizers.
+  soft_reference_queue_.ClearWhiteReferences(&cleared_references_, collector_);
+  weak_reference_queue_.ClearWhiteReferences(&cleared_references_, collector_);
+  // Defer PhantomReference processing until we've finished marking through finalizers.
   {
-    TimingLogger::ScopedTiming t2(concurrent ? "EnqueueFinalizerReferences" :
-        "(Paused)EnqueueFinalizerReferences", timings);
-    if (concurrent) {
-      StartPreservingReferences(self);
-    }
+    // TODO: Capture mark state of some system weaks here. If the referent was marked here,
+    // then it is now safe to return, since it can only refer to marked objects. If it becomes
+    // marked below, that is no longer guaranteed.
+    MutexLock mu(self, *Locks::reference_processor_lock_);
+    rp_state_ = RpState::kInitClearingDone;
+    // At this point, all mutator-accessible data is marked (black). Objects enqueued for
+    // finalization will only be made available to the mutator via CollectClearedReferences after
+    // we're fully done marking. Soft and WeakReferences accessible to the mutator have been
+    // processed and refer only to black objects.  Thus there is no danger of the mutator getting
+    // access to non-black objects.  Weak reference processing is still nominally suspended,
+    // But many kinds of references, including all java.lang.ref ones, are handled normally from
+    // here on. See GetReferent().
+  }
+  {
+    TimingLogger::ScopedTiming t2(
+        concurrent_ ? "EnqueueFinalizerReferences" : "(Paused)EnqueueFinalizerReferences", timings);
     // Preserve all white objects with finalize methods and schedule them for finalization.
     FinalizerStats finalizer_stats =
-        finalizer_reference_queue_.EnqueueFinalizerReferences(&cleared_references_, collector);
+        finalizer_reference_queue_.EnqueueFinalizerReferences(&cleared_references_, collector_);
     if (ATraceEnabled()) {
       static constexpr size_t kBufSize = 80;
       char buf[kBufSize];
       snprintf(buf, kBufSize, "Marking from %" PRIu32 " / %" PRIu32 " finalizers",
                finalizer_stats.num_enqueued_, finalizer_stats.num_refs_);
       ATraceBegin(buf);
-      collector->ProcessMarkStack();
+      collector_->ProcessMarkStack();
       ATraceEnd();
     } else {
-      collector->ProcessMarkStack();
-    }
-    if (concurrent) {
-      StopPreservingReferences(self);
+      collector_->ProcessMarkStack();
     }
   }
-  // Clear all finalizer referent reachable soft and weak references with white referents.
-  soft_reference_queue_.ClearWhiteReferences(&cleared_references_, collector);
-  weak_reference_queue_.ClearWhiteReferences(&cleared_references_, collector);
-  // Clear all phantom references with white referents.
-  phantom_reference_queue_.ClearWhiteReferences(&cleared_references_, collector);
+
+  // Process all soft and weak references with white referents, where the references are reachable
+  // only from finalizers. It is unclear that there is any way to do this without slightly
+  // violating some language spec. We choose to apply normal Reference processing rules for these.
+  // This exposes the following issues:
+  // 1) In the case of an unmarked referent, we may end up enqueuing an "unreachable" reference.
+  //    This appears unavoidable, since we need to clear the reference for safety, unless we
+  //    mark the referent and undo finalization decisions for objects we encounter during marking.
+  //    (Some versions of the RI seem to do something along these lines.)
+  //    Or we could clear the reference without enqueuing it, which also seems strange and
+  //    unhelpful.
+  // 2) In the case of a marked referent, we will preserve a reference to objects that may have
+  //    been enqueued for finalization. Again fixing this would seem to involve at least undoing
+  //    previous finalization / reference clearing decisions. (This would also mean than an object
+  //    containing both a strong and a WeakReference to the same referent could see the
+  //    WeakReference cleared.)
+  // The treatment in (2) is potentially quite dangerous, since Reference.get() can e.g. return a
+  // finalized object containing pointers to native objects that have already been deallocated.
+  // But it can be argued that this is just an instance of the broader rule that it is not safe
+  // for finalizers to access otherwise inaccessible finalizable objects.
+  soft_reference_queue_.ClearWhiteReferences(&cleared_references_, collector_,
+                                             /*report_cleared=*/ true);
+  weak_reference_queue_.ClearWhiteReferences(&cleared_references_, collector_,
+                                             /*report_cleared=*/ true);
+
+  // Clear all phantom references with white referents. It's fine to do this just once here.
+  phantom_reference_queue_.ClearWhiteReferences(&cleared_references_, collector_);
+
   // At this point all reference queues other than the cleared references should be empty.
   DCHECK(soft_reference_queue_.IsEmpty());
   DCHECK(weak_reference_queue_.IsEmpty());
   DCHECK(finalizer_reference_queue_.IsEmpty());
   DCHECK(phantom_reference_queue_.IsEmpty());
+
   {
     MutexLock mu(self, *Locks::reference_processor_lock_);
     // Need to always do this since the next GC may be concurrent. Doing this for only concurrent
     // could result in a stale is_marked_callback_ being called before the reference processing
     // starts since there is a small window of time where slow_path_enabled_ is enabled but the
     // callback isn't yet set.
-    collector_ = nullptr;
-    if (!kUseReadBarrier && concurrent) {
+    if (!kUseReadBarrier && concurrent_) {
       // Done processing, disable the slow path and broadcast to the waiters.
       DisableSlowPath(self);
     }