profiler: decouple ThreadHeapUsageTracker from allocator shim internals

base/debug/thread_heap_usage_tracker.cc

 // Copyright 2016 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/debug/thread_heap_usage_tracker.h"
 
 #include <stdint.h>
 #include <algorithm>
 #include <new>
 #include <type_traits>
@@ skipping 12 matching lines @@
 
 namespace base {
 namespace debug {
 
 namespace {
 
 using base::allocator::AllocatorDispatch;
 
 ThreadLocalStorage::StaticSlot g_thread_allocator_usage = TLS_INITIALIZER;
 
-ThreadHeapUsage* const kInitializingSentinel =
+ThreadHeapUsage* const kInitializationOrTeardownSentinel =

[Sigurður Ásgeirsson, 2017/02/01 22:21:27]

I'd be tempted to use two sentinels here, as you have a state machine going

unintialized -> being initialized -> initialized -> de-initialized

where re-initialization should never happen. I worry that when/if this derails,
it'll be a bear to figure out what's derailed with one sentinel only. This does
however imply a second comparison (or something like a CHECK) in
GetOrCreateThreadUsage...

[Primiano Tucci (use gerrit), 2017/02/01 22:39:05]

Yeah I thought to this. Honestly the only reason I didn't do that was to avoid a
double if, fearing to slow down the TUT. Thinking more though, I think I can
come up with a kSentinelMask, so keep still one branch but two distinct value

     reinterpret_cast<ThreadHeapUsage*>(-1);
 
 bool g_heap_tracking_enabled = false;
 
 // Forward declared as it needs to delegate memory allocation to the next
 // lower shim.
 ThreadHeapUsage* GetOrCreateThreadUsage();
 
 size_t GetAllocSizeEstimate(const AllocatorDispatch* next, void* ptr) {
   if (ptr == nullptr)
@@ skipping 123 matching lines @@
                                         &FreeFn,
                                         &GetSizeEstimateFn,
                                         &BatchMallocFn,
                                         &BatchFreeFn,
                                         &FreeDefiniteSizeFn,
                                         nullptr};
 
 ThreadHeapUsage* GetOrCreateThreadUsage() {
   ThreadHeapUsage* allocator_usage =
       static_cast<ThreadHeapUsage*>(g_thread_allocator_usage.Get());
-  if (allocator_usage == kInitializingSentinel)
+  if (allocator_usage == kInitializationOrTeardownSentinel)
     return nullptr;  // Re-entrancy case.
 
   if (allocator_usage == nullptr) {
     // Prevent reentrancy due to the allocation below.
-    g_thread_allocator_usage.Set(kInitializingSentinel);
+    g_thread_allocator_usage.Set(kInitializationOrTeardownSentinel);
 
-    // Delegate the allocation of the per-thread structure to the underlying
-    // heap shim, for symmetry with the deallocation. Otherwise interposing
-    // shims may mis-attribute or mis-direct this allocation.
-    const AllocatorDispatch* next = allocator_dispatch.next;
-    allocator_usage = new (next->alloc_function(next, sizeof(ThreadHeapUsage)))
-        ThreadHeapUsage();
+    allocator_usage = new ThreadHeapUsage();

[Sigurður Ásgeirsson, 2017/02/01 22:21:27]

comically this'll lead to the shim accounting its own usage, except on Mac :).

[Primiano Tucci (use gerrit), 2017/02/01 22:39:05]

Hmm how? This shouldn't happen because we set the setinel up, which would cause
the GetOrCreate to return nullptr and hence skip the accounting (same on
deletion below)

[Sigurður Ásgeirsson, 2017/02/01 22:46:16]

Ah, true :).

     static_assert(std::is_pod<ThreadHeapUsage>::value,
                   "AllocatorDispatch must be POD");
     memset(allocator_usage, 0, sizeof(*allocator_usage));
     g_thread_allocator_usage.Set(allocator_usage);
   }
 
   return allocator_usage;
 }
 
 }  // namespace
@@ skipping 90 matching lines @@
   g_heap_tracking_enabled = false;
 }
 
 base::allocator::AllocatorDispatch*
 ThreadHeapUsageTracker::GetDispatchForTesting() {
   return &allocator_dispatch;
 }
 
 void ThreadHeapUsageTracker::EnsureTLSInitialized() {
   if (!g_thread_allocator_usage.initialized()) {
-    g_thread_allocator_usage.Initialize([](void* allocator_usage) {
-      // Delegate the freeing of the per-thread structure to the next-lower
-      // heap shim. Otherwise this free will re-initialize the TLS on thread
-      // exit.
-      allocator_dispatch.next->free_function(allocator_dispatch.next,
-                                             allocator_usage);
+    g_thread_allocator_usage.Initialize([](void* thread_heap_usage) {
+      // Deleting the ThreadHeapUsage TLS object will re-enter the shim and hit
+      // RecordFree() above. The sentinel prevents RecordFree() from re-creating
+      // another ThreadHeapUsage object.
+      if (thread_heap_usage == kInitializationOrTeardownSentinel)

[Sigurður Ásgeirsson, 2017/02/01 22:21:27]

How would this ever happen?

[Primiano Tucci (use gerrit), 2017/02/01 22:39:05]

Ha! this is tricky but will happen all the times.
This destructor is called after the TLS slot is cleared up. So re-setting the
sentinel (the line below) will re-create the TLS slot. so this destructor will
be called a second time immediately after this returns. the second time though
the slot contains the sentinel and we early out.
Thankfully the TLS code explicitly mentions and supports this case. see 
https://cs.chromium.org/chromium/src/base/threading/thread_local_storage.cc?r...

[Sigurður Ásgeirsson, 2017/02/01 22:46:16]

Ah - perhaps this warrants a quick comment, as it's totally non-obvious?

[Primiano Tucci (use gerrit), 2017/02/02 00:38:27]

Done.

+        return;
+      g_thread_allocator_usage.Set(kInitializationOrTeardownSentinel);
+      delete static_cast<ThreadHeapUsage*>(thread_heap_usage);
     });
   }
 }
 
 }  // namespace debug
 }  // namespace base