Type profiler by intercepting 'new' and 'delete' expressions.

third_party/tcmalloc/chromium/src/allocated_type_map.cc

Index: third_party/tcmalloc/chromium/src/allocated_type_map.cc
diff --git a/third_party/tcmalloc/chromium/src/allocated_type_map.cc b/third_party/tcmalloc/chromium/src/allocated_type_map.cc
new file mode 100644
index 0000000000000000000000000000000000000000..8f641ccda7d11136844f014f1edd75668551a7b8
--- /dev/null
+++ b/third_party/tcmalloc/chromium/src/allocated_type_map.cc
@@ -0,0 +1,90 @@
+// 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 <config.h>
+
+#include <new>
+#include <typeinfo>
+
+#include <gperftools/allocated_type_map.h>
+
+#include "addressmap-inl.h"
+#include "base/logging.h"
+#include "base/low_level_alloc.h"
+#include "base/spinlock.h"
+#include "tcmalloc_guard.h"
+
+
+//----------------------------------------------------------------------
+// Locking
+//----------------------------------------------------------------------
+
+static SpinLock allocated_type_lock(SpinLock::LINKER_INITIALIZED);

[Ryan Sleevi, 2012/08/10 04:16:09]

Please consider the guidance from Google's Open Source C++ Style Guide,
specifically
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Variab...

"There are no special requirements for global variables, which should be rare in
any case, but if you use one, consider prefixing it with g_ or some other marker
to easily distinguish it from local variables."

I'll defer to TCMalloc style here, but the current naming definitely makes it
subtle.

[Dai Mikurube (NOT FULLTIME), 2012/08/10 07:30:03]

It was following other tcmalloc files.  I renamed the variables in this file.

+
+//----------------------------------------------------------------------
+// Simple allocator for allocated type map's internal memory
+//----------------------------------------------------------------------
+
+static LowLevelAlloc::Arena* allocated_type_map_memory = NULL;
+
+static void* AllocatedTypeMalloc(size_t bytes) {
+  return LowLevelAlloc::AllocWithArena(bytes, allocated_type_map_memory);
+}
+
+static void AllocatedTypeFree(void* p) {
+  LowLevelAlloc::Free(p);
+}
+
+//----------------------------------------------------------------------
+// Profiling control/state data
+//----------------------------------------------------------------------
+
+static AddressMap<AllocatedObject>* allocated_type_map = NULL;
+
+//----------------------------------------------------------------------
+// Manage allocated type map
+//----------------------------------------------------------------------
+
+static void InitializeAllocatedTypeMemory() {
+  if (allocated_type_map_memory != NULL) {
+    DCHECK_NE(allocated_type_map, NULL);

[Ryan Sleevi, 2012/08/10 04:16:09]

BUG? Your use of std::cerr in the _logger wrappers suggests to me that you're
trying to avoid any form of heap allocation during error logging, in the event
of re-entrancy. Thus, the use of DCHECK_NE here seems counter-productive to
that, since DCHECK_NE may invoke the DLOG() operators and/or re-enter
new/delete.

It seems to me that you should either:
1) CHECK
2) Remove this

[Dai Mikurube (NOT FULLTIME), 2012/08/10 07:30:03]

_log doesn't call this function.  Functions in this file are called only from
_tcmalloc.  (_log is rarely used as I replied, that can be removed if that's not
good to commit.)

This checks "g_allocated_type_map and g_allocated_type_map_memory get non-NULL
value at the same time.  Both are NULL, or both are non-NULL."  Usually, it
doesn't happen because *AllocatedType functions have locks.

[Ryan Sleevi, 2012/08/10 09:38:50]

I think you may have missed my point. I'm aware of what it is checking, but my
concern is you may be setting yourself up for a recursion overflow.

Consider when this situation happens (how/when it can happen was described in
the original comment for line 60). Your initial callstack will look something
like:

0: InitializeAllocatedTypeMemory()
1: InsertAllocatedType()
2: __op_new_intercept__()
3: operator new(...)

This DCHECK_NE fires, which will call DLOG() with the DCHECK_NE's statement,
which will call logging::LogMessage(). If logging::LogMessage() calls new, your
stack will look like

0: InitializeAllocatedTypeMemory()
1: InsertAllocatedType()
2: __op_new_intercept__()
3: operator new(...)
4: logging::LogMessage() [expanded from DCHECK_NE]
5: InitializeAllocatedTypeMemory()
6: InsertAllocatedType()
7: __op_new_intercept__()
8: operator new(...)

and thus you'll end up re-entering yourself and recursing repeatedly. The
recursion "should" be bounded by the time that it takes the cache lines to
update, so it shouldn't run off forever.

While this file's operator new calls are not interposed (because you have it
excluded at the .gypi level), thus making line 58-60 safe, logging::LogMessage
is part of the base library, which is interposed, and thus does seem risky.

However, the point being is that any time you hit this DCHECK, you reasonably
know that any further allocations (with new) will also hit this DCHECK, which
means you *could* end up looping indefinitely. It would be safer thus to not
have this DCHECK or to forcibly crash at this point, since it's hard to get
right.

This is probably entirely contingent upon how you've got Clang doing the
interposing, but it's very subtle that you're "inside new". It makes me think
any new is at risk of recursion.

[Dai Mikurube (NOT FULLTIME), 2012/08/13 12:57:42]

Sorry, I overlooked it.  I'll handle it tomorrow.
(It's introduced at
http://codereview.chromium.org/10411047/diff/72024/third_party/tcmalloc/chrom....
 Reverting may be one idea.)

[Dai Mikurube (NOT FULLTIME), 2012/08/14 05:30:48]

Actually, this DCHECK_NE is in third_party/tcmalloc/chromium/src/base/logging.h.
 LogMessage() is not used.

But, for safety, I replaced it to RAW_DCHECK, which is defined only in
third_party/tcmalloc/chromium/src/base/logging.h.

+    return;
+  }
+
+  allocated_type_map_memory =
+      LowLevelAlloc::NewArena(0, LowLevelAlloc::DefaultArena());
+
+  allocated_type_map =
+      new(AllocatedTypeMalloc(sizeof(AddressMap<AllocatedObject>)))
+          AddressMap<AllocatedObject>(AllocatedTypeMalloc, AllocatedTypeFree);

[Ryan Sleevi, 2012/08/10 04:16:09]

BUG?

Do you not need to add any barriers here? Does the SpinLockHolder properly
introduce memory barriers?

I'm just wondering whether it's possible for the spinlock to be released by
InsertAllocatedType before other cache lines/CPUs have updated
allocated_type_map_memory + allocated_type_map, which might cause the DCHECK to
fire on 51

[Dai Mikurube (NOT FULLTIME), 2012/08/10 07:30:03]

Ah, I understand your question.  I was thinking it doesn't happen.  atomicops
(e.g. Acquire_CompareAndSwap) works with lock-prefixes.

+}
+
+void InsertAllocatedType(void* address,
+                         unsigned int size,
+                         const std::type_info& type) {
+  SpinLockHolder l(&allocated_type_lock);
+  InitializeAllocatedTypeMemory();
+
+  allocated_type_map->Insert(address, AllocatedObject(size, &type));
+}
+
+void EraseAllocatedType(void* address) {
+  SpinLockHolder l(&allocated_type_lock);
+  InitializeAllocatedTypeMemory();
+
+  AllocatedObject obj;
+  allocated_type_map->FindAndRemove(address, &obj);
+}
+
+const std::type_info* LookupAllocatedType(const void* address) {
+  SpinLockHolder l(&allocated_type_lock);
+  InitializeAllocatedTypeMemory();
+
+  const AllocatedObject* found = allocated_type_map->Find(address);
+  if (found == NULL)
+    return NULL;
+  return found->type;
+}
+
+static const TCMallocGuard tcmalloc_initializer;