Add lockless SynchronizedGet for indirect reference table.

Used for decoding global references without holding locks.

Results on JniCallback:
Before: 615ms (3 samples).
After: 585ms (3 samples).

Change-Id: Ifcac8d0359cf658d87f695c6eb869d148af002e5

8 files changed, 132 insertions, 120 deletions

diff --git a/runtime/indirect_reference_table-inl.h b/runtime/indirect_reference_table-inl.h
new file mode 100644
index 0000000..1a28347
--- /dev/null
+++ b/runtime/indirect_reference_table-inl.h
@@ -0,0 +1,87 @@
+/*
+ * Copyright (C) 2014 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef ART_RUNTIME_INDIRECT_REFERENCE_TABLE_INL_H_
+#define ART_RUNTIME_INDIRECT_REFERENCE_TABLE_INL_H_
+
+#include "indirect_reference_table.h"
+
+#include "verify_object-inl.h"
+
+namespace art {
+namespace mirror {
+class Object;
+}  // namespace mirror
+
+// Verifies that the indirect table lookup is valid.
+// Returns "false" if something looks bad.
+inline bool IndirectReferenceTable::GetChecked(IndirectRef iref) const {
+  if (UNLIKELY(iref == nullptr)) {
+    LOG(WARNING) << "Attempt to look up NULL " << kind_;
+    return false;
+  }
+  if (UNLIKELY(GetIndirectRefKind(iref) == kSirtOrInvalid)) {
+    LOG(ERROR) << "JNI ERROR (app bug): invalid " << kind_ << " " << iref;
+    AbortIfNoCheckJNI();
+    return false;
+  }
+  const int topIndex = segment_state_.parts.topIndex;
+  int idx = ExtractIndex(iref);
+  if (UNLIKELY(idx >= topIndex)) {
+    LOG(ERROR) << "JNI ERROR (app bug): accessed stale " << kind_ << " "
+               << iref << " (index " << idx << " in a table of size " << topIndex << ")";
+    AbortIfNoCheckJNI();
+    return false;
+  }
+  if (UNLIKELY(table_[idx] == nullptr)) {
+    LOG(ERROR) << "JNI ERROR (app bug): accessed deleted " << kind_ << " " << iref;
+    AbortIfNoCheckJNI();
+    return false;
+  }
+  if (UNLIKELY(!CheckEntry("use", iref, idx))) {
+    return false;
+  }
+  return true;
+}
+
+// Make sure that the entry at "idx" is correctly paired with "iref".
+inline bool IndirectReferenceTable::CheckEntry(const char* what, IndirectRef iref, int idx) const {
+  const mirror::Object* obj = table_[idx];
+  IndirectRef checkRef = ToIndirectRef(obj, idx);
+  if (UNLIKELY(checkRef != iref)) {
+    LOG(ERROR) << "JNI ERROR (app bug): attempt to " << what
+               << " stale " << kind_ << " " << iref
+               << " (should be " << checkRef << ")";
+    AbortIfNoCheckJNI();
+    return false;
+  }
+  return true;
+}
+
+inline mirror::Object* IndirectReferenceTable::Get(IndirectRef iref) const {
+  if (!GetChecked(iref)) {
+    return kInvalidIndirectRefObject;
+  }
+  mirror::Object* obj = table_[ExtractIndex(iref)];
+  if (LIKELY(obj != kClearedJniWeakGlobal)) {
+    VerifyObject(obj);
+  }
+  return obj;
+}
+
+}  // namespace art
+
+#endif  // ART_RUNTIME_INDIRECT_REFERENCE_TABLE_INL_H_
diff --git a/runtime/indirect_reference_table.cc b/runtime/indirect_reference_table.cc
index 987df91..b81e43a 100644
--- a/runtime/indirect_reference_table.cc
+++ b/runtime/indirect_reference_table.cc
@@ -14,7 +14,8 @@
  * limitations under the License.
  */
 
-#include "indirect_reference_table.h"
+#include "indirect_reference_table-inl.h"
+
 #include "jni_internal.h"
 #include "reference_table.h"
 #include "runtime.h"
@@ -53,7 +54,7 @@ std::ostream& operator<<(std::ostream& os, const MutatorLockedDumpable<T>& rhs)
   return os;
 }
 
-static void AbortMaybe() {
+void IndirectReferenceTable::AbortIfNoCheckJNI() {
   // If -Xcheck:jni is on, it'll give a more detailed error before aborting.
   if (!Runtime::Current()->GetJavaVM()->check_jni) {
     // Otherwise, we want to abort rather than hand back a bad reference.
@@ -67,12 +68,23 @@ IndirectReferenceTable::IndirectReferenceTable(size_t initialCount,
   CHECK_LE(initialCount, maxCount);
   CHECK_NE(desiredKind, kSirtOrInvalid);
 
-  table_ = reinterpret_cast<mirror::Object**>(malloc(initialCount * sizeof(const mirror::Object*)));
-  CHECK(table_ != NULL);
-  memset(table_, 0xd1, initialCount * sizeof(const mirror::Object*));
+  std::string error_str;
+  const size_t initial_bytes = initialCount * sizeof(const mirror::Object*);
+  const size_t table_bytes = maxCount * sizeof(const mirror::Object*);
+  table_mem_map_.reset(MemMap::MapAnonymous("indirect ref table", nullptr, table_bytes,
+                                            PROT_READ | PROT_WRITE, false, &error_str));
+  CHECK(table_mem_map_.get() != nullptr) << error_str;
+
+  table_ = reinterpret_cast<mirror::Object**>(table_mem_map_->Begin());
+  CHECK(table_ != nullptr);
+  memset(table_, 0xd1, initial_bytes);
 
-  slot_data_ = reinterpret_cast<IndirectRefSlot*>(calloc(initialCount, sizeof(IndirectRefSlot)));
-  CHECK(slot_data_ != NULL);
+  const size_t slot_bytes = maxCount * sizeof(IndirectRefSlot);
+  slot_mem_map_.reset(MemMap::MapAnonymous("indirect ref table slots", nullptr, slot_bytes,
+                                           PROT_READ | PROT_WRITE, false, &error_str));
+  CHECK(slot_mem_map_.get() != nullptr) << error_str;
+  slot_data_ = reinterpret_cast<IndirectRefSlot*>(slot_mem_map_->Begin());
+  CHECK(slot_data_ != nullptr);
 
   segment_state_.all = IRT_FIRST_SEGMENT;
   alloc_entries_ = initialCount;
@@ -81,25 +93,6 @@ IndirectReferenceTable::IndirectReferenceTable(size_t initialCount,
 }
 
 IndirectReferenceTable::~IndirectReferenceTable() {
-  free(table_);
-  free(slot_data_);
-  table_ = NULL;
-  slot_data_ = NULL;
-  alloc_entries_ = max_entries_ = -1;
-}
-
-// Make sure that the entry at "idx" is correctly paired with "iref".
-bool IndirectReferenceTable::CheckEntry(const char* what, IndirectRef iref, int idx) const {
-  const mirror::Object* obj = table_[idx];
-  IndirectRef checkRef = ToIndirectRef(obj, idx);
-  if (UNLIKELY(checkRef != iref)) {
-    LOG(ERROR) << "JNI ERROR (app bug): attempt to " << what
-               << " stale " << kind_ << " " << iref
-               << " (should be " << checkRef << ")";
-    AbortMaybe();
-    return false;
-  }
-  return true;
 }
 
 IndirectRef IndirectReferenceTable::Add(uint32_t cookie, mirror::Object* obj) {
@@ -127,20 +120,6 @@ IndirectRef IndirectReferenceTable::Add(uint32_t cookie, mirror::Object* obj) {
     }
     DCHECK_GT(newSize, alloc_entries_);
 
-    table_ = reinterpret_cast<mirror::Object**>(realloc(table_, newSize * sizeof(mirror::Object*)));
-    slot_data_ = reinterpret_cast<IndirectRefSlot*>(realloc(slot_data_,
-                                                            newSize * sizeof(IndirectRefSlot)));
-    if (table_ == NULL || slot_data_ == NULL) {
-      LOG(FATAL) << "JNI ERROR (app bug): unable to expand "
-                 << kind_ << " table (from "
-                 << alloc_entries_ << " to " << newSize
-                 << ", max=" << max_entries_ << ")\n"
-                 << MutatorLockedDumpable<IndirectReferenceTable>(*this);
-    }
-
-    // Clear the newly-allocated slot_data_ elements.
-    memset(slot_data_ + alloc_entries_, 0, (newSize - alloc_entries_) * sizeof(IndirectRefSlot));
-
     alloc_entries_ = newSize;
   }
 
@@ -185,55 +164,6 @@ void IndirectReferenceTable::AssertEmpty() {
   }
 }
 
-// Verifies that the indirect table lookup is valid.
-// Returns "false" if something looks bad.
-bool IndirectReferenceTable::GetChecked(IndirectRef iref) const {
-  if (UNLIKELY(iref == NULL)) {
-    LOG(WARNING) << "Attempt to look up NULL " << kind_;
-    return false;
-  }
-  if (UNLIKELY(GetIndirectRefKind(iref) == kSirtOrInvalid)) {
-    LOG(ERROR) << "JNI ERROR (app bug): invalid " << kind_ << " " << iref;
-    AbortMaybe();
-    return false;
-  }
-
-  int topIndex = segment_state_.parts.topIndex;
-  int idx = ExtractIndex(iref);
-  if (UNLIKELY(idx >= topIndex)) {
-    LOG(ERROR) << "JNI ERROR (app bug): accessed stale " << kind_ << " "
-               << iref << " (index " << idx << " in a table of size " << topIndex << ")";
-    AbortMaybe();
-    return false;
-  }
-
-  if (UNLIKELY(table_[idx] == NULL)) {
-    LOG(ERROR) << "JNI ERROR (app bug): accessed deleted " << kind_ << " " << iref;
-    AbortMaybe();
-    return false;
-  }
-
-  if (UNLIKELY(!CheckEntry("use", iref, idx))) {
-    return false;
-  }
-
-  return true;
-}
-
-static int Find(mirror::Object* direct_pointer, int bottomIndex, int topIndex,
-                mirror::Object** table) {
-  for (int i = bottomIndex; i < topIndex; ++i) {
-    if (table[i] == direct_pointer) {
-      return i;
-    }
-  }
-  return -1;
-}
-
-bool IndirectReferenceTable::ContainsDirectPointer(mirror::Object* direct_pointer) const {
-  return Find(direct_pointer, 0, segment_state_.parts.topIndex, table_) != -1;
-}
-
 // Removes an object. We extract the table offset bits from "iref"
 // and zap the corresponding entry, leaving a hole if it's not at the top.
 // If the entry is not between the current top index and the bottom index
@@ -346,15 +276,4 @@ void IndirectReferenceTable::Dump(std::ostream& os) const {
   ReferenceTable::Dump(os, entries);
 }
 
-mirror::Object* IndirectReferenceTable::Get(IndirectRef iref) const {
-  if (!GetChecked(iref)) {
-    return kInvalidIndirectRefObject;
-  }
-  mirror::Object* obj = table_[ExtractIndex(iref)];;
-  if (obj != kClearedJniWeakGlobal) {
-    VerifyObject(obj);
-  }
-  return obj;
-}
-
 }  // namespace art
diff --git a/runtime/indirect_reference_table.h b/runtime/indirect_reference_table.h
index a2de726..f365acc 100644
--- a/runtime/indirect_reference_table.h
+++ b/runtime/indirect_reference_table.h
@@ -24,6 +24,7 @@
 
 #include "base/logging.h"
 #include "base/mutex.h"
+#include "mem_map.h"
 #include "object_callbacks.h"
 #include "offsets.h"
 
@@ -72,7 +73,7 @@ class Object;
  * To make everything fit nicely in 32-bit integers, the maximum size of
  * the table is capped at 64K.
  *
- * None of the table functions are synchronized.
+ * Only SynchronizedGet is synchronized.
  */
 
 /*
@@ -191,11 +192,6 @@ static const uint32_t IRT_FIRST_SEGMENT = 0;
  * and local refs to improve performance.  A large circular buffer might
  * reduce the amortized cost of adding global references.
  *
- * TODO: if we can guarantee that the underlying storage doesn't move,
- * e.g. by using oversized mmap regions to handle expanding tables, we may
- * be able to avoid having to synchronize lookups.  Might make sense to
- * add a "synchronized lookup" call that takes the mutex as an argument,
- * and either locks or doesn't lock based on internal details.
  */
 union IRTSegmentState {
   uint32_t          all;
@@ -234,7 +230,7 @@ class IrtIterator {
     }
   }
 
-  mirror::Object** table_;
+  mirror::Object** const table_;
   size_t i_;
   size_t capacity_;
 };
@@ -267,10 +263,15 @@ class IndirectReferenceTable {
    *
    * Returns kInvalidIndirectRefObject if iref is invalid.
    */
-  mirror::Object* Get(IndirectRef iref) const SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
-
-  // TODO: remove when we remove work_around_app_jni_bugs support.
-  bool ContainsDirectPointer(mirror::Object* direct_pointer) const;
+  mirror::Object* Get(IndirectRef iref) const SHARED_LOCKS_REQUIRED(Locks::mutator_lock_)
+      ALWAYS_INLINE;
+
+  // Synchronized get which reads a reference, acquiring a lock if necessary.
+  mirror::Object* SynchronizedGet(Thread* /*self*/, ReaderWriterMutex* /*mutex*/,
+                                  IndirectRef iref) const
+      SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
+    return Get(iref);
+  }
 
   /*
    * Remove an existing entry.
@@ -351,6 +352,9 @@ class IndirectReferenceTable {
     }
   }
 
+  // Abort if check_jni is not enabled.
+  static void AbortIfNoCheckJNI();
+
   /* extra debugging checks */
   bool GetChecked(IndirectRef) const;
   bool CheckEntry(const char*, IndirectRef, int) const;
@@ -358,6 +362,10 @@ class IndirectReferenceTable {
   /* semi-public - read/write by jni down calls */
   IRTSegmentState segment_state_;
 
+  // Mem map where we store the indirect refs.
+  UniquePtr<MemMap> table_mem_map_;
+  // Mem map where we store the extended debugging info.
+  UniquePtr<MemMap> slot_mem_map_;
   /* bottom of the stack */
   mirror::Object** table_;
   /* bit mask, ORed into all irefs */
diff --git a/runtime/indirect_reference_table_test.cc b/runtime/indirect_reference_table_test.cc
index 9b42e59..449817a 100644
--- a/runtime/indirect_reference_table_test.cc
+++ b/runtime/indirect_reference_table_test.cc
@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-#include "indirect_reference_table.h"
+#include "indirect_reference_table-inl.h"
 
 #include "common_runtime_test.h"
 #include "mirror/object-inl.h"
diff --git a/runtime/jni_internal.cc b/runtime/jni_internal.cc
index e6a35d0..915f2c9 100644
--- a/runtime/jni_internal.cc
+++ b/runtime/jni_internal.cc
@@ -29,6 +29,7 @@
 #include "class_linker-inl.h"
 #include "dex_file-inl.h"
 #include "gc/accounting/card_table-inl.h"
+#include "indirect_reference_table-inl.h"
 #include "interpreter/interpreter.h"
 #include "jni.h"
 #include "mirror/art_field-inl.h"
diff --git a/runtime/jni_internal.h b/runtime/jni_internal.h
index ec911b2..cdf3c47 100644
--- a/runtime/jni_internal.h
+++ b/runtime/jni_internal.h
@@ -116,7 +116,8 @@ class JavaVMExt : public JavaVM {
 
   // JNI global references.
   ReaderWriterMutex globals_lock DEFAULT_MUTEX_ACQUIRED_AFTER;
-  IndirectReferenceTable globals GUARDED_BY(globals_lock);
+  // Not guarded by globals_lock since we sometimes use SynchronizedGet in Thread::DecodeJObject.
+  IndirectReferenceTable globals;
 
   Mutex libraries_lock DEFAULT_MUTEX_ACQUIRED_AFTER;
   Libraries* libraries GUARDED_BY(libraries_lock);
diff --git a/runtime/jni_internal_test.cc b/runtime/jni_internal_test.cc
index 14fc25c..778b9e5 100644
--- a/runtime/jni_internal_test.cc
+++ b/runtime/jni_internal_test.cc
@@ -987,9 +987,6 @@ TEST_F(JniInternalTest, PushLocalFrame_PopLocalFrame) {
     // Our local reference for the survivor is invalid because the survivor
     // gets a new local reference...
     EXPECT_EQ(JNIInvalidRefType, env_->GetObjectRefType(inner2));
-    // ...but the survivor should be in the local reference table.
-    JNIEnvExt* env = reinterpret_cast<JNIEnvExt*>(env_);
-    EXPECT_TRUE(env->locals.ContainsDirectPointer(inner2_direct_pointer));
 
     env_->PopLocalFrame(NULL);
   }
diff --git a/runtime/thread.cc b/runtime/thread.cc
index 23a6779..a35c001 100644
--- a/runtime/thread.cc
+++ b/runtime/thread.cc
@@ -44,6 +44,7 @@
 #include "gc/accounting/card_table-inl.h"
 #include "gc/heap.h"
 #include "gc/space/space.h"
+#include "indirect_reference_table-inl.h"
 #include "jni_internal.h"
 #include "mirror/art_field-inl.h"
 #include "mirror/art_method-inl.h"
@@ -1254,10 +1255,8 @@ mirror::Object* Thread::DecodeJObject(jobject obj) const {
       result = kInvalidIndirectRefObject;
     }
   } else if (kind == kGlobal) {
-    JavaVMExt* vm = Runtime::Current()->GetJavaVM();
-    IndirectReferenceTable& globals = vm->globals;
-    ReaderMutexLock mu(const_cast<Thread*>(this), vm->globals_lock);
-    result = const_cast<mirror::Object*>(globals.Get(ref));
+    JavaVMExt* const vm = Runtime::Current()->GetJavaVM();
+    result = vm->globals.SynchronizedGet(const_cast<Thread*>(this), &vm->globals_lock, ref);
   } else {
     DCHECK_EQ(kind, kWeakGlobal);
     result = Runtime::Current()->GetJavaVM()->DecodeWeakGlobal(const_cast<Thread*>(this), ref);