TraceEvent fixes for thread name changes

Support thread name changes in trace event

This is needed for trace names to appear correctly on startup traces, as early threads start tracing before their name is set.
The thread local boolean was previously defeating the logic to detect name changes (the code was inaccessible), and we'd just permanently record early threads as having no name.

BUG=None
TEST=--trace-startup results in correct names in traces


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109439

[joth, 2011-11-08 10:19:53]

On 2011/11/04 12:43:36, joth wrote:

ping! any thoughts?

[jbates, 2011-11-08 18:44:38]

http://codereview.chromium.org/8470001/diff/4001/base/debug/trace_event.cc
File base/debug/trace_event.cc (right):

http://codereview.chromium.org/8470001/diff/4001/base/debug/trace_event.cc#ne...
base/debug/trace_event.cc:519: if (cur_name && *cur_name &&
Since the steady state is (cur_name == g_current_thread_name.Get().Get()), it
looks like we can avoid two branches here by changing this to:
if (cur_name != g_current_thread_name.Get().Get() &&
    cur_name && *cur_name) {

http://codereview.chromium.org/8470001/diff/4001/base/debug/trace_event.h
File base/debug/trace_event.h (left):

http://codereview.chromium.org/8470001/diff/4001/base/debug/trace_event.h#old...
base/debug/trace_event.h:51: //        The |arg_values|, when used, are always
deep copied with the _COPY
The old comment is true I think, see the use of
base::debug::TraceValue::ForceCopy in the _COPY macro arguments.

[nduca, 2011-11-09 04:02:24]

Make sure this passes the bots.... I think it will but a lot of threads get
created at times when singletons dont work yet/etc.

http://codereview.chromium.org/8470001/diff/4001/base/debug/trace_event.cc
File base/debug/trace_event.cc (right):

http://codereview.chromium.org/8470001/diff/4001/base/debug/trace_event.cc#ne...
base/debug/trace_event.cc:519: if (cur_name && *cur_name &&
If we do that, we might want to explain the logic, or use some temp var names.
Its a bit hard to read that way.

http://codereview.chromium.org/8470001/diff/4001/base/debug/trace_event.h
File base/debug/trace_event.h (right):

http://codereview.chromium.org/8470001/diff/4001/base/debug/trace_event.h#new...
base/debug/trace_event.h:328: struct StaticMemorySingletonTraits;
is this needed?

[nduca, 2011-11-09 04:02:28]

LGTM

[joth, 2011-11-09 13:17:57]

+jar for OWNERS mark of approval.

Thanks!

http://codereview.chromium.org/8470001/diff/4001/base/debug/trace_event.cc
File base/debug/trace_event.cc (right):

http://codereview.chromium.org/8470001/diff/4001/base/debug/trace_event.cc#ne...
base/debug/trace_event.cc:519: if (cur_name && *cur_name &&
On 2011/11/08 18:44:38, jbates wrote:
> Since the steady state is (cur_name == g_current_thread_name.Get().Get()), it
> looks like we can avoid two branches here by changing this to:
> if (cur_name != g_current_thread_name.Get().Get() &&
>     cur_name && *cur_name) {

Done.

http://codereview.chromium.org/8470001/diff/4001/base/debug/trace_event.cc#ne...
base/debug/trace_event.cc:519: if (cur_name && *cur_name &&
On 2011/11/09 04:02:24, nduca wrote:
> If we do that, we might want to explain the logic, or use some temp var names.
> Its a bit hard to read that way.

Done.

http://codereview.chromium.org/8470001/diff/4001/base/debug/trace_event.h
File base/debug/trace_event.h (left):

http://codereview.chromium.org/8470001/diff/4001/base/debug/trace_event.h#old...
base/debug/trace_event.h:51: //        The |arg_values|, when used, are always
deep copied with the _COPY
On 2011/11/08 18:44:38, jbates wrote:
> The old comment is true I think, see the use of
> base::debug::TraceValue::ForceCopy in the _COPY macro arguments.
Oh so they are. I think I was stood on my head when I was reading this or
something.

Reverted.

http://codereview.chromium.org/8470001/diff/4001/base/debug/trace_event.h
File base/debug/trace_event.h (right):

http://codereview.chromium.org/8470001/diff/4001/base/debug/trace_event.h#new...
base/debug/trace_event.h:328: struct StaticMemorySingletonTraits;
On 2011/11/09 04:02:24, nduca wrote:
> is this needed?

yep, used on line 668. This allows singleton.h to be included in the .cc
instead.

[jbates, 2011-11-09 17:50:23]

lgtm

[jar (doing other things), 2011-11-10 00:18:35]

LGTM (tiny nit below)

http://codereview.chromium.org/8470001/diff/10001/base/debug/trace_event.cc
File base/debug/trace_event.cc (right):

http://codereview.chromium.org/8470001/diff/10001/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:519: // Check if the thread name has been set or
changed since th previous
nit typo: th--> the

[joth, 2011-11-10 10:15:26]

Thanks all. Sending to CQ...

http://codereview.chromium.org/8470001/diff/10001/base/debug/trace_event.cc
File base/debug/trace_event.cc (right):

http://codereview.chromium.org/8470001/diff/10001/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:519: // Check if the thread name has been set or
changed since th previous
On 2011/11/10 00:18:35, jar wrote:
> nit typo: th--> the

Done.

[commit-bot: I haz the power, 2011-11-10 10:21:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joth@chromium.org/8470001/13001

[commit-bot: I haz the power, 2011-11-10 10:31:31]

Try job failure for 8470001-13001 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2011-11-10 11:11:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joth@chromium.org/8470001/20001

[commit-bot: I haz the power, 2011-11-10 12:39:43]

Try job failure for 8470001-20001 (retry) on mac_rel for step "base_unittests".
It's a second try, previously, step "base_unittests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2011-11-10 14:19:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joth@chromium.org/8470001/19003

[commit-bot: I haz the power, 2011-11-10 16:26:30]

Change committed as 109439