metrics: Connect leak detector to allocator

base/allocator/allocator_extension.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/allocator/allocator_extension.h"
 
 #include "base/logging.h"
 
 #if defined(USE_TCMALLOC)
 #include "third_party/tcmalloc/chromium/src/gperftools/heap-profiler.h"
 #include "third_party/tcmalloc/chromium/src/gperftools/malloc_extension.h"
+#include "third_party/tcmalloc/chromium/src/gperftools/malloc_hook.h"
 #endif
 
 namespace base {
 namespace allocator {
 
 void ReleaseFreeMemory() {
 #if defined(USE_TCMALLOC)
   ::MallocExtension::instance()->ReleaseFreeMemory();
 #endif
 }
 
 bool GetNumericProperty(const char* name, size_t* value) {
 #if defined(USE_TCMALLOC)
   return ::MallocExtension::instance()->GetNumericProperty(name, value);
 #endif
   return false;
 }
 
 bool IsHeapProfilerRunning() {
 #if defined(USE_TCMALLOC)
   return ::IsHeapProfilerRunning();
 #endif
   return false;
 }
 
+AllocHookFunc SetSingleAllocHook(AllocHookFunc func) {
+#if defined(USE_TCMALLOC)
+  return MallocHook::SetNewHook(func);

[Primiano Tucci (use gerrit), 2016/03/01 21:10:28]

Just looked at malloc_hook.h, it looks SetNewHook is marked as deprecated.
Looks like what you want here is AddNewHook

[Simon Que, 2016/03/02 06:25:09]

I specifically went with the deprecated version because of performance. There is
a substantial overhead to using the new hook system because the hooks are copied
into a local array before being invoked, to allow for hook functions to
add/remove hooks.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/tcmall...

The deprecated system has a single hook which is invoked directly, and incurs no
extra overhead.

Running locally, I see a total of 1.5% overhead from Invoke{New|Delete}HookSlow.

See also:
https://docs.google.com/presentation/d/1DTc7d1e7CIFSIpAhfm7f41PepkwSReInn-3se...

That said, I could still simplify this into SetHooks(newhook, deletehook) that
sets the deprecated hook.

[Primiano Tucci (use gerrit), 2016/03/02 06:39:54]

Ok let's keep these deprecated ones. Worst case when they become deprectated for
real should not be a great deal refactoring the code in base to make it work.
Also, very likely the shim layer will happen before.

+#endif
+  return nullptr;
+}
+
+FreeHookFunc SetSingleFreeHook(FreeHookFunc func) {
+#if defined(USE_TCMALLOC)
+  return MallocHook::SetDeleteHook(func);

[Primiano Tucci (use gerrit), 2016/03/01 21:10:27]

And RemoveNewHook here

+#endif
+  return nullptr;
+}
+
+AllocHookFunc GetSingleAllocHook() {
+#if defined(USE_TCMALLOC)
+  // Due to the way things are implemented in tcmalloc's
+  // gperftools/malloc_hook.h, MallocHook::GetNewHook() cannot be called.
+  // Instead, overwrite the stored callback with nullptr and get the stored
+  // callback as a return value from SetNewHook(). Then write it back as the
+  // stored value. Same goes for GetSingleFreeHook().
+  //
+  // This implementation should not have too much overhead as long as
+  // MallocHook's deprecated hook registration functions are trivial.
+  AllocHookFunc existing_hook = MallocHook::SetNewHook(nullptr);

[Primiano Tucci (use gerrit), 2016/03/01 21:10:27]

This is racy by design, how do you prevent that something else doesn't
AddNewHook in the meantime?
This cannot be a base API as is.
Let's keep the base api simple, and have just a void
SetAllocatorHooks(malloc_func_ptr, free_func_ptr) method. Inside the method do a
DCHECK with a bool as in my previous comment to check that you are not
overriding a previously hooks.
There doesn't seem to be a sane way of implementing a Get here, so let's not
have a get.
Doesn't really seem you need it for anything else other than CHECK.

[Simon Que, 2016/03/02 06:25:09]

Doesn't quite work that way, since the non-deprecated hook system uses a list of
hooks. I can add a Remove(newhook,deletehook) function though.

[Primiano Tucci (use gerrit), 2016/03/02 06:39:54]

Is this problem solved (i.e. can we get rid of these getters) if we use the
deprectaed setters?

+  MallocHook::SetNewHook(existing_hook);
+  return existing_hook;
+#endif
+  return nullptr;
+}
+
+FreeHookFunc GetSingleFreeHook() {
+#if defined(USE_TCMALLOC)
+  FreeHookFunc existing_hook = MallocHook::SetDeleteHook(nullptr);
+  MallocHook::SetDeleteHook(existing_hook);
+  return existing_hook;
+#endif
+  return nullptr;
+}
+
+int GetCallStack(void** stack, int max_stack_size, int /* skip_count */) {
+#if defined(USE_TCMALLOC)
+  // |skip_count| is not used by the following function. Instead, the number of
+  // frames to skip is determined internally.
+  // See MallocHook_GetCallerStackTrace() in
+  // third_party/tcmalloc/chromium/src/malloc_hook.cc.
+  return MallocHook::GetCallerStackTrace(stack, max_stack_size, 0);
+#endif
+  return 0;
+}
+
 }  // namespace allocator
 }  // namespace base