Removed MallocHookScope and replaced it with MallocLocker, which behaves like MutexLocker but also …

runtime/vm/malloc_hooks.cc

 // Copyright (c) 2017, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "platform/globals.h"
 
 #if defined(DART_USE_TCMALLOC) && !defined(PRODUCT)
 
 #include "vm/malloc_hooks.h"
 
 #include "gperftools/malloc_hook.h"
 
 #include "platform/assert.h"
 #include "vm/hash_map.h"
 #include "vm/json_stream.h"
-#include "vm/lockers.h"
+#include "vm/os_thread.h"
 
 namespace dart {
 
-// A locker-type class to automatically grab and release the
-// in_malloc_hook_flag_.
-class MallocHookScope {
+// A locker-type class similar to MutexLocker which tracks which thread
+// currently holds the lock. We use this instead of MutexLocker and
+// mutex->IsOwnedByCurrentThread() since IsOwnedByCurrentThread() is only
+// enabled for debug mode.
+class MallocLocker : public ValueObject {

[zra, 2017/02/22 22:54:54]

This seems generally useful. Should it go in os_thread.h with a more generic
name? OwnedLocker?

[bkonyi, 2017/02/22 23:04:26]

In its current state, I don't think that would work since owner_ is a static
variable. Mutex does have a owner_ field itself, but it's only defined in DEBUG,
which is part of the reason I created MallocLocker here. If we're okay adding a
ThreadId field to all mutexes, then we can just use MutexLocker and
Mutex::IsOwnedByCurrentThread.

[siva, 2017/02/22 23:53:04]

Discussed offline with Zach, For this CL we leave it the way it is here
and then in a new CL add a re-entrant lock class and use it here instead.

[bkonyi, 2017/02/23 00:28:46]

That makes sense to me.

  public:
-  static void InitMallocHookFlag() {
-    MutexLocker ml(malloc_hook_scope_mutex_);
-    ASSERT(in_malloc_hook_flag_ == kUnsetThreadLocalKey);
-    in_malloc_hook_flag_ = OSThread::CreateThreadLocal();
-    OSThread::SetThreadLocal(in_malloc_hook_flag_, 0);
+  explicit MallocLocker(Mutex* mutex) : mutex_(mutex) {
+    mutex_->Lock();
+    ASSERT(owner_ == OSThread::kInvalidThreadId);
+    owner_ = OSThread::GetCurrentThreadId();
   }
 
-  static void DestroyMallocHookFlag() {
-    MutexLocker ml(malloc_hook_scope_mutex_);
-    ASSERT(in_malloc_hook_flag_ != kUnsetThreadLocalKey);
-    OSThread::DeleteThreadLocal(in_malloc_hook_flag_);
-    in_malloc_hook_flag_ = kUnsetThreadLocalKey;
+  virtual ~MallocLocker() {
+    ASSERT(IsOwnedByCurrentThread());
+    owner_ = OSThread::kInvalidThreadId;
+    mutex_->Unlock();
   }
 
-  MallocHookScope() {
-    MutexLocker ml(malloc_hook_scope_mutex_);
-    ASSERT(in_malloc_hook_flag_ != kUnsetThreadLocalKey);
-    OSThread::SetThreadLocal(in_malloc_hook_flag_, 1);
-  }
-
-  ~MallocHookScope() {
-    MutexLocker ml(malloc_hook_scope_mutex_);
-    ASSERT(in_malloc_hook_flag_ != kUnsetThreadLocalKey);
-    OSThread::SetThreadLocal(in_malloc_hook_flag_, 0);
-  }
-
-  static bool IsInHook() {
-    MutexLocker ml(malloc_hook_scope_mutex_);
-    if (in_malloc_hook_flag_ == kUnsetThreadLocalKey) {
-      // Bail out if the malloc hook flag is invalid. This means that
-      // MallocHookState::TearDown() has been called and MallocHookScope is no
-      // longer intitialized. Don't worry if MallocHookState::TearDown() is
-      // called before the hooks grab the mutex, since
-      // MallocHooksState::Active() is checked after the lock is taken before
-      // proceeding to act on the allocation/free.
-      return false;
-    }
-    return OSThread::GetThreadLocal(in_malloc_hook_flag_);
+  static bool IsOwnedByCurrentThread() {
+    return (owner_ == OSThread::GetCurrentThreadId());
   }
 
  private:
-  static Mutex* malloc_hook_scope_mutex_;
-  static ThreadLocalKey in_malloc_hook_flag_;
+  Mutex* mutex_;
+  static ThreadId owner_;
 
-  DISALLOW_ALLOCATION();
-  DISALLOW_COPY_AND_ASSIGN(MallocHookScope);
+  DISALLOW_COPY_AND_ASSIGN(MallocLocker);

[zra, 2017/02/22 22:54:54]

Is this redundant if this is inheriting from ValueObject?

[bkonyi, 2017/02/22 23:04:26]

Looks like it is. I had based MallocLocker on MutexLocker which included the
DISALLOW_COPY_AND_ASSIGN(MutexLocker), so it's redundant there as well.

[bkonyi, 2017/02/23 00:28:46]

Done.

 };
 
 
 // Custom key/value trait specifically for address/size pairs. Unlike
 // RawPointerKeyValueTrait, the default value is -1 as 0 can be a valid entry.
 class AddressKeyValueTrait {
  public:
   typedef const void* Key;
   typedef intptr_t Value;
 
@@ skipping 100 matching lines @@
   static intptr_t original_pid_;
   static Mutex* malloc_hook_mutex_;
   static intptr_t allocation_count_;
   static intptr_t heap_allocated_memory_in_bytes_;
   static AddressMap* address_map_;
 
   static const intptr_t kInvalidPid = -1;
 };
 
 
-// MallocHookScope state.
-Mutex* MallocHookScope::malloc_hook_scope_mutex_ = new Mutex();
-ThreadLocalKey MallocHookScope::in_malloc_hook_flag_ = kUnsetThreadLocalKey;
+// MallocLocker state.
+ThreadId MallocLocker::owner_ = OSThread::kInvalidThreadId;

[siva, 2017/02/22 23:53:04]

Shouldn't this be clubbed just below the line :
Mutex* MallocHooksState::malloc_hook_mutex_ = new Mutex();

[bkonyi, 2017/02/23 00:28:46]

Done.

 
 // MallocHooks state / locks.
 bool MallocHooksState::active_ = false;
 intptr_t MallocHooksState::original_pid_ = MallocHooksState::kInvalidPid;
 Mutex* MallocHooksState::malloc_hook_mutex_ = new Mutex();
 
 // Memory allocation state information.
 intptr_t MallocHooksState::allocation_count_ = 0;
 intptr_t MallocHooksState::heap_allocated_memory_in_bytes_ = 0;
 AddressMap* MallocHooksState::address_map_ = NULL;
 
 
 void MallocHooks::InitOnce() {
   if (!FLAG_enable_malloc_hooks) {
     return;
   }
-  MutexLocker ml(MallocHooksState::malloc_hook_mutex());
+  MallocLocker ml(MallocHooksState::malloc_hook_mutex());
   ASSERT(!MallocHooksState::Active());
 
-  MallocHookScope::InitMallocHookFlag();
   MallocHooksState::Init();
 
   // Register malloc hooks.
   bool success = false;
   success = MallocHook::AddNewHook(&MallocHooksState::RecordAllocHook);
   ASSERT(success);
   success = MallocHook::AddDeleteHook(&MallocHooksState::RecordFreeHook);
   ASSERT(success);
 }
 
 
 void MallocHooks::TearDown() {
   if (!FLAG_enable_malloc_hooks) {
     return;
   }
-  MutexLocker ml(MallocHooksState::malloc_hook_mutex());
+  MallocLocker ml(MallocHooksState::malloc_hook_mutex());
   ASSERT(MallocHooksState::Active());
 
   // Remove malloc hooks.
   bool success = false;
   success = MallocHook::RemoveNewHook(&MallocHooksState::RecordAllocHook);
   ASSERT(success);
   success = MallocHook::RemoveDeleteHook(&MallocHooksState::RecordFreeHook);
   ASSERT(success);
 
   MallocHooksState::TearDown();
-  MallocHookScope::DestroyMallocHookFlag();
 }
 
 
 void MallocHooks::ResetStats() {
   if (!FLAG_enable_malloc_hooks) {
     return;
   }
-  MutexLocker ml(MallocHooksState::malloc_hook_mutex());
+  MallocLocker ml(MallocHooksState::malloc_hook_mutex());
   if (MallocHooksState::Active()) {
     MallocHooksState::ResetStats();
   }
 }
 
 
 bool MallocHooks::Active() {
   if (!FLAG_enable_malloc_hooks) {
     return false;
   }
   ASSERT(MallocHooksState::malloc_hook_mutex()->IsOwnedByCurrentThread());
   return MallocHooksState::Active();
 }
 
 
 void MallocHooks::PrintToJSONObject(JSONObject* jsobj) {
   if (!FLAG_enable_malloc_hooks) {
     return;
   }
   intptr_t allocated_memory = 0;
   intptr_t allocation_count = 0;
   bool add_usage = false;
   // AddProperty may call malloc which would result in an attempt
   // to acquire the lock recursively so we extract the values first
   // and then add the JSON properties.
   {
-    MutexLocker ml(MallocHooksState::malloc_hook_mutex());
+    MallocLocker ml(MallocHooksState::malloc_hook_mutex());
     if (Active()) {
       allocated_memory = MallocHooksState::heap_allocated_memory_in_bytes();
       allocation_count = MallocHooksState::allocation_count();
       add_usage = true;
     }
   }
   if (add_usage) {
     jsobj->AddProperty("_heapAllocatedMemoryUsage", allocated_memory);
     jsobj->AddProperty("_heapAllocationCount", allocation_count);
   }
 }
 
 
 intptr_t MallocHooks::allocation_count() {
   if (!FLAG_enable_malloc_hooks) {
     return 0;
   }
-  MutexLocker ml(MallocHooksState::malloc_hook_mutex());
+  MallocLocker ml(MallocHooksState::malloc_hook_mutex());
   return MallocHooksState::allocation_count();
 }
 
 
 intptr_t MallocHooks::heap_allocated_memory_in_bytes() {
   if (!FLAG_enable_malloc_hooks) {
     return 0;
   }
-  MutexLocker ml(MallocHooksState::malloc_hook_mutex());
+  MallocLocker ml(MallocHooksState::malloc_hook_mutex());
   return MallocHooksState::heap_allocated_memory_in_bytes();
 }
 
 
 void MallocHooksState::RecordAllocHook(const void* ptr, size_t size) {
-  if (MallocHookScope::IsInHook() || !MallocHooksState::IsOriginalProcess()) {
+  if (MallocLocker::IsOwnedByCurrentThread() ||
+      !MallocHooksState::IsOriginalProcess()) {
     return;
   }
 
-  MutexLocker ml(MallocHooksState::malloc_hook_mutex());
+  MallocLocker ml(MallocHooksState::malloc_hook_mutex());
   // Now that we hold the lock, check to make sure everything is still active.
   if ((ptr != NULL) && MallocHooksState::Active()) {
-    // Set the malloc hook flag to avoid calling hooks again if memory is
-    // allocated/freed below.
-    MallocHookScope mhs;
     MallocHooksState::IncrementHeapAllocatedMemoryInBytes(size);
     MallocHooksState::address_map()->Insert(ptr, size);
   }
 }
 
 
 void MallocHooksState::RecordFreeHook(const void* ptr) {
-  if (MallocHookScope::IsInHook() || !MallocHooksState::IsOriginalProcess()) {
+  if (MallocLocker::IsOwnedByCurrentThread() ||
+      !MallocHooksState::IsOriginalProcess()) {
     return;
   }
 
-  MutexLocker ml(MallocHooksState::malloc_hook_mutex());
+  MallocLocker ml(MallocHooksState::malloc_hook_mutex());
   // Now that we hold the lock, check to make sure everything is still active.
   if ((ptr != NULL) && MallocHooksState::Active()) {
-    // Set the malloc hook flag to avoid calling hooks again if memory is
-    // allocated/freed below.
-    MallocHookScope mhs;
     intptr_t size = 0;
     if (MallocHooksState::address_map()->Lookup(ptr, &size)) {
       MallocHooksState::DecrementHeapAllocatedMemoryInBytes(size);
       MallocHooksState::address_map()->Remove(ptr);
     }
   }
 }
 
 }  // namespace dart
 
 #endif  // defined(DART_USE_TCMALLOC) && !defined(PRODUCT)