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

base/allocator/allocated_type_tcmalloc.cc

Index: base/allocator/allocated_type_tcmalloc.cc
diff --git a/base/allocator/allocated_type_tcmalloc.cc b/base/allocator/allocated_type_tcmalloc.cc
new file mode 100644
index 0000000000000000000000000000000000000000..0e13a2d20698df55c779f47053eda99e9d05eb49
--- /dev/null
+++ b/base/allocator/allocated_type_tcmalloc.cc
@@ -0,0 +1,39 @@
+// 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 "allocated_type.h"
+#include "allocated_type_tcmalloc.h"
+#include "base/atomicops.h"
+#include <gperftools/allocated_type_map.h>
+#include <gperftools/heap-profiler.h>
+
+volatile Atomic32 do_not_intercept = 0;

[jar (doing other things), 2012/08/02 23:38:30]

It is not clear why you are using atomics.

When defining logical variables, in is always confusing to have a negative in
the name.

Suggest using:

g_enable_intercept

which is probably the negation of what you have.

Also note that globals should be prefixed by "g_"

[Dai Mikurube (NOT FULLTIME), 2012/08/03 10:01:50]

The reason why I use Atomic is they may be called from multiple threads.  I
would like to disable the interceptors whenever this value is not zero (in other
words, whenever someone is in a dangerous zone -- between fork and exec).

I'm afraid LaunchProcess() (the caller of these functions in
process_util_posix.cc) can work in parallel.  Or, is it guaranteed not to work
in parallel?

I chose g_disable_intercept_if_positive since it disables interceptors while it
is not zero.

[jar (doing other things), 2012/08/04 01:13:47]

Please use a positive name, rather than a negative "disable."

In this case, please switch to g_enable_intercept_if_positive

Use of a negative complicates reading a fair amount. An example where this is
commonly (sadly) seen is NDEBUG.  This should IMO have been defined as DEBUG, so
that you don't have to do mental gymnastics and cancel out a double negative to
figure out what is happening.

I learned of this philosophy years ago, when discussing compiler flags, which
*should* always be in the positive, and shouldn't have words like "disable" or
"no" in their names.

On 2012/08/03 10:01:50, Dai Mikurube wrote:

[jar (doing other things), 2012/08/04 01:13:47]

re: atomics

If this is accessed on multiple threads, then the results will be pretty
unpredictable, as allocations and deallocations will be lost when some thread
asynchronously turns this flag on and off.

Is that confusion, such as a deallocation not being found in the allocation map,
acceptable?  Is it acceptable that a deallocation may be for a different type
than the allocation, because an intervening deallocation and allocation was
missed?

On 2012/08/03 10:01:50, Dai Mikurube wrote:

[Dai Mikurube (NOT FULLTIME), 2012/08/07 10:39:18]

re: atomics
Yes, it is acceptable for now.

The allocated_type_map is mainly designed as a helper for a long-running
profiler (DMP). DMP focuses on trend of memory usage... some error is
acceptable.

The "disabled" time is usually very short compared to the entire runtime. 
(LaunchProcess and GetAppOutputInternal called rarely.)

Ot doesn't work well for tests of LaunchProcess and GetAppOutputInternal, of
course.  We give up it for now.

Ok, now I know it is confusing behavior.  I add a comment on it.

On 2012/08/04 01:13:47, jar wrote:

[Dai Mikurube (NOT FULLTIME), 2012/08/07 10:39:18]

re: name
Ok, your suggestion is so reasonable.  But in this case,
"g_enable_intercept_if_zero(_or_negative)"?  "g_enable_intercept_if_positive"
tells a lie.

Or, another way may be to make the default value "1", and decrement in
Suspend...().  Which do you prefer?

On 2012/08/04 01:13:47, jar wrote:

+
+void SuspendAllocatedTypeIntercept() {
+  base::subtle::NoBarrier_AtomicIncrement(&do_not_intercept, 1);

[jar (doing other things), 2012/08/02 23:38:30]

Given the name of the function, I suspect you should do an AtomicExchange, and
not an increment.  Increments would be appropriate if calls nested.

Since the variable started out at zero, which I assume means false, and the name
was negating, I think you start out with a default of intercepting.  As a
result, I would guess that the only legal call at that point would this
SuspendAllocatedTypeIntercept (which drives the value more positive).  It is
interesting that with the current incrementing semantics, a call to
ResumeAlocatedTypeIntercept would *also* turn off interception :-/.

[Dai Mikurube (NOT FULLTIME), 2012/08/03 10:01:50]

I agree with that ResumeAllocatedTypeIntercept before Suspend would disable
intercepting.  I changed the disabling condition to
g_disable_intercept_if_positive > 0.

+}
+
+void ResumeAllocatedTypeIntercept() {
+  base::subtle::NoBarrier_AtomicIncrement(&do_not_intercept, -1);
+}
+
+void* __op_new_intercept__(
+    void* ptr, size_t size, const std::type_info& type) {

[jar (doing other things), 2012/08/02 23:38:30]

nit: wrap to match the indent of the paren, and put one arg per line.

[Dai Mikurube (NOT FULLTIME), 2012/08/03 10:01:50]

Done.

+  if (do_not_intercept == 0) {

[jar (doing other things), 2012/08/02 23:38:30]

You should not access an atomic directly.  Use NoBarrier_Load().

...but I have no idea why you're doing this with atomics.

[Dai Mikurube (NOT FULLTIME), 2012/08/03 10:01:50]

Done.

+    if (IsHeapProfilerRunning()) {
+      InsertAllocatedType(ptr, size, type);
+    }
+  }
+  return ptr;
+}
+
+void* __op_delete_intercept__(
+    void* ptr, size_t size, const std::type_info &type) {

[jar (doing other things), 2012/08/02 23:38:30]

nit: wrap to match paren indent.

[Dai Mikurube (NOT FULLTIME), 2012/08/03 10:01:50]

Done.

+  if (do_not_intercept == 0) {
+    if (IsHeapProfilerRunning()) {
+      EraseAllocatedType(ptr);
+    }
+  }
+  return ptr;
+}