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

base/allocator/type_profiler.cc

+// Copyright 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/type_profiler.h"
+
+#if defined(TYPE_PROFILING)
+
+namespace base {
+namespace type_profiler {
+
+static InterceptFunction* g_new_intercept = NULL;
+static InterceptFunction* g_delete_intercept = NULL;
+
+void SetInterceptFunctions(InterceptFunction* new_intercept,

[jar (doing other things), 2012/09/02 01:37:11]

You added a comment on IsInterceptFunctionAvailable(), when I asked you a
question about the thread safety of SetInterceptFunction(), and whether it is
guaranteed to only be called while Chrome is single threaded.

When is this function called?

The header file should state that it must not be called in a multi-threaded
context.

[Dai Mikurube (NOT FULLTIME), 2012/09/03 08:16:30]

ContentMainRunnerImpl::Initialize (content/app/content_main_runner.cc) is the
only caller except for unit tests.

My original thought was it should be called only ONCE in a process.  I forgot to
describe it.  (Many people gave me suggestions around here.  I was a little
confused like
http://codereview.chromium.org/10411047/diff/127003/base/allocator/type_profi...
.)

I added checks in this function to show it (a similar idea with the long-living
base/allocator/allocator_extensions), and added comments on the header.  It
doesn't work perfectly for multi-thread context, but locking sounds too much for
this (allocator_extensions, too).

Making ContentMainRunnerImpl a 'friend' may be an additional idea...

+                           InterceptFunction* delete_intercept) {
+  g_new_intercept = new_intercept;
+  g_delete_intercept = delete_intercept;
+}
+
+// This function is not thread safe.  Use it only when SetInterceptFunctions()
+// is called in a single-threaded status such as type_profiler_unittests.

[jar (doing other things), 2012/09/02 01:37:11]

This comment should appear in the header, rather than the implementation.  As
noted, it would be better to have a private member function, which only friend
(tests) can call.

[Dai Mikurube (NOT FULLTIME), 2012/09/03 08:16:30]

Done.

+bool IsInterceptFunctionsAvailable() {
+  return g_new_intercept != NULL || g_delete_intercept != NULL;
+}
+
+}  // namespace type_profiler
+}  // namespace base
+
+void* __op_new_intercept__(void* ptr,
+                           size_t size,
+                           const std::type_info& type) {
+  base::type_profiler::InterceptFunction* new_intercept =

[jar (doing other things), 2012/09/02 01:37:11]

As noted above, this is a racy assignment, relative to SetInterceptFunctions(). 
It is critical that this only be called after a SetInterceptFunctions() call has
been made. 

Why are you pulling the value into a local, rather than accessing the global? 
Is this a performance optimization attempt, or were you hoping to resolve the
race?  ...or is there another reason?

[Dai Mikurube (NOT FULLTIME), 2012/09/03 08:16:30]

It is because of maruel's suggestion...

http://codereview.chromium.org/10411047/diff/71025/base/allocator/allocated_t...
http://codereview.chromium.org/10411047/diff/80024/base/allocator/allocated_t...

[jar (doing other things), 2012/09/04 17:40:38]

I put comments into the thread you suggested.

My bottom line summary: You should be using locks here.

On 2012/09/03 08:16:30, Dai Mikurube wrote:

+      base::type_profiler::g_new_intercept;
+  if (new_intercept)
+    return new_intercept(ptr, size, type);
+
+  return ptr;
+}
+
+void* __op_delete_intercept__(void* ptr,
+                              size_t size,
+                              const std::type_info& type) {
+  base::type_profiler::InterceptFunction* delete_intercept =
+      base::type_profiler::g_delete_intercept;
+  if (delete_intercept)
+    return delete_intercept(ptr, size, type);
+
+  return ptr;
+}
+
+#endif  // defined(TYPE_PROFILING)