Merge "Cleanup transaction support"

15 files changed, 56 insertions, 80 deletions

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 c0bb36a..23405e6 100644
--- a/runtime/runtime.cc
+++ b/runtime/runtime.cc
@@ -127,7 +127,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) {
@@ -900,8 +900,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);
 }
@@ -1147,73 +1147,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 755c0f0..ed60d4d 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,
@@ -550,7 +552,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 c2fad44..b802882 100644
--- a/runtime/thread.cc
+++ b/runtime/thread.cc
@@ -1346,6 +1346,7 @@ class CountStackDepthVisitor : public StackVisitor {
   bool skipping_;
 };
 
+template<bool kTransactionActive>
 class BuildInternalStackTraceVisitor : public StackVisitor {
  public:
   explicit BuildInternalStackTraceVisitor(Thread* self, Thread* thread, int skip_depth)
@@ -1369,7 +1370,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");
@@ -1397,14 +1398,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;
   }
@@ -1425,6 +1420,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));
@@ -1433,8 +1429,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.
   }
@@ -1447,6 +1444,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 f1c1d77..d25bbe9 100644
--- a/runtime/thread.h
+++ b/runtime/thread.h
@@ -450,7 +450,8 @@ class 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_);