Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years, 1 month ago by pbos-webrtc Modified:
5 years ago Reviewers:
tommi CC:
Andrew MacDonald, mflodman, niklas.enbom, peah-webrtc, qiang.lu, stefan-webrtc, tterriberry_mozilla.com, webrtc-reviews_webrtc.org, yujie_mao (webrtc) Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionImplement 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
Patch Set 1 #Patch Set 2 : add tracing shutdown #Patch Set 3 : fix test_main.cc #Patch Set 4 : fix race + test compile #Patch Set 5 : revert test_main.cc, it shouldn't be on for all tests either way #
Total comments: 20
Patch Set 6 : prefix checking + cl format #Patch Set 7 : as class #Patch Set 8 : things I said I'd done #Patch Set 9 : rebase onto PlatformThread #Patch Set 10 : fix some bugs after testing on device woo #
Total comments: 11
Patch Set 11 : feedback + move into rtc::tracing #Patch Set 12 : rebase #
Total comments: 30
Patch Set 13 : rebase #Patch Set 14 : say nit again #Patch Set 15 : update tracer check expectation #Patch Set 16 : reinterpret cast windows #Patch Set 17 : readd jitter_buffer trace, rebase #
Created: 5 years ago
Messages
Total messages: 64 (18 generated)
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.
add tracing shutdown
The CQ bit was checked by pbos@webrtc.org to run a CQ dry run
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 ==========
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
The CQ bit was unchecked by commit-bot@chromium.org
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)
fix test_main.cc
The CQ bit was checked by pbos@webrtc.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
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)
fix race + test compile
The CQ bit was checked by pbos@webrtc.org to run a CQ dry run
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
revert test_main.cc, it shouldn't be on for all tests either way
I'll stop slamming CQ now and make sure this runs locally, but PTAL as soon as you have time. :)
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
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!
prefix checking + cl format
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)
as class
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.
things I said I'd done
rebase onto PlatformThread
fix some bugs after testing on device woo
Please take another look on your favorite show. :)
The CQ bit was checked by pbos@webrtc.org to run a CQ dry run
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
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?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
feedback + move into rtc::tracing
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.
rebase
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
rebase
say nit again
update tracer check expectation
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.
The CQ bit was checked by pbos@webrtc.org
The CQ bit was unchecked by pbos@webrtc.org
The CQ bit was checked by pbos@webrtc.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
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)
reinterpret cast windows
The CQ bit was checked by pbos@webrtc.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
lgtm, ship it!
The CQ bit was checked by tommi@webrtc.org
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
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/2253)
readd jitter_buffer trace
Message was sent while issue was closed.
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} ==========
Message was sent while issue was closed.
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... ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/6f28cf0b951a9d41246f022f48a6cd035fad151d Cr-Commit-Position: refs/heads/master@{#10921}
Message was sent while issue was closed.
Committed patchset #17 (id:320001) manually as 6f28cf0b951a9d41246f022f48a6cd035fad151d (presubmit successful).
Message was sent while issue was closed.
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
Message was sent while issue was closed.
(possible fix: explicitly cast e.tid to int)
Message was sent while issue was closed.
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? |
