Cleanup transaction support

Updates Thread::CreateInternalStackTrace to support both transactional and
non-transactional modes using template.

Generalizes non-transactional mode for invariant fields (which are set only
once).

Removes ArrayLog::VisitRoots as we never create Array logs of ObjectArray. As
ObjectArray elements are set using Object::SetFieldObject, they are already
recorded in the object logs: the object is the array itself and the offset
corresponds to the element index in this array. And also checks we never log
ObjectArray in array logs.

Fixes location of thrown exception when calling native method during class
initialization.

Change-Id: Idbc368d3b8292b85ff40bc8a7c559e085477bf89

16 files changed, 57 insertions, 81 deletions

diff --git a/compiler/jni/jni_compiler_test.cc b/compiler/jni/jni_compiler_test.cc
index 31acb69..3204282 100644
--- a/compiler/jni/jni_compiler_test.cc
+++ b/compiler/jni/jni_compiler_test.cc
@@ -590,7 +590,7 @@ jint Java_MyClassNatives_nativeUpCall(JNIEnv* env, jobject thisObj, jint i) {
     ScopedObjectAccess soa(env);
 
     // Build stack trace
-    jobject internal = Thread::Current()->CreateInternalStackTrace(soa);
+    jobject internal = Thread::Current()->CreateInternalStackTrace<false>(soa);
     jobjectArray ste_array = Thread::InternalStackTraceToStackTraceElementArray(soa, internal);
     mirror::ObjectArray<mirror::StackTraceElement>* trace_array =
         soa.Decode<mirror::ObjectArray<mirror::StackTraceElement>*>(ste_array);
diff --git a/runtime/exception_test.cc b/runtime/exception_test.cc
index 9c76a14..208eb74 100644
--- a/runtime/exception_test.cc
+++ b/runtime/exception_test.cc
@@ -199,7 +199,7 @@ TEST_F(ExceptionTest, StackTraceElement) {
     thread->PushShadowFrame(reinterpret_cast<ShadowFrame*>(&fake_stack[0]));
   }
 
-  jobject internal = thread->CreateInternalStackTrace(soa);
+  jobject internal = thread->CreateInternalStackTrace<false>(soa);
   ASSERT_TRUE(internal != NULL);
   jobjectArray ste_array = Thread::InternalStackTraceToStackTraceElementArray(soa, internal);
   ASSERT_TRUE(ste_array != NULL);
diff --git a/runtime/interpreter/interpreter.cc b/runtime/interpreter/interpreter.cc
index abe7fe1..1bf0078 100644
--- a/runtime/interpreter/interpreter.cc
+++ b/runtime/interpreter/interpreter.cc
@@ -68,7 +68,7 @@ static void UnstartedRuntimeJni(Thread* self, ArtMethod* method,
     result->SetL(Array::CreateMultiArray(self, sirt_class, sirt_dimensions));
   } else if (name == "java.lang.Object java.lang.Throwable.nativeFillInStackTrace()") {
     ScopedObjectAccessUnchecked soa(self);
-    result->SetL(soa.Decode<Object*>(self->CreateInternalStackTrace(soa)));
+    result->SetL(soa.Decode<Object*>(self->CreateInternalStackTrace<true>(soa)));
   } else if (name == "int java.lang.System.identityHashCode(java.lang.Object)") {
     mirror::Object* obj = reinterpret_cast<Object*>(args[0]);
     result->SetI((obj != nullptr) ? obj->IdentityHashCode() : 0);
@@ -96,7 +96,7 @@ static void UnstartedRuntimeJni(Thread* self, ArtMethod* method,
     result->SetI(Primitive::ComponentSize(primitive_type));
   } else {
     // Throw an exception so we can abort the transaction and undo every change.
-    ThrowLocation throw_location;
+    ThrowLocation throw_location = self->GetCurrentLocationForThrow();
     self->ThrowNewExceptionF(throw_location, "Ljava/lang/InternalError;",
                              "Attempt to invoke native method in non-started runtime: %s",
                              name.c_str());
diff --git a/runtime/mirror/class.h b/runtime/mirror/class.h
index ddc07ff..d955b97 100644
--- a/runtime/mirror/class.h
+++ b/runtime/mirror/class.h
@@ -391,11 +391,8 @@ class MANAGED Class : public Object {
   void SetComponentType(Class* new_component_type) SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
     DCHECK(GetComponentType() == NULL);
     DCHECK(new_component_type != NULL);
-    if (Runtime::Current()->IsActiveTransaction()) {
-      SetFieldObject<true>(ComponentTypeOffset(), new_component_type, false);
-    } else {
-      SetFieldObject<false>(ComponentTypeOffset(), new_component_type, false);
-    }
+    // Component type is invariant: use non-transactional mode without check.
+    SetFieldObject<false, false>(ComponentTypeOffset(), new_component_type, false);
   }
 
   size_t GetComponentSize() SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
diff --git a/runtime/mirror/object_test.cc b/runtime/mirror/object_test.cc
index 7d8da14..32f30c3 100644
--- a/runtime/mirror/object_test.cc
+++ b/runtime/mirror/object_test.cc
@@ -425,8 +425,8 @@ TEST_F(ObjectTest, StringLength) {
   EXPECT_EQ(string->GetLength(), 7);
   EXPECT_EQ(string->GetUtfLength(), 7);
 
-  string->SetOffset<false>(2);
-  string->SetCount<false>(5);
+  string->SetOffset(2);
+  string->SetCount(5);
   EXPECT_TRUE(string->Equals("droid"));
   EXPECT_EQ(string->GetLength(), 5);
   EXPECT_EQ(string->GetUtfLength(), 5);
diff --git a/runtime/mirror/string.cc b/runtime/mirror/string.cc
index d4f11b2..88a8e6f 100644
--- a/runtime/mirror/string.cc
+++ b/runtime/mirror/string.cc
@@ -59,10 +59,11 @@ int32_t String::FastIndexOf(int32_t ch, int32_t start) {
   return -1;
 }
 
-template<bool kTransactionActive>
 void String::SetArray(CharArray* new_array) {
+  // Array is invariant so use non-transactional mode. Also disable check as we may run inside
+  // a transaction.
   DCHECK(new_array != NULL);
-  SetFieldObject<kTransactionActive>(OFFSET_OF_OBJECT_MEMBER(String, array_), new_array, false);
+  SetFieldObject<false, false>(OFFSET_OF_OBJECT_MEMBER(String, array_), new_array, false);
 }
 
 // TODO: get global references for these
@@ -168,13 +169,8 @@ String* String::Alloc(Thread* self, const SirtRef<CharArray>& array) {
   // Hold reference in case AllocObject causes GC.
   String* string = down_cast<String*>(GetJavaLangString()->AllocObject(self));
   if (LIKELY(string != nullptr)) {
-    if (Runtime::Current()->IsActiveTransaction()) {
-      string->SetArray<true>(array.get());
-      string->SetCount<true>(array->GetLength());
-    } else {
-      string->SetArray<false>(array.get());
-      string->SetCount<false>(array->GetLength());
-    }
+    string->SetArray(array.get());
+    string->SetCount(array->GetLength());
   }
   return string;
 }
diff --git a/runtime/mirror/string.h b/runtime/mirror/string.h
index 1340e7d..de9e4c4 100644
--- a/runtime/mirror/string.h
+++ b/runtime/mirror/string.h
@@ -118,17 +118,18 @@ class MANAGED String : public Object {
     SetField32<false, false>(OFFSET_OF_OBJECT_MEMBER(String, hash_code_), new_hash_code, false);
   }
 
-  template<bool kTransactionActive>
   void SetCount(int32_t new_count) {
+    // Count is invariant so use non-transactional mode. Also disable check as we may run inside
+    // a transaction.
     DCHECK_LE(0, new_count);
-    SetField32<kTransactionActive>(OFFSET_OF_OBJECT_MEMBER(String, count_), new_count, false);
+    SetField32<false, false>(OFFSET_OF_OBJECT_MEMBER(String, count_), new_count, false);
   }
 
-  template<bool kTransactionActive>
   void SetOffset(int32_t new_offset) SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
+    // Offset is only used during testing so use non-transactional mode.
     DCHECK_LE(0, new_offset);
     DCHECK_GE(GetLength(), new_offset);
-    SetField32<kTransactionActive>(OFFSET_OF_OBJECT_MEMBER(String, offset_), new_offset, false);
+    SetField32<false>(OFFSET_OF_OBJECT_MEMBER(String, offset_), new_offset, false);
   }
 
   static String* Alloc(Thread* self, int32_t utf16_length)
@@ -137,7 +138,6 @@ class MANAGED String : public Object {
   static String* Alloc(Thread* self, const SirtRef<CharArray>& array)
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
 
-  template<bool kTransactionActive>
   void SetArray(CharArray* new_array) SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
 
   // Field order required by test "ValidateFieldOrderOfJavaCppUnionClasses".
diff --git a/runtime/native/dalvik_system_VMStack.cc b/runtime/native/dalvik_system_VMStack.cc
index 9975bf7..cf31064 100644
--- a/runtime/native/dalvik_system_VMStack.cc
+++ b/runtime/native/dalvik_system_VMStack.cc
@@ -30,7 +30,7 @@ static jobject GetThreadStack(const ScopedFastNativeObjectAccess& soa, jobject p
     SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
   jobject trace = nullptr;
   if (soa.Decode<mirror::Object*>(peer) == soa.Self()->GetPeer()) {
-    trace = soa.Self()->CreateInternalStackTrace(soa);
+    trace = soa.Self()->CreateInternalStackTrace<false>(soa);
   } else {
     // Suspend thread to build stack trace.
     soa.Self()->TransitionFromRunnableToSuspended(kNative);
@@ -39,7 +39,7 @@ static jobject GetThreadStack(const ScopedFastNativeObjectAccess& soa, jobject p
     if (thread != nullptr) {
       // Must be runnable to create returned array.
       CHECK_EQ(soa.Self()->TransitionFromSuspendedToRunnable(), kNative);
-      trace = thread->CreateInternalStackTrace(soa);
+      trace = thread->CreateInternalStackTrace<false>(soa);
       soa.Self()->TransitionFromRunnableToSuspended(kNative);
       // Restart suspended thread.
       Runtime::Current()->GetThreadList()->Resume(thread, false);
diff --git a/runtime/native/java_lang_Throwable.cc b/runtime/native/java_lang_Throwable.cc
index d1a1105..3ed4cfe 100644
--- a/runtime/native/java_lang_Throwable.cc
+++ b/runtime/native/java_lang_Throwable.cc
@@ -22,7 +22,7 @@ namespace art {
 
 static jobject Throwable_nativeFillInStackTrace(JNIEnv* env, jclass) {
   ScopedFastNativeObjectAccess soa(env);
-  return soa.Self()->CreateInternalStackTrace(soa);
+  return soa.Self()->CreateInternalStackTrace<false>(soa);
 }
 
 static jobjectArray Throwable_nativeGetStackTrace(JNIEnv* env, jclass, jobject javaStackState) {
diff --git a/runtime/native/org_apache_harmony_dalvik_ddmc_DdmVmInternal.cc b/runtime/native/org_apache_harmony_dalvik_ddmc_DdmVmInternal.cc
index a7ca0b8..5d90f1a 100644
--- a/runtime/native/org_apache_harmony_dalvik_ddmc_DdmVmInternal.cc
+++ b/runtime/native/org_apache_harmony_dalvik_ddmc_DdmVmInternal.cc
@@ -49,7 +49,7 @@ static jobjectArray DdmVmInternal_getStackTraceById(JNIEnv* env, jclass, jint th
   if (static_cast<uint32_t>(thin_lock_id) == self->GetThreadId()) {
     // No need to suspend ourself to build stacktrace.
     ScopedObjectAccess soa(env);
-    jobject internal_trace = self->CreateInternalStackTrace(soa);
+    jobject internal_trace = self->CreateInternalStackTrace<false>(soa);
     trace = Thread::InternalStackTraceToStackTraceElementArray(soa, internal_trace);
   } else {
     // Suspend thread to build stack trace.
@@ -59,7 +59,7 @@ static jobjectArray DdmVmInternal_getStackTraceById(JNIEnv* env, jclass, jint th
     if (thread != nullptr) {
       {
         ScopedObjectAccess soa(env);
-        jobject internal_trace = thread->CreateInternalStackTrace(soa);
+        jobject internal_trace = thread->CreateInternalStackTrace<false>(soa);
         trace = Thread::InternalStackTraceToStackTraceElementArray(soa, internal_trace);
       }
       // Restart suspended thread.
diff --git a/runtime/runtime.cc b/runtime/runtime.cc
index a8da2f8..c4010e3 100644
--- a/runtime/runtime.cc
+++ b/runtime/runtime.cc
@@ -124,7 +124,7 @@ Runtime::Runtime()
       system_thread_group_(nullptr),
       system_class_loader_(nullptr),
       dump_gc_performance_on_shutdown_(false),
-      preinitialization_transaction(nullptr),
+      preinitialization_transaction_(nullptr),
       null_pointer_handler_(nullptr),
       suspend_handler_(nullptr),
       stack_overflow_handler_(nullptr) {
@@ -894,8 +894,8 @@ void Runtime::VisitNonThreadRoots(RootCallback* callback, void* arg) {
       verifier->VisitRoots(callback, arg);
     }
   }
-  if (preinitialization_transaction != nullptr) {
-    preinitialization_transaction->VisitRoots(callback, arg);
+  if (preinitialization_transaction_ != nullptr) {
+    preinitialization_transaction_->VisitRoots(callback, arg);
   }
   instrumentation_.VisitRoots(callback, arg);
 }
@@ -1141,73 +1141,68 @@ void Runtime::StartProfiler(const char* appDir, const char* procName, bool start
 }
 
 // Transaction support.
-// TODO move them to header file for inlining.
-bool Runtime::IsActiveTransaction() const {
-  return preinitialization_transaction != nullptr;
-}
-
 void Runtime::EnterTransactionMode(Transaction* transaction) {
   DCHECK(IsCompiler());
   DCHECK(transaction != nullptr);
   DCHECK(!IsActiveTransaction());
-  preinitialization_transaction = transaction;
+  preinitialization_transaction_ = transaction;
 }
 
 void Runtime::ExitTransactionMode() {
   DCHECK(IsCompiler());
   DCHECK(IsActiveTransaction());
-  preinitialization_transaction = nullptr;
+  preinitialization_transaction_ = nullptr;
 }
 
 void Runtime::RecordWriteField32(mirror::Object* obj, MemberOffset field_offset,
                                  uint32_t value, bool is_volatile) const {
   DCHECK(IsCompiler());
   DCHECK(IsActiveTransaction());
-  preinitialization_transaction->RecordWriteField32(obj, field_offset, value, is_volatile);
+  preinitialization_transaction_->RecordWriteField32(obj, field_offset, value, is_volatile);
 }
 
 void Runtime::RecordWriteField64(mirror::Object* obj, MemberOffset field_offset,
                                  uint64_t value, bool is_volatile) const {
   DCHECK(IsCompiler());
   DCHECK(IsActiveTransaction());
-  preinitialization_transaction->RecordWriteField64(obj, field_offset, value, is_volatile);
+  preinitialization_transaction_->RecordWriteField64(obj, field_offset, value, is_volatile);
 }
 
 void Runtime::RecordWriteFieldReference(mirror::Object* obj, MemberOffset field_offset,
                                         mirror::Object* value, bool is_volatile) const {
   DCHECK(IsCompiler());
   DCHECK(IsActiveTransaction());
-  preinitialization_transaction->RecordWriteFieldReference(obj, field_offset, value, is_volatile);
+  preinitialization_transaction_->RecordWriteFieldReference(obj, field_offset, value, is_volatile);
 }
 
 void Runtime::RecordWriteArray(mirror::Array* array, size_t index, uint64_t value) const {
   DCHECK(IsCompiler());
   DCHECK(IsActiveTransaction());
-  preinitialization_transaction->RecordWriteArray(array, index, value);
+  preinitialization_transaction_->RecordWriteArray(array, index, value);
 }
 
 void Runtime::RecordStrongStringInsertion(mirror::String* s, uint32_t hash_code) const {
   DCHECK(IsCompiler());
   DCHECK(IsActiveTransaction());
-  preinitialization_transaction->RecordStrongStringInsertion(s, hash_code);
+  preinitialization_transaction_->RecordStrongStringInsertion(s, hash_code);
 }
 
 void Runtime::RecordWeakStringInsertion(mirror::String* s, uint32_t hash_code) const {
   DCHECK(IsCompiler());
   DCHECK(IsActiveTransaction());
-  preinitialization_transaction->RecordWeakStringInsertion(s, hash_code);
+  preinitialization_transaction_->RecordWeakStringInsertion(s, hash_code);
 }
 
 void Runtime::RecordStrongStringRemoval(mirror::String* s, uint32_t hash_code) const {
   DCHECK(IsCompiler());
   DCHECK(IsActiveTransaction());
-  preinitialization_transaction->RecordStrongStringRemoval(s, hash_code);
+  preinitialization_transaction_->RecordStrongStringRemoval(s, hash_code);
 }
 
 void Runtime::RecordWeakStringRemoval(mirror::String* s, uint32_t hash_code) const {
   DCHECK(IsCompiler());
   DCHECK(IsActiveTransaction());
-  preinitialization_transaction->RecordWeakStringRemoval(s, hash_code);
+  preinitialization_transaction_->RecordWeakStringRemoval(s, hash_code);
 }
 
 void Runtime::SetFaultMessage(const std::string& message) {
diff --git a/runtime/runtime.h b/runtime/runtime.h
index 50c88d3..84c2c87 100644
--- a/runtime/runtime.h
+++ b/runtime/runtime.h
@@ -378,7 +378,9 @@ class Runtime {
   void UpdateProfilerState(int state);
 
   // Transaction support.
-  bool IsActiveTransaction() const;
+  bool IsActiveTransaction() const {
+    return preinitialization_transaction_ != nullptr;
+  }
   void EnterTransactionMode(Transaction* transaction);
   void ExitTransactionMode();
   void RecordWriteField32(mirror::Object* obj, MemberOffset field_offset, uint32_t value,
@@ -544,7 +546,7 @@ class Runtime {
   bool dump_gc_performance_on_shutdown_;
 
   // Transaction used for pre-initializing classes at compilation time.
-  Transaction* preinitialization_transaction;
+  Transaction* preinitialization_transaction_;
   NullPointerHandler* null_pointer_handler_;
   SuspensionHandler* suspend_handler_;
   StackOverflowHandler* stack_overflow_handler_;
diff --git a/runtime/thread.cc b/runtime/thread.cc
index 38e4204..d170e72 100644
--- a/runtime/thread.cc
+++ b/runtime/thread.cc
@@ -1305,6 +1305,7 @@ class CountStackDepthVisitor : public StackVisitor {
   bool skipping_;
 };
 
+template<bool kTransactionActive>
 class BuildInternalStackTraceVisitor : public StackVisitor {
  public:
   explicit BuildInternalStackTraceVisitor(Thread* self, Thread* thread, int skip_depth)
@@ -1328,7 +1329,7 @@ class BuildInternalStackTraceVisitor : public StackVisitor {
     // Save PC trace in last element of method trace, also places it into the
     // object graph.
     // We are called from native: use non-transactional mode.
-    method_trace->Set<false>(depth, dex_pc_trace);
+    method_trace->Set<kTransactionActive>(depth, dex_pc_trace);
     // Set the Object*s and assert that no thread suspension is now possible.
     const char* last_no_suspend_cause =
         self_->StartAssertNoThreadSuspension("Building internal stack trace");
@@ -1356,14 +1357,8 @@ class BuildInternalStackTraceVisitor : public StackVisitor {
     if (m->IsRuntimeMethod()) {
       return true;  // Ignore runtime frames (in particular callee save).
     }
-    // TODO dedup this code.
-    if (Runtime::Current()->IsActiveTransaction()) {
-      method_trace_->Set<true>(count_, m);
-      dex_pc_trace_->Set<true>(count_, m->IsProxyMethod() ? DexFile::kDexNoIndex : GetDexPc());
-    } else {
-      method_trace_->Set<false>(count_, m);
-      dex_pc_trace_->Set<false>(count_, m->IsProxyMethod() ? DexFile::kDexNoIndex : GetDexPc());
-    }
+    method_trace_->Set<kTransactionActive>(count_, m);
+    dex_pc_trace_->Set<kTransactionActive>(count_, m->IsProxyMethod() ? DexFile::kDexNoIndex : GetDexPc());
     ++count_;
     return true;
   }
@@ -1384,6 +1379,7 @@ class BuildInternalStackTraceVisitor : public StackVisitor {
   mirror::ObjectArray<mirror::Object>* method_trace_;
 };
 
+template<bool kTransactionActive>
 jobject Thread::CreateInternalStackTrace(const ScopedObjectAccessUnchecked& soa) const {
   // Compute depth of stack
   CountStackDepthVisitor count_visitor(const_cast<Thread*>(this));
@@ -1392,8 +1388,9 @@ jobject Thread::CreateInternalStackTrace(const ScopedObjectAccessUnchecked& soa)
   int32_t skip_depth = count_visitor.GetSkipDepth();
 
   // Build internal stack trace.
-  BuildInternalStackTraceVisitor build_trace_visitor(soa.Self(), const_cast<Thread*>(this),
-                                                     skip_depth);
+  BuildInternalStackTraceVisitor<kTransactionActive> build_trace_visitor(soa.Self(),
+                                                                         const_cast<Thread*>(this),
+                                                                         skip_depth);
   if (!build_trace_visitor.Init(depth)) {
     return nullptr;  // Allocation failed.
   }
@@ -1406,6 +1403,8 @@ jobject Thread::CreateInternalStackTrace(const ScopedObjectAccessUnchecked& soa)
   }
   return soa.AddLocalReference<jobjectArray>(trace);
 }
+template jobject Thread::CreateInternalStackTrace<false>(const ScopedObjectAccessUnchecked& soa) const;
+template jobject Thread::CreateInternalStackTrace<true>(const ScopedObjectAccessUnchecked& soa) const;
 
 jobjectArray Thread::InternalStackTraceToStackTraceElementArray(const ScopedObjectAccess& soa,
     jobject internal, jobjectArray output_array, int* stack_depth) {
diff --git a/runtime/thread.h b/runtime/thread.h
index b063b1e..91f91ec 100644
--- a/runtime/thread.h
+++ b/runtime/thread.h
@@ -396,7 +396,8 @@ class PACKED(4) Thread {
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
 
   // Create the internal representation of a stack trace, that is more time
-  // and space efficient to compute than the StackTraceElement[]
+  // and space efficient to compute than the StackTraceElement[].
+  template<bool kTransactionActive>
   jobject CreateInternalStackTrace(const ScopedObjectAccessUnchecked& soa) const
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
 
diff --git a/runtime/transaction.cc b/runtime/transaction.cc
index fcda6c9..e18cf04 100644
--- a/runtime/transaction.cc
+++ b/runtime/transaction.cc
@@ -84,19 +84,18 @@ void Transaction::RecordWriteFieldReference(mirror::Object* obj, MemberOffset fi
 void Transaction::RecordWriteArray(mirror::Array* array, size_t index, uint64_t value) {
   DCHECK(array != nullptr);
   DCHECK(array->IsArrayInstance());
+  DCHECK(!array->IsObjectArray());
   MutexLock mu(Thread::Current(), log_lock_);
   ArrayLog& array_log = array_logs_[array];
   array_log.LogValue(index, value);
 }
 
 void Transaction::RecordStrongStringInsertion(mirror::String* s, uint32_t hash_code) {
-  DCHECK(s != nullptr);
   InternStringLog log(s, hash_code, InternStringLog::kStrongString, InternStringLog::kInsert);
   LogInternedString(log);
 }
 
 void Transaction::RecordWeakStringInsertion(mirror::String* s, uint32_t hash_code) {
-  DCHECK(s != nullptr);
   InternStringLog log(s, hash_code, InternStringLog::kWeakString, InternStringLog::kInsert);
   LogInternedString(log);
 }
@@ -198,9 +197,7 @@ void Transaction::VisitArrayLogs(RootCallback* callback, void* arg) {
 
   for (auto it : array_logs_) {
     mirror::Array* old_root = it.first;
-    if (old_root->IsObjectArray()) {
-      it.second.VisitRoots(callback, arg);
-    }
+    CHECK(!old_root->IsObjectArray());
     mirror::Array* new_root = old_root;
     callback(reinterpret_cast<mirror::Object**>(&new_root), arg, 0, kRootUnknown);
     if (new_root != old_root) {
@@ -403,23 +400,12 @@ void Transaction::ArrayLog::UndoArrayWrite(mirror::Array* array, Primitive::Type
     case Primitive::kPrimDouble:
       array->AsDoubleArray()->SetWithoutChecks<false>(index, static_cast<double>(value));
       break;
-    case Primitive::kPrimNot: {
-      mirror::ObjectArray<mirror::Object>* obj_array = array->AsObjectArray<mirror::Object>();
-      obj_array->SetWithoutChecks<false>(index, reinterpret_cast<mirror::Object*>(
-          static_cast<uintptr_t>(value)));
+    case Primitive::kPrimNot:
+      LOG(FATAL) << "ObjectArray should be treated as Object";
       break;
-    }
     default:
       LOG(FATAL) << "Unsupported type " << array_type;
   }
 }
 
-void Transaction::ArrayLog::VisitRoots(RootCallback* callback, void* arg) {
-  for (auto& it : array_values_) {
-    mirror::Object* obj = reinterpret_cast<mirror::Object*>(static_cast<uintptr_t>(it.second));
-    callback(&obj, arg, 0, kRootUnknown);
-    it.second = reinterpret_cast<uintptr_t>(obj);
-  }
-}
-
 }  // namespace art
diff --git a/runtime/transaction.h b/runtime/transaction.h
index cf696de..6fd86c8 100644
--- a/runtime/transaction.h
+++ b/runtime/transaction.h
@@ -118,7 +118,6 @@ class Transaction {
     void LogValue(size_t index, uint64_t value);
 
     void Undo(mirror::Array* obj) SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
-    void VisitRoots(RootCallback* callback, void* arg);
 
     size_t Size() const {
       return array_values_.size();
@@ -145,6 +144,7 @@ class Transaction {
     };
     InternStringLog(mirror::String* s, uint32_t hash_code, StringKind kind, StringOp op)
       : str_(s), hash_code_(hash_code), string_kind_(kind), string_op_(op) {
+      DCHECK(s != nullptr);
     }
 
     void Undo(InternTable* intern_table) EXCLUSIVE_LOCKS_REQUIRED(Locks::intern_table_lock_);