Implement standalone event tracing in AppRTCDemo.

Implement standalone event tracing in AppRTCDemo.

Logs tracing events (TRACE_EVENT0 and friends) to storage in a format
compatible with chrome://tracing which can be used for performance
evaluation, finding lock contention and other sweet things). Tracing is
still basic and doesn't contain thread metadata or logging of tracing
arguments.

BUG=webrtc:5158
R=tommi@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/6f28cf0b951a9d41246f022f48a6cd035fad151d

[pbos-webrtc, 2015-11-19 18:34:14]

Tommi can you start taking a look? This is in a working state, but I need
another pair of eyes on it. Not all TODOs are in place etc, but I think you can
start so that I get an overall feel of what state this thing is in.

[pbos-webrtc, 2015-11-19 19:10:32]

add tracing shutdown

[pbos-webrtc, 2015-11-19 19:11:50]

The CQ bit was checked by pbos@webrtc.org to run a CQ dry run

[pbos-webrtc, 2015-11-19 19:12:09]

Description was changed from

==========
Implement standalone event tracing in AppRTCDemo.

Logs tracing events (TRACE_EVENT0 and friends) to storage in a format
compatible with chrome://tracing which can be used for performance
evaluation, finding lock contention and other sweet things). Tracing is
still basic and doesn't contain certain metadata or logging of tracing
arguments.

BUG=webrtc:5158
R=tommi@webrtc.org
==========

to

==========
Implement standalone event tracing in AppRTCDemo.

Logs tracing events (TRACE_EVENT0 and friends) to storage in a format
compatible with chrome://tracing which can be used for performance
evaluation, finding lock contention and other sweet things). Tracing is
still basic and doesn't contain thread metadata or logging of tracing
arguments.

BUG=webrtc:5158
R=tommi@webrtc.org
==========

[commit-bot: I haz the power, 2015-11-19 19:12:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1457383002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1457383002/20001

[commit-bot: I haz the power, 2015-11-19 19:13:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-19 19:13:57]

Dry run: Try jobs failed on following builders:
  android_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/9162)
  mac_x64_gn_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_x64_gn_dbg/builds/5373)

[pbos-webrtc, 2015-11-19 19:15:40]

fix test_main.cc

[pbos-webrtc, 2015-11-19 19:16:08]

The CQ bit was checked by pbos@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-19 19:16:13]

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

[commit-bot: I haz the power, 2015-11-19 19:19:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-19 19:19:12]

Dry run: Try jobs failed on following builders:
  linux_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_gn_rel/builds/6879)

[pbos-webrtc, 2015-11-19 19:23:22]

fix race + test compile

[pbos-webrtc, 2015-11-19 19:23:35]

The CQ bit was checked by pbos@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-19 19:23:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1457383002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1457383002/60001

[pbos-webrtc, 2015-11-19 19:25:24]

revert test_main.cc, it shouldn't be on for all tests either way

[pbos-webrtc, 2015-11-19 19:25:54]

I'll stop slamming CQ now and make sure this runs locally, but PTAL as soon as
you have time. :)

[pbos-webrtc, 2015-11-19 19:40:45]

On 2015/11/19 19:25:54, pbos-webrtc wrote:
> I'll stop slamming CQ now and make sure this runs locally, but PTAL as soon as
> you have time. :)

What's causing shutdown slowdowns? Stay tuned for next week's Tracing-and-You
episode: http://i.imgur.com/wM6v6wv.png

[tommi, 2015-11-20 08:48:23]

On 2015/11/19 19:40:45, pbos-webrtc wrote:
> On 2015/11/19 19:25:54, pbos-webrtc wrote:
> > I'll stop slamming CQ now and make sure this runs locally, but PTAL as soon
as
> > you have time. :)
> 
> What's causing shutdown slowdowns? Stay tuned for next week's Tracing-and-You
> episode: http://i.imgur.com/wM6v6wv.png

My favorite show!

[pbos-webrtc, 2015-11-20 10:47:15]

prefix checking + cl format

[tommi, 2015-11-20 13:49:54]

https://codereview.webrtc.org/1457383002/diff/80001/talk/app/webrtc/java/jni/...
File talk/app/webrtc/java/jni/peerconnection_jni.cc (right):

https://codereview.webrtc.org/1457383002/diff/80001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/peerconnection_jni.cc:1065: if
(j_event_tracing_filename == NULL)
nit: use |if (!ptr)| or |if (ptr == nullptr)|  I personally prefer the former.

https://codereview.webrtc.org/1457383002/diff/80001/webrtc/base/event_tracer.cc
File webrtc/base/event_tracer.cc (right):

https://codereview.webrtc.org/1457383002/diff/80001/webrtc/base/event_tracer....
webrtc/base/event_tracer.cc:76: const unsigned char* category_enabled;
uint8_t?

https://codereview.webrtc.org/1457383002/diff/80001/webrtc/base/event_tracer....
webrtc/base/event_tracer.cc:77: char phase;
what is phase used for?
Is this code borrowed from Chrome to some extent? Can we link to the code on
which this is based, especially format related code?  Would it make sense to
have a test, maybe compile-time type checks, that guarantee compatibility of
webrtc trace logs with chrome's?

https://codereview.webrtc.org/1457383002/diff/80001/webrtc/base/event_tracer....
webrtc/base/event_tracer.cc:85: strstr(name, TRACE_DISABLED_BY_DEFAULT("")) ==
name ? "" : name);
I'm not groking what this actually does or is supposed to do.  if name ==
"webrtc" for example, then this will be:

strstr("webrtc", "disabled-by-default-") == "webrtc" ? "" : "webrtc");

Did you mean to pass 'name' to TRACE_DISABLED_BY_DEFAULT?  That actually doesn't
make sense to me either, so render me confused :)

https://codereview.webrtc.org/1457383002/diff/80001/webrtc/base/event_tracer....
webrtc/base/event_tracer.cc:89: static rtc::CriticalSection* g_event_crit;
also initialize?

can we bundle these variables in a single class and have only a single global
pointer?

https://codereview.webrtc.org/1457383002/diff/80001/webrtc/base/event_tracer....
webrtc/base/event_tracer.cc:97: static bool g_event_output_file_owned;
and this one?

https://codereview.webrtc.org/1457383002/diff/80001/webrtc/base/event_tracer....
webrtc/base/event_tracer.cc:133: for (const TraceEvent e : events) {
copy by value intentionally or missing &?

https://codereview.webrtc.org/1457383002/diff/80001/webrtc/base/event_tracer....
webrtc/base/event_tracer.cc:166: g_event_output_file = output_file;
how do we know we're not overwriting a previous value?  I don't see that it's
guaranteed that StartInternalCaptureToFilePtr isn't called on different threads.
 It seems like there should be a CompareAndSwap atomic operation instead of just
Load.

https://codereview.webrtc.org/1457383002/diff/80001/webrtc/base/event_tracer....
webrtc/base/event_tracer.cc:174: ThreadWrapper::CreateThread(&LoggingThread,
nullptr, "EventTracingThread")
instead of having g_event_output_file be a global, can we pass it to the thread
here and let the thread own it?
if we need to pass the owned flag too, consider passing them in a struct that's
owned by the thread.

https://codereview.webrtc.org/1457383002/diff/80001/webrtc/base/event_tracer....
webrtc/base/event_tracer.cc:209: if
(rtc::AtomicOps::AcquireLoad(&g_event_logging_active) == 0)
this should also be a CompareAndSwap since otherwise we have a race (even though
we're using atomics)

[pbos-webrtc, 2015-11-20 14:56:04]

as class

[pbos-webrtc, 2015-11-20 15:11:06]

PTAL, I'll need to remove the rtc::Thread dependency and instead use
ThreadWrapper, but I can't do that since base can't depend on system wrappers.
I'll have to solve that next week, but this should give a pretty good next step
to review.

https://codereview.webrtc.org/1457383002/diff/80001/talk/app/webrtc/java/jni/...
File talk/app/webrtc/java/jni/peerconnection_jni.cc (right):

https://codereview.webrtc.org/1457383002/diff/80001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/peerconnection_jni.cc:1065: if
(j_event_tracing_filename == NULL)
On 2015/11/20 13:49:54, tommi (webrtc) wrote:
> nit: use |if (!ptr)| or |if (ptr == nullptr)|  I personally prefer the former.

Done.

https://codereview.webrtc.org/1457383002/diff/80001/webrtc/base/event_tracer.cc
File webrtc/base/event_tracer.cc (right):

https://codereview.webrtc.org/1457383002/diff/80001/webrtc/base/event_tracer....
webrtc/base/event_tracer.cc:76: const unsigned char* category_enabled;
On 2015/11/20 13:49:54, tommi (webrtc) wrote:
> uint8_t?

Consistent with AddTraceEvent functions.

https://codereview.webrtc.org/1457383002/diff/80001/webrtc/base/event_tracer....
webrtc/base/event_tracer.cc:77: char phase;
On 2015/11/20 13:49:54, tommi (webrtc) wrote:
> what is phase used for?
> Is this code borrowed from Chrome to some extent? Can we link to the code on
> which this is based, especially format related code?  Would it make sense to
> have a test, maybe compile-time type checks, that guarantee compatibility of
> webrtc trace logs with chrome's?

Done, by referring to webrtc/base/trace_event.h. W/r/t testing, I'd assume
changing those would break existing trace files, but if there should be a test
for this I'd think of it separate (and probably written in Chromium to make sure
that its trace_event.h remains using the same symbols.

Phase is an event type (begin, end, instant) etc.

https://codereview.webrtc.org/1457383002/diff/80001/webrtc/base/event_tracer....
webrtc/base/event_tracer.cc:85: strstr(name, TRACE_DISABLED_BY_DEFAULT("")) ==
name ? "" : name);
On 2015/11/20 13:49:54, tommi (webrtc) wrote:
> I'm not groking what this actually does or is supposed to do.  if name ==
> "webrtc" for example, then this will be:
> 
> strstr("webrtc", "disabled-by-default-") == "webrtc" ? "" : "webrtc");
> 
> Did you mean to pass 'name' to TRACE_DISABLED_BY_DEFAULT?  That actually
doesn't
> make sense to me either, so render me confused :)

Should be a prefix search, I was lazy and did strstr, fixed in another patchset.

https://codereview.webrtc.org/1457383002/diff/80001/webrtc/base/event_tracer....
webrtc/base/event_tracer.cc:89: static rtc::CriticalSection* g_event_crit;
On 2015/11/20 13:49:54, tommi (webrtc) wrote:
> also initialize?
> 
> can we bundle these variables in a single class and have only a single global
> pointer?

class done

https://codereview.webrtc.org/1457383002/diff/80001/webrtc/base/event_tracer....
webrtc/base/event_tracer.cc:97: static bool g_event_output_file_owned;
On 2015/11/20 13:49:54, tommi (webrtc) wrote:
> and this one?

Done.

https://codereview.webrtc.org/1457383002/diff/80001/webrtc/base/event_tracer....
webrtc/base/event_tracer.cc:133: for (const TraceEvent e : events) {
On 2015/11/20 13:49:54, tommi (webrtc) wrote:
> copy by value intentionally or missing &?

Done.

https://codereview.webrtc.org/1457383002/diff/80001/webrtc/base/event_tracer....
webrtc/base/event_tracer.cc:166: g_event_output_file = output_file;
On 2015/11/20 13:49:54, tommi (webrtc) wrote:
> how do we know we're not overwriting a previous value?  I don't see that it's
> guaranteed that StartInternalCaptureToFilePtr isn't called on different
threads.
>  It seems like there should be a CompareAndSwap atomic operation instead of
just
> Load.

Assuming that start and stop are done on the same thread Stop triggers removal
of the output file, so it can't start at the same time.

https://codereview.webrtc.org/1457383002/diff/80001/webrtc/base/event_tracer....
webrtc/base/event_tracer.cc:174: ThreadWrapper::CreateThread(&LoggingThread,
nullptr, "EventTracingThread")
On 2015/11/20 13:49:54, tommi (webrtc) wrote:
> instead of having g_event_output_file be a global, can we pass it to the
thread
> here and let the thread own it?
> if we need to pass the owned flag too, consider passing them in a struct
that's
> owned by the thread.

-> class

https://codereview.webrtc.org/1457383002/diff/80001/webrtc/base/event_tracer....
webrtc/base/event_tracer.cc:209: if
(rtc::AtomicOps::AcquireLoad(&g_event_logging_active) == 0)
On 2015/11/20 13:49:54, tommi (webrtc) wrote:
> this should also be a CompareAndSwap since otherwise we have a race (even
though
> we're using atomics)

This is only a best-effort fast path for early abort. I've added an enabled_
that's protected within the logging class to make sure no events are logged
after we've stopped. Checking that one instead now.

[pbos-webrtc, 2015-11-23 10:24:31]

things I said I'd done

[pbos-webrtc, 2015-11-24 15:37:33]

rebase onto PlatformThread

[pbos-webrtc, 2015-11-24 15:53:44]

fix some bugs after testing on device woo

[pbos-webrtc, 2015-11-24 15:54:04]

Please take another look on your favorite show. :)

[pbos-webrtc, 2015-11-24 15:57:17]

The CQ bit was checked by pbos@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-24 15:57:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1457383002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1457383002/180001

[tommi, 2015-11-24 16:27:09]

https://codereview.webrtc.org/1457383002/diff/180001/webrtc/base/event_tracer.cc
File webrtc/base/event_tracer.cc (right):

https://codereview.webrtc.org/1457383002/diff/180001/webrtc/base/event_tracer...
webrtc/base/event_tracer.cc:94: TraceEvent{name, category_enabled, phase,
timestamp, 1, thread_id});
Are we using this syntax anywhere else?

https://codereview.webrtc.org/1457383002/diff/180001/webrtc/base/event_tracer...
webrtc/base/event_tracer.cc:97: void Log() {
can you add a link to some documentation about this format?
If we can have a regression test to make sure we're in sync with Chromium, then
that would be great.

https://codereview.webrtc.org/1457383002/diff/180001/webrtc/base/event_tracer...
webrtc/base/event_tracer.cc:140: enabled_ = true;
do we need both enabled_ and g_event_logging_active?

https://codereview.webrtc.org/1457383002/diff/180001/webrtc/base/event_tracer...
webrtc/base/event_tracer.cc:142:
rtc::AtomicOps::ReleaseStore(&g_event_logging_active, 1);
can we rather do this in a CompareExhange step and do it at the top?  If the
swap operation fails, [D]CHECK.
As is, the check above is racy.  Multiple threads could successfully pass it.

https://codereview.webrtc.org/1457383002/diff/180001/webrtc/base/event_tracer...
webrtc/base/event_tracer.cc:157:
rtc::AtomicOps::ReleaseStore(&g_event_logging_active, 0);
CompareExchange instead.

https://codereview.webrtc.org/1457383002/diff/180001/webrtc/base/event_tracer.h
File webrtc/base/event_tracer.h (right):

https://codereview.webrtc.org/1457383002/diff/180001/webrtc/base/event_tracer...
webrtc/base/event_tracer.h:71: static void SetupInternalTracer();
would it make sense to move these out of the class and into a namespace, e.g.
webrtc::internal?
or perhaps rtc::tracing::internal?

[commit-bot: I haz the power, 2015-11-24 17:57:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-24 17:57:34]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[pbos-webrtc, 2015-11-26 12:21:00]

feedback + move into rtc::tracing

[pbos-webrtc, 2015-11-26 12:21:07]

PTAL

https://codereview.webrtc.org/1457383002/diff/180001/webrtc/base/event_tracer.cc
File webrtc/base/event_tracer.cc (right):

https://codereview.webrtc.org/1457383002/diff/180001/webrtc/base/event_tracer...
webrtc/base/event_tracer.cc:94: TraceEvent{name, category_enabled, phase,
timestamp, 1, thread_id});
On 2015/11/24 16:27:09, tommi (webrtc) wrote:
> Are we using this syntax anywhere else?

I found like one instance of it in a test, removed TraceEvent in front of it
since that was superfluous. Are you fine with this?

https://codereview.webrtc.org/1457383002/diff/180001/webrtc/base/event_tracer...
webrtc/base/event_tracer.cc:97: void Log() {
On 2015/11/24 16:27:09, tommi (webrtc) wrote:
> can you add a link to some documentation about this format?
> If we can have a regression test to make sure we're in sync with Chromium,
then
> that would be great.

Added documentation link. The upstream repo even links to this google doc:
https://github.com/catapult-project/catapult/blob/master/tracing/README.md

https://codereview.webrtc.org/1457383002/diff/180001/webrtc/base/event_tracer...
webrtc/base/event_tracer.cc:140: enabled_ = true;
On 2015/11/24 16:27:09, tommi (webrtc) wrote:
> do we need both enabled_ and g_event_logging_active?

Yes, because g_event_logging_active is a fast path and best effort. The actual
synchronization of trace_events_ for instance happens based on enabled_ and
crit_ locking.

Checking g_event_logging by AtomicLoad is just a regular read instruction on
x86, so way faster than taking the lock and aborting when we're not logging.

https://codereview.webrtc.org/1457383002/diff/180001/webrtc/base/event_tracer...
webrtc/base/event_tracer.cc:142:
rtc::AtomicOps::ReleaseStore(&g_event_logging_active, 1);
On 2015/11/24 16:27:09, tommi (webrtc) wrote:
> can we rather do this in a CompareExhange step and do it at the top?  If the
> swap operation fails, [D]CHECK.
> As is, the check above is racy.  Multiple threads could successfully pass it.

Done.

https://codereview.webrtc.org/1457383002/diff/180001/webrtc/base/event_tracer...
webrtc/base/event_tracer.cc:157:
rtc::AtomicOps::ReleaseStore(&g_event_logging_active, 0);
On 2015/11/24 16:27:09, tommi (webrtc) wrote:
> CompareExchange instead.

Done.

[pbos-webrtc, 2015-11-26 16:52:37]

rebase

[tommi, 2015-11-27 22:07:26]

https://codereview.webrtc.org/1457383002/diff/220001/webrtc/base/event_tracer.cc
File webrtc/base/event_tracer.cc (right):

https://codereview.webrtc.org/1457383002/diff/220001/webrtc/base/event_tracer...
webrtc/base/event_tracer.cc:12: #include <inttypes.h>
Hm.. have you tried this on the win bots?

https://codereview.webrtc.org/1457383002/diff/220001/webrtc/base/event_tracer...
webrtc/base/event_tracer.cc:81: // TODO(pbos): Log metadata for all threads,
etc.
nit: add empty line above this one.

https://codereview.webrtc.org/1457383002/diff/220001/webrtc/base/event_tracer...
webrtc/base/event_tracer.cc:95: if (!enabled_)
Is this check necessary?  We've just checked that the global flag is set, right?

https://codereview.webrtc.org/1457383002/diff/220001/webrtc/base/event_tracer...
webrtc/base/event_tracer.cc:109: wakeup_event_.Wait(kLoggingIntervalMs);
let's check for the return value here.  If the event is signaled, we know we're
shutting down.

https://codereview.webrtc.org/1457383002/diff/220001/webrtc/base/event_tracer...
webrtc/base/event_tracer.cc:115: shutting_down = !enabled_;
and then this check isn't necessary

https://codereview.webrtc.org/1457383002/diff/220001/webrtc/base/event_tracer...
webrtc/base/event_tracer.cc:146: enabled_ = true;
I don't think we need this variable or this scope

https://codereview.webrtc.org/1457383002/diff/220001/webrtc/base/event_tracer...
webrtc/base/event_tracer.cc:160: rtc::CritScope lock(&crit_);
can lose this scope

https://codereview.webrtc.org/1457383002/diff/220001/webrtc/base/event_tracer...
webrtc/base/event_tracer.cc:168:
rtc::AtomicOps::CompareAndSwap(&g_event_logging_active, 1, 0));
if we want to allow calling Stop() when not running, we could check the return
value here and bail if we're not running.

https://codereview.webrtc.org/1457383002/diff/220001/webrtc/base/event_tracer...
webrtc/base/event_tracer.cc:202: const char* prefix_ptr = kDisabledTracePrefix;
nit: &kDisabledTracePrefix[0]

https://codereview.webrtc.org/1457383002/diff/220001/webrtc/base/event_tracer...
webrtc/base/event_tracer.cc:205: while (*prefix_ptr == *name_ptr) {
what if both strings are "foo"?  Won't we read past the '\0'?

https://codereview.webrtc.org/1457383002/diff/220001/webrtc/base/event_tracer...
webrtc/base/event_tracer.cc:229: thread_id);
nit: can just pass rtc::CurrentThreadId().  (and rtc_TimeMicros() too for that
matter)

https://codereview.webrtc.org/1457383002/diff/220001/webrtc/base/event_tracer...
webrtc/base/event_tracer.cc:238: g_event_logger = new EventLogger();
since there's no synchronization here, we should at least set this pointer
atomically.  As is, two threads could successfully get here without triggering
the checks.

https://codereview.webrtc.org/1457383002/diff/220001/webrtc/base/event_tracer...
webrtc/base/event_tracer.cc:243: RTC_DCHECK(g_event_logger);
not necessary since you dereference anyway

https://codereview.webrtc.org/1457383002/diff/220001/webrtc/base/event_tracer...
webrtc/base/event_tracer.cc:260: RTC_DCHECK(g_event_logger);
same here

https://codereview.webrtc.org/1457383002/diff/220001/webrtc/base/event_tracer...
webrtc/base/event_tracer.cc:271: g_event_logger = nullptr;
we need to free this object atomically

[pbos-webrtc, 2015-12-03 01:02:44]

rebase

[pbos-webrtc, 2015-12-03 15:16:20]

say nit again

[pbos-webrtc, 2015-12-03 15:22:36]

update tracer check expectation

[pbos-webrtc, 2015-12-03 15:23:05]

PTAL! Be extra careful w/r/t atomics. I believe this is "a pointer to a volatile
pointer to a T".

https://codereview.webrtc.org/1457383002/diff/220001/webrtc/base/event_tracer.cc
File webrtc/base/event_tracer.cc (right):

https://codereview.webrtc.org/1457383002/diff/220001/webrtc/base/event_tracer...
webrtc/base/event_tracer.cc:12: #include <inttypes.h>
On 2015/11/27 22:07:26, tommi (webrtc) wrote:
> Hm.. have you tried this on the win bots?

Yep!

https://codereview.webrtc.org/1457383002/diff/220001/webrtc/base/event_tracer...
webrtc/base/event_tracer.cc:81: // TODO(pbos): Log metadata for all threads,
etc.
On 2015/11/27 22:07:26, tommi (webrtc) wrote:
> nit: add empty line above this one.

Done.

https://codereview.webrtc.org/1457383002/diff/220001/webrtc/base/event_tracer...
webrtc/base/event_tracer.cc:95: if (!enabled_)
On 2015/11/27 22:07:26, tommi (webrtc) wrote:
> Is this check necessary?  We've just checked that the global flag is set,
right?

Done, adding stale events is OK, we clear them on Start now either way.

https://codereview.webrtc.org/1457383002/diff/220001/webrtc/base/event_tracer...
webrtc/base/event_tracer.cc:109: wakeup_event_.Wait(kLoggingIntervalMs);
On 2015/11/27 22:07:26, tommi (webrtc) wrote:
> let's check for the return value here.  If the event is signaled, we know
we're
> shutting down.

Done + renamed.

https://codereview.webrtc.org/1457383002/diff/220001/webrtc/base/event_tracer...
webrtc/base/event_tracer.cc:115: shutting_down = !enabled_;
On 2015/11/27 22:07:26, tommi (webrtc) wrote:
> and then this check isn't necessary

Done.

https://codereview.webrtc.org/1457383002/diff/220001/webrtc/base/event_tracer...
webrtc/base/event_tracer.cc:146: enabled_ = true;
On 2015/11/27 22:07:26, tommi (webrtc) wrote:
> I don't think we need this variable or this scope

Done for the variable, scope needed because we need to reset the vector to
remove old events.

After Start() returns logging should be active, so we need to block until events
can be added.

https://codereview.webrtc.org/1457383002/diff/220001/webrtc/base/event_tracer...
webrtc/base/event_tracer.cc:160: rtc::CritScope lock(&crit_);
On 2015/11/27 22:07:26, tommi (webrtc) wrote:
> can lose this scope

Done.

https://codereview.webrtc.org/1457383002/diff/220001/webrtc/base/event_tracer...
webrtc/base/event_tracer.cc:168:
rtc::AtomicOps::CompareAndSwap(&g_event_logging_active, 1, 0));
On 2015/11/27 22:07:26, tommi (webrtc) wrote:
> if we want to allow calling Stop() when not running, we could check the return
> value here and bail if we're not running.

Done.

https://codereview.webrtc.org/1457383002/diff/220001/webrtc/base/event_tracer...
webrtc/base/event_tracer.cc:202: const char* prefix_ptr = kDisabledTracePrefix;
On 2015/11/27 22:07:26, tommi (webrtc) wrote:
> nit: &kDisabledTracePrefix[0]

Done.

https://codereview.webrtc.org/1457383002/diff/220001/webrtc/base/event_tracer...
webrtc/base/event_tracer.cc:205: while (*prefix_ptr == *name_ptr) {
On 2015/11/27 22:07:26, tommi (webrtc) wrote:
> what if both strings are "foo"?  Won't we read past the '\0'?

Done.

https://codereview.webrtc.org/1457383002/diff/220001/webrtc/base/event_tracer...
webrtc/base/event_tracer.cc:229: thread_id);
On 2015/11/27 22:07:26, tommi (webrtc) wrote:
> nit: can just pass rtc::CurrentThreadId().  (and rtc_TimeMicros() too for that
> matter)

Genious! Done. Derp.

https://codereview.webrtc.org/1457383002/diff/220001/webrtc/base/event_tracer...
webrtc/base/event_tracer.cc:238: g_event_logger = new EventLogger();
On 2015/11/27 22:07:26, tommi (webrtc) wrote:
> since there's no synchronization here, we should at least set this pointer
> atomically.  As is, two threads could successfully get here without triggering
> the checks.

Done.

https://codereview.webrtc.org/1457383002/diff/220001/webrtc/base/event_tracer...
webrtc/base/event_tracer.cc:243: RTC_DCHECK(g_event_logger);
On 2015/11/27 22:07:26, tommi (webrtc) wrote:
> not necessary since you dereference anyway

Done.

https://codereview.webrtc.org/1457383002/diff/220001/webrtc/base/event_tracer...
webrtc/base/event_tracer.cc:260: RTC_DCHECK(g_event_logger);
On 2015/11/27 22:07:26, tommi (webrtc) wrote:
> same here

Done.

https://codereview.webrtc.org/1457383002/diff/220001/webrtc/base/event_tracer...
webrtc/base/event_tracer.cc:271: g_event_logger = nullptr;
On 2015/11/27 22:07:26, tommi (webrtc) wrote:
> we need to free this object atomically

Assuming you mean the CAS we talked about, done.

[pbos-webrtc, 2015-12-03 15:23:38]

The CQ bit was checked by pbos@webrtc.org

[pbos-webrtc, 2015-12-03 15:23:43]

The CQ bit was unchecked by pbos@webrtc.org

[pbos-webrtc, 2015-12-03 15:23:49]

The CQ bit was checked by pbos@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-03 15:24:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1457383002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1457383002/280001

[commit-bot: I haz the power, 2015-12-03 15:26:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-03 15:26:46]

Dry run: Try jobs failed on following builders:
  ios_arm64_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/6037)

[pbos-webrtc, 2015-12-03 15:28:40]

reinterpret cast windows

[pbos-webrtc, 2015-12-03 15:28:49]

The CQ bit was checked by pbos@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-03 15:28:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1457383002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1457383002/300001

[commit-bot: I haz the power, 2015-12-03 17:29:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-03 17:29:18]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[tommi, 2015-12-07 20:08:55]

lgtm, ship it!

[tommi, 2015-12-07 20:09:06]

The CQ bit was checked by tommi@webrtc.org

[commit-bot: I haz the power, 2015-12-07 20:09:21]

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

[commit-bot: I haz the power, 2015-12-07 20:24:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-07 20:24:05]

Try jobs failed on following builders:
  presubmit on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/2253)

[pbos-webrtc, 2015-12-07 22:10:09]

readd jitter_buffer trace

[commit-bot: I haz the power, 2015-12-07 22:17:29]

Description was changed from

==========
Implement standalone event tracing in AppRTCDemo.

Logs tracing events (TRACE_EVENT0 and friends) to storage in a format
compatible with chrome://tracing which can be used for performance
evaluation, finding lock contention and other sweet things). Tracing is
still basic and doesn't contain thread metadata or logging of tracing
arguments.

BUG=webrtc:5158
R=tommi@webrtc.org
==========

to

==========
Implement standalone event tracing in AppRTCDemo.

Logs tracing events (TRACE_EVENT0 and friends) to storage in a format
compatible with chrome://tracing which can be used for performance
evaluation, finding lock contention and other sweet things). Tracing is
still basic and doesn't contain thread metadata or logging of tracing
arguments.

BUG=webrtc:5158
R=tommi@webrtc.org

Committed: https://crrev.com/6f28cf0b951a9d41246f022f48a6cd035fad151d
Cr-Commit-Position: refs/heads/master@{#10921}
==========

[pbos-webrtc, 2015-12-07 22:17:29]

Description was changed from

==========
Implement standalone event tracing in AppRTCDemo.

Logs tracing events (TRACE_EVENT0 and friends) to storage in a format
compatible with chrome://tracing which can be used for performance
evaluation, finding lock contention and other sweet things). Tracing is
still basic and doesn't contain thread metadata or logging of tracing
arguments.

BUG=webrtc:5158
R=tommi@webrtc.org

Committed: https://crrev.com/6f28cf0b951a9d41246f022f48a6cd035fad151d
Cr-Commit-Position: refs/heads/master@{#10921}
==========

to

==========
Implement standalone event tracing in AppRTCDemo.

Logs tracing events (TRACE_EVENT0 and friends) to storage in a format
compatible with chrome://tracing which can be used for performance
evaluation, finding lock contention and other sweet things). Tracing is
still basic and doesn't contain thread metadata or logging of tracing
arguments.

BUG=webrtc:5158
R=tommi@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/6f28cf0b951a9d41246f022f4...
==========

[commit-bot: I haz the power, 2015-12-07 22:17:30]

Patchset 17 (id:??) landed as
https://crrev.com/6f28cf0b951a9d41246f022f48a6cd035fad151d
Cr-Commit-Position: refs/heads/master@{#10921}

[pbos-webrtc, 2015-12-07 22:17:31]

Committed patchset #17 (id:320001) manually as
6f28cf0b951a9d41246f022f48a6cd035fad151d (presubmit successful).

[Nico, 2015-12-10 21:20:54]

clang/win complains:

[2348/30810] CXX obj/third_party/webrtc/base/rtc_base_approved/event_tracer.obj
FAILED: ninja -t msvc -e environment.x64 --
../../third_party/llvm-build/Release+Asserts/bin/clang-cl.exe /nologo /showI
ncludes /FC @obj/third_party/webrtc/base/rtc_base_approved/event_tracer.obj.rsp
/c ../../third_party/webrtc/base/event_t
racer.cc /Foobj/third_party/webrtc/base/rtc_base_approved/event_tracer.obj
/Fdobj/third_party/webrtc/base/rtc_base_appro
ved_cc.pdb
../../third_party/webrtc/base/event_tracer.cc(124,46) :  error: format specifies
type 'int' but the argument has type 'r
tc::PlatformThreadId' (aka 'unsigned long') [-Werror,-Wformat]
                e.phase, e.timestamp, e.pid, e.tid);
                                             ^~~~~
1 error generated.
[2348/30810] CXX obj/courgette/courgette_lib/adjustment_method_2.obj

[Nico, 2015-12-10 21:21:54]

(possible fix: explicitly cast e.tid to int)

[pbos-webrtc, 2015-12-11 13:57:27]

On 2015/12/10 21:21:54, Nico wrote:
> (possible fix: explicitly cast e.tid to int)

Sorry about that, different fix up here:
https://codereview.webrtc.org/1522483002/

IIUC that should work for you as well, right?