From 4345c46b8a927cf13d9bbe38f8cf0593f5de181b Mon Sep 17 00:00:00 2001 From: Mathieu Chartier Date: Fri, 27 Jun 2014 10:20:14 -0700 Subject: [PATCH] Fix local reference leaks in debugger and use a cache. Changed alloc record stack trace element to use jmethodID instead of JNI weak global references. Added code to delete the local ref created in AllocRecord::SetType. Bug: 15886342 External bug: https://code.google.com/p/android/issues/detail?id=72330 Change-Id: Id18e765820baad02246768dc9d633aada60f4fed --- runtime/debugger.cc | 65 ++++++++++++++++++++++++++++++++++------------------- runtime/debugger.h | 19 ++++++++++++++++ 2 files changed, 61 insertions(+), 23 deletions(-) diff --git a/runtime/debugger.cc b/runtime/debugger.cc index f19c353f1..11415b49e 100644 --- a/runtime/debugger.cc +++ b/runtime/debugger.cc @@ -74,18 +74,13 @@ class AllocRecordStackTraceElement { } mirror::ArtMethod* Method() SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) { - mirror::ArtMethod* method = reinterpret_cast( - Thread::Current()->DecodeJObject(method_)); - return method; + ScopedObjectAccessUnchecked soa(Thread::Current()); + return soa.DecodeMethod(method_); } void SetMethod(mirror::ArtMethod* m) SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) { ScopedObjectAccessUnchecked soa(Thread::Current()); - JNIEnv* env = soa.Env(); - if (method_ != nullptr) { - env->DeleteWeakGlobalRef(method_); - } - method_ = env->NewWeakGlobalRef(soa.AddLocalReference(m)); + method_ = soa.EncodeMethod(m); } uint32_t DexPc() const { @@ -97,27 +92,46 @@ class AllocRecordStackTraceElement { } private: - jobject method_; // This is a weak global. + jmethodID method_; uint32_t dex_pc_; }; +jobject Dbg::TypeCache::Add(mirror::Class* t) { + ScopedObjectAccessUnchecked soa(Thread::Current()); + int32_t hash_code = t->IdentityHashCode(); + auto range = objects_.equal_range(hash_code); + for (auto it = range.first; it != range.second; ++it) { + if (soa.Decode(it->second) == t) { + // Found a matching weak global, return it. + return it->second; + } + } + JNIEnv* env = soa.Env(); + const jobject local_ref = soa.AddLocalReference(t); + const jobject weak_global = env->NewWeakGlobalRef(local_ref); + env->DeleteLocalRef(local_ref); + objects_.insert(std::make_pair(hash_code, weak_global)); + return weak_global; +} + +void Dbg::TypeCache::Clear() { + ScopedObjectAccess soa(Thread::Current()); + for (const auto& p : objects_) { + soa.Vm()->DeleteWeakGlobalRef(soa.Self(), p.second); + } + objects_.clear(); +} + class AllocRecord { public: AllocRecord() : type_(nullptr), byte_count_(0), thin_lock_id_(0) {} mirror::Class* Type() SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) { - mirror::Class* type = reinterpret_cast( - Thread::Current()->DecodeJObject(type_)); - return type; + return down_cast(Thread::Current()->DecodeJObject(type_)); } void SetType(mirror::Class* t) SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) { - ScopedObjectAccessUnchecked soa(Thread::Current()); - JNIEnv* env = soa.Env(); - if (type_ != nullptr) { - env->DeleteWeakGlobalRef(type_); - } - type_ = env->NewWeakGlobalRef(soa.AddLocalReference(t)); + type_ = Dbg::GetTypeCache().Add(t); } size_t GetDepth() SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) { @@ -274,6 +288,7 @@ AllocRecord* Dbg::recent_allocation_records_ = nullptr; // TODO: CircularBuffer size_t Dbg::alloc_record_max_ = 0; size_t Dbg::alloc_record_head_ = 0; size_t Dbg::alloc_record_count_ = 0; +Dbg::TypeCache Dbg::type_cache_; // Deoptimization support. Mutex* Dbg::deoptimization_lock_ = nullptr; @@ -4253,8 +4268,10 @@ void Dbg::SetAllocTrackingEnabled(bool enabled) { Runtime::Current()->GetInstrumentation()->UninstrumentQuickAllocEntryPoints(); { MutexLock mu(Thread::Current(), *alloc_tracker_lock_); + LOG(INFO) << "Disabling alloc tracker"; delete[] recent_allocation_records_; recent_allocation_records_ = NULL; + type_cache_.Clear(); } } } @@ -4376,8 +4393,12 @@ class StringTable { StringTable() { } - void Add(const char* s) { - table_.insert(s); + void Add(const std::string& str) { + table_.insert(str); + } + + void Add(const char* str) { + table_.insert(str); } size_t IndexOf(const char* s) const { @@ -4476,9 +4497,7 @@ jbyteArray Dbg::GetRecentAllocations() { int idx = HeadIndex(); while (count--) { AllocRecord* record = &recent_allocation_records_[idx]; - - class_names.Add(record->Type()->GetDescriptor().c_str()); - + class_names.Add(record->Type()->GetDescriptor()); for (size_t i = 0; i < kMaxAllocRecordStackDepth; i++) { mirror::ArtMethod* m = record->StackElement(i)->Method(); if (m != NULL) { diff --git a/runtime/debugger.h b/runtime/debugger.h index 1cf0b0c42..258963846 100644 --- a/runtime/debugger.h +++ b/runtime/debugger.h @@ -23,6 +23,7 @@ #include +#include #include #include #include @@ -160,6 +161,17 @@ struct DeoptimizationRequest { class Dbg { public: + class TypeCache { + public: + // Returns a weak global for the input type. Deduplicates. + jobject Add(mirror::Class* t) SHARED_LOCKS_REQUIRED(Locks::mutator_lock_); + // Clears the type cache and deletes all the weak global refs. + void Clear(); + + private: + std::multimap objects_; + }; + static bool ParseJdwpOptions(const std::string& options); static void SetJdwpAllowed(bool allowed); @@ -555,6 +567,10 @@ class Dbg { static void DdmSendHeapSegments(bool native) SHARED_LOCKS_REQUIRED(Locks::mutator_lock_); + static TypeCache& GetTypeCache() { + return type_cache_; + } + private: static void DdmBroadcast(bool connect) SHARED_LOCKS_REQUIRED(Locks::mutator_lock_); static void PostThreadStartOrStop(Thread*, uint32_t) @@ -604,6 +620,9 @@ class Dbg { static size_t* GetReferenceCounterForEvent(uint32_t instrumentation_event); + // Weak global type cache, TODO improve this. + static TypeCache type_cache_; + // Instrumentation event reference counters. // TODO we could use an array instead of having all these dedicated counters. Instrumentation // events are bits of a mask so we could convert them to array index. -- 2.11.0