Enable lockless decoding of weak globals

Will help speed up decoding weak DexCache roots.

Change-Id: I9a68beb4106cbd383111a30e249c9b0149064e78
diff --git a/runtime/indirect_reference_table.h b/runtime/indirect_reference_table.h
index c398555..d13526b 100644
--- a/runtime/indirect_reference_table.h
+++ b/runtime/indirect_reference_table.h
@@ -290,9 +290,7 @@
 
   // Synchronized get which reads a reference, acquiring a lock if necessary.
   template<ReadBarrierOption kReadBarrierOption = kWithReadBarrier>
-  mirror::Object* SynchronizedGet(Thread* /*self*/, ReaderWriterMutex* /*mutex*/,
-                                  IndirectRef iref) const
-      SHARED_REQUIRES(Locks::mutator_lock_) {
+  mirror::Object* SynchronizedGet(IndirectRef iref) const SHARED_REQUIRES(Locks::mutator_lock_) {
     return Get<kReadBarrierOption>(iref);
   }
 
diff --git a/runtime/java_vm_ext.cc b/runtime/java_vm_ext.cc
index ef7a924..8060e3d 100644
--- a/runtime/java_vm_ext.cc
+++ b/runtime/java_vm_ext.cc
@@ -375,7 +375,7 @@
       unchecked_functions_(&gJniInvokeInterface),
       weak_globals_lock_("JNI weak global reference table lock", kJniWeakGlobalsLock),
       weak_globals_(kWeakGlobalsInitial, kWeakGlobalsMax, kWeakGlobal),
-      allow_new_weak_globals_(true),
+      allow_accessing_weak_globals_(true),
       weak_globals_add_condition_("weak globals add condition", weak_globals_lock_) {
   functions = unchecked_functions_;
   SetCheckJniEnabled(runtime_options.Exists(RuntimeArgumentMap::CheckJni));
@@ -473,8 +473,7 @@
     return nullptr;
   }
   MutexLock mu(self, weak_globals_lock_);
-  while (UNLIKELY((!kUseReadBarrier && !allow_new_weak_globals_) ||
-                  (kUseReadBarrier && !self->GetWeakRefAccessEnabled()))) {
+  while (UNLIKELY(!MayAccessWeakGlobals(self))) {
     weak_globals_add_condition_.WaitHoldingLocks(self);
   }
   IndirectRef ref = weak_globals_.Add(IRT_FIRST_SEGMENT, obj);
@@ -542,14 +541,19 @@
 }
 
 void JavaVMExt::DisallowNewWeakGlobals() {
-  MutexLock mu(Thread::Current(), weak_globals_lock_);
-  allow_new_weak_globals_ = false;
+  Thread* const self = Thread::Current();
+  MutexLock mu(self, weak_globals_lock_);
+  // DisallowNewWeakGlobals is only called by CMS during the pause. It is required to have the
+  // mutator lock exclusively held so that we don't have any threads in the middle of
+  // DecodeWeakGlobal.
+  Locks::mutator_lock_->AssertExclusiveHeld(self);
+  allow_accessing_weak_globals_.StoreSequentiallyConsistent(false);
 }
 
 void JavaVMExt::AllowNewWeakGlobals() {
   Thread* self = Thread::Current();
   MutexLock mu(self, weak_globals_lock_);
-  allow_new_weak_globals_ = true;
+  allow_accessing_weak_globals_.StoreSequentiallyConsistent(true);
   weak_globals_add_condition_.Broadcast(self);
 }
 
@@ -557,7 +561,7 @@
   // Lock and unlock once to ensure that no threads are still in the
   // middle of adding new weak globals.
   MutexLock mu(Thread::Current(), weak_globals_lock_);
-  CHECK(!allow_new_weak_globals_);
+  CHECK(!allow_accessing_weak_globals_.LoadSequentiallyConsistent());
 }
 
 void JavaVMExt::BroadcastForNewWeakGlobals() {
@@ -567,8 +571,8 @@
   weak_globals_add_condition_.Broadcast(self);
 }
 
-mirror::Object* JavaVMExt::DecodeGlobal(Thread* self, IndirectRef ref) {
-  return globals_.SynchronizedGet(self, &globals_lock_, ref);
+mirror::Object* JavaVMExt::DecodeGlobal(IndirectRef ref) {
+  return globals_.SynchronizedGet(ref);
 }
 
 void JavaVMExt::UpdateGlobal(Thread* self, IndirectRef ref, mirror::Object* result) {
@@ -576,7 +580,25 @@
   globals_.Update(ref, result);
 }
 
+inline bool JavaVMExt::MayAccessWeakGlobals(Thread* self) const {
+  return MayAccessWeakGlobalsUnlocked(self);
+}
+
+inline bool JavaVMExt::MayAccessWeakGlobalsUnlocked(Thread* self) const {
+  return kUseReadBarrier ? self->GetWeakRefAccessEnabled() :
+      allow_accessing_weak_globals_.LoadSequentiallyConsistent();
+}
+
 mirror::Object* JavaVMExt::DecodeWeakGlobal(Thread* self, IndirectRef ref) {
+  // It is safe to access GetWeakRefAccessEnabled without the lock since CC uses checkpoints to call
+  // SetWeakRefAccessEnabled, and the other collectors only modify allow_accessing_weak_globals_
+  // when the mutators are paused.
+  // This only applies in the case where MayAccessWeakGlobals goes from false to true. In the other
+  // case, it may be racy, this is benign since DecodeWeakGlobalLocked does the correct behavior
+  // if MayAccessWeakGlobals is false.
+  if (LIKELY(MayAccessWeakGlobalsUnlocked(self))) {
+    return weak_globals_.SynchronizedGet(ref);
+  }
   MutexLock mu(self, weak_globals_lock_);
   return DecodeWeakGlobalLocked(self, ref);
 }
@@ -585,8 +607,7 @@
   if (kDebugLocking) {
     weak_globals_lock_.AssertHeld(self);
   }
-  while (UNLIKELY((!kUseReadBarrier && !allow_new_weak_globals_) ||
-                  (kUseReadBarrier && !self->GetWeakRefAccessEnabled()))) {
+  while (UNLIKELY(!MayAccessWeakGlobals(self))) {
     weak_globals_add_condition_.WaitHoldingLocks(self);
   }
   return weak_globals_.Get(ref);
diff --git a/runtime/java_vm_ext.h b/runtime/java_vm_ext.h
index e80266f..d68a85f 100644
--- a/runtime/java_vm_ext.h
+++ b/runtime/java_vm_ext.h
@@ -126,7 +126,7 @@
   void SweepJniWeakGlobals(IsMarkedVisitor* visitor)
       SHARED_REQUIRES(Locks::mutator_lock_) REQUIRES(!weak_globals_lock_);
 
-  mirror::Object* DecodeGlobal(Thread* self, IndirectRef ref)
+  mirror::Object* DecodeGlobal(IndirectRef ref)
       SHARED_REQUIRES(Locks::mutator_lock_);
 
   void UpdateGlobal(Thread* self, IndirectRef ref, mirror::Object* result)
@@ -155,6 +155,12 @@
       REQUIRES(!globals_lock_);
 
  private:
+  // Return true if self can currently access weak globals.
+  bool MayAccessWeakGlobalsUnlocked(Thread* self) const SHARED_REQUIRES(Locks::mutator_lock_);
+  bool MayAccessWeakGlobals(Thread* self) const
+      SHARED_REQUIRES(Locks::mutator_lock_)
+      REQUIRES(weak_globals_lock_);
+
   Runtime* const runtime_;
 
   // Used for testing. By default, we'll LOG(FATAL) the reason.
@@ -184,8 +190,10 @@
   // Since weak_globals_ contain weak roots, be careful not to
   // directly access the object references in it. Use Get() with the
   // read barrier enabled.
-  IndirectReferenceTable weak_globals_ GUARDED_BY(weak_globals_lock_);
-  bool allow_new_weak_globals_ GUARDED_BY(weak_globals_lock_);
+  // Not guarded by weak_globals_lock since we may use SynchronizedGet in DecodeWeakGlobal.
+  IndirectReferenceTable weak_globals_;
+  // Not guarded by weak_globals_lock since we may use SynchronizedGet in DecodeWeakGlobal.
+  Atomic<bool> allow_accessing_weak_globals_;
   ConditionVariable weak_globals_add_condition_ GUARDED_BY(weak_globals_lock_);
 
   DISALLOW_COPY_AND_ASSIGN(JavaVMExt);
diff --git a/runtime/thread.cc b/runtime/thread.cc
index 63534b1..9929487 100644
--- a/runtime/thread.cc
+++ b/runtime/thread.cc
@@ -1728,7 +1728,7 @@
       result = nullptr;
     }
   } else if (kind == kGlobal) {
-    result = tlsPtr_.jni_env->vm->DecodeGlobal(const_cast<Thread*>(this), ref);
+    result = tlsPtr_.jni_env->vm->DecodeGlobal(ref);
   } else {
     DCHECK_EQ(kind, kWeakGlobal);
     result = tlsPtr_.jni_env->vm->DecodeWeakGlobal(const_cast<Thread*>(this), ref);