Reland of "Add thread-local allocation context for tracing"

Reland of "Add thread-local allocation context for tracing"

This is a reland of https://crrev.com/1340013002.

Original issue's description:
> This introduces a thread-local context that can be used for heap
> profiling. It contains a pseudo stack where trace events are the
> stack frames. There is a fast early-out in case capturing context
> is disabled.
>
> This is a prerequisite for the heap profiler in chrome://tracing. It
> will log allocations with context obtained from the system introduced
> here.
>
> This is intended to replace the tcmalloc-specific heap profiler in
> base/trace_event/trace_event_memory and its pseudo stack once the heap
> profiler lands.
>
> BUG=524631
>
> Committed: https://crrev.com/abff8221c8d35916a8d7e12845ba61bc5c48aff8
> Cr-Commit-Position: refs/heads/master@{#350799}

BUG=524631
TBR=primiano@chromium.org

Committed: https://crrev.com/a9576610c2d86860afd8a331197285a73b0ee7ac
Cr-Commit-Position: refs/heads/master@{#350880}

[Primiano Tucci (use gerrit), 2015-09-25 16:53:50]

https://codereview.chromium.org/1368993002/diff/20001/base/trace_event/memory...
File base/trace_event/memory_profiler_allocation_context_unittest.cc (right):

https://codereview.chromium.org/1368993002/diff/20001/base/trace_event/memory...
base/trace_event/memory_profiler_allocation_context_unittest.cc:14: const char*
kCupcake = "Cupcake";
nit: const char kCupcake[] = "Cupcake"; and similar below

https://codereview.chromium.org/1368993002/diff/20001/base/trace_event/memory...
base/trace_event/memory_profiler_allocation_context_unittest.cc:64: StackFrame c
= kCupcake;
do you still need this level of indirection?

[Primiano Tucci (use gerrit), 2015-09-25 16:55:13]

Anyways LGTM once you fix the char[]

[Ruud van Asseldonk, 2015-09-25 17:37:29]

The CQ bit was checked by ruuda@google.com

[Ruud van Asseldonk, 2015-09-25 17:37:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org
Link to the patchset: https://codereview.chromium.org/1368993002/#ps40001
(title: "Address primiano comment")

[commit-bot: I haz the power, 2015-09-25 17:38:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1368993002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1368993002/40001

[commit-bot: I haz the power, 2015-09-25 19:16:56]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-09-25 19:17:40]

Patchset 3 (id:??) landed as
https://crrev.com/a9576610c2d86860afd8a331197285a73b0ee7ac
Cr-Commit-Position: refs/heads/master@{#350880}

[Dmitry Skiba, 2015-09-29 19:00:16]

https://codereview.chromium.org/1368993002/diff/40001/base/trace_event/memory...
File base/trace_event/memory_profiler_allocation_context.cc (right):

https://codereview.chromium.org/1368993002/diff/40001/base/trace_event/memory...
base/trace_event/memory_profiler_allocation_context.cc:30: if
(g_tls_alloc_ctx_tracker.initialized()) {
Hmm, so we are setting g_tls_alloc_ctx_tracker value only once? Shouldn't this
be .Get() instead? (And .Set() in else branch, and .Initialize() as a separate
thread-safe test?)

[Primiano Tucci (use gerrit), 2015-09-30 08:26:31]

Thanks for the comment dmitry!
Ruud: I think we need to fix this in the next CLs

See more below

https://codereview.chromium.org/1368993002/diff/40001/base/trace_event/memory...
File base/trace_event/memory_profiler_allocation_context.cc (right):

https://codereview.chromium.org/1368993002/diff/40001/base/trace_event/memory...
base/trace_event/memory_profiler_allocation_context.cc:30: if
(g_tls_alloc_ctx_tracker.initialized()) {
Ahh I think I see your point. Yes, looks like the TLS .Initialize() should be
called once per all, not once per thread (the dtor would still be executed per
thread).
Right, I think this CL needs to be changed as follow:

move initialized() check + Initialize to SetCaptureEnabled() (making the
capture_enabled_ fully barrierer-ed)

In this code we should just:
get() else set()


capture_enabled() should be strengthened to respect the barrier in the slow
path, i.e.

bool capture_enabled()
{
 if (!UNLIKELY(subtle::NoBarrier_Load(&capture_enabled_)))
   return false;
 
  // At this point we are in the slow path
  // Do a barriered read, as the previous one is not enough to guarantee that
the TLS has been constructed (not on all platforms).
  if (!subtle::Acquire_Load(&capture_enabled_))))
    return false;

  return true;
}

[Ruud van Asseldonk, 2015-10-05 09:31:27]

On 2015/09/30 at 08:26:31, primiano wrote:

>   // At this point we are in the slow path
>   // Do a barriered read, as the previous one is not enough to guarantee that
the TLS has been constructed (not on all platforms).
>   if (!subtle::Acquire_Load(&capture_enabled_))))
>     return false;

Why is that? If |capture_enabled_| is initialized to false, and it is set to
true by a fully barriered write, then you can observe a stale false from a
nobarrier read, which is harmless. You can observe a stale true when disabling
capturing but that is not a problem because at that point the TLS has been
initialized. If you ever read a true then TLS has been initialized.