Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2159323002:
Add tracing AutoOpenCloseEvent. (Closed)
|
Created:
4 years, 5 months ago by alexandermont Modified:
4 years, 2 months ago Reviewers:
Primiano Tucci (use gerrit), benjhayden, oystein (OOO til 10th of July), nduca, charliea (OOO until 10-5), DaleCurtis, eakuefner, Nico, fmeawad CC:
chromium-reviews, feature-media-reviews_chromium.org, posciak+watch_chromium.org, telemetry-reviews_chromium.org, tracing+reviews_chromium.org, wfh+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd tracing AutoOpenCloseEvent.
The AutoOpenCloseEvent class handles events that start and stop. A user creates a AutoOpenCloseEvent object, then calls Start() on it to indicate that the event starts, and Stop() on it to indicate that the event stops. AutoOpenCloseEvent also handles the case where Start() was called before tracing started.
Committed: https://crrev.com/146c424c7d4fc43699d530675c9839833f0e8418
Cr-Commit-Position: refs/heads/master@{#425439}
Patch Set 1 #
Total comments: 12
Patch Set 2 : fix more includes #Patch Set 3 : Implement new design #
Total comments: 30
Patch Set 4 : code review changes #Patch Set 5 : code review changes #Patch Set 6 : format #Patch Set 7 : fix formatting #
Total comments: 4
Patch Set 8 : code review changes #
Total comments: 21
Patch Set 9 : code review changes #
Total comments: 20
Patch Set 10 : code review changes #
Total comments: 7
Patch Set 11 : updates #Patch Set 12 : implement new design #
Total comments: 29
Patch Set 13 : update design #
Total comments: 36
Patch Set 14 : code review changes #
Total comments: 8
Patch Set 15 : code review changes #
Total comments: 1
Patch Set 16 : rebase #Patch Set 17 : create the AutoOpenCloseEvent when you use it #
Total comments: 2
Patch Set 18 : fix formatting #
Total comments: 9
Patch Set 19 : fix multithreading #
Total comments: 7
Patch Set 20 : add forward declaration #
Messages
Total messages: 112 (32 generated)
Description was changed from ========== Add PersistentAsyncEvent (PROTOTYPE CODE; DO NOT SUBMIT) ========== to ========== Add PersistentAsyncEvent (PROTOTYPE CODE; DO NOT SUBMIT) CQ_INCLUDE_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq ==========
alexandermont@chromium.org changed reviewers: + benjhayden@chromium.org, charliea@chromium.org, eakuefner@chromium.org, nduca@chromium.org
caseq@chromium.org changed reviewers: + caseq@chromium.org
Is there anything that documents the intent and high-level design behind this?
charliea@chromium.org changed reviewers: - caseq@chromium.org
I think the spirit of this CL is spot-on, but the API exposed for this is just
too different from the rest of how tracing works. What we really need is a set
of macros that look like:
TRACE_EVENT_PERSISTENT_ASYNC_BEGIN1("media", "VideoFrameCompositor::Active",
video_id);
TRACE_EVENT_PERSISTENT_ASYNC_END1("media", "VideoFrameCompositor::Active",
video_id);
that mirror these:
https://cs.chromium.org/chromium/src/base/trace_event/common/trace_event_comm...
https://codereview.chromium.org/2159323002/diff/1/base/trace_event/persistent...
File base/trace_event/persistent_async_event.h (right):
https://codereview.chromium.org/2159323002/diff/1/base/trace_event/persistent...
base/trace_event/persistent_async_event.h:16: // Note: The categories and event
names must have application lifetime
Take a look at
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/timing/Pe...
to see how they use a non-constant trace event name
https://codereview.chromium.org/2159323002/diff/1/base/trace_event/vfc_persis...
File base/trace_event/vfc_persistent_async_event.cc (right):
https://codereview.chromium.org/2159323002/diff/1/base/trace_event/vfc_persis...
base/trace_event/vfc_persistent_async_event.cc:24: void
VFCPersistentAsyncEvent::RecordStartEventWithOffset(double offset) {
Unless we need something like this immediately, I'd say not to add it for now.
We should keep in mind that it might be necessary in the future though and make
it so that the API can be easily altered to allow this option.
primiano@chromium.org changed reviewers: + primiano@chromium.org
No idea about what this CL is intending to do, just one thing I noticed: if you plan to add observers and they are not indefinitely lived, consider using AsyncEnabledStateObserver, or you will end up very likely with a racy design. More info and context in crbug.com/610629
Is this CL still happening? charliea: I'm thinking about skyostil's BlameContext. It seems appropriate for objects like the VFC to explicitly own a PersistentAsyncEvent instead of building something like a meta-log to own it. https://codereview.chromium.org/2159323002/diff/1/base/trace_event/persistent... File base/trace_event/persistent_async_event.cc (right): https://codereview.chromium.org/2159323002/diff/1/base/trace_event/persistent... base/trace_event/persistent_async_event.cc:25: gettimeofday(&start_time_, 0); charliea can help you figure out what function to use to get the time. https://codereview.chromium.org/2159323002/diff/1/base/trace_event/persistent... base/trace_event/persistent_async_event.cc:47: if (active_) braces should go on the same line https://codereview.chromium.org/2159323002/diff/1/base/trace_event/persistent... base/trace_event/persistent_async_event.cc:64: } } // namespace trace_event https://codereview.chromium.org/2159323002/diff/1/base/trace_event/persistent... File base/trace_event/persistent_async_event.h (right): https://codereview.chromium.org/2159323002/diff/1/base/trace_event/persistent... base/trace_event/persistent_async_event.h:23: public: These should only be indented by 1 space. https://codereview.chromium.org/2159323002/diff/1/base/trace_event/persistent... base/trace_event/persistent_async_event.h:36: protected: only indent by 1 space https://codereview.chromium.org/2159323002/diff/1/base/trace_event/persistent... base/trace_event/persistent_async_event.h:43: } } // namespace trace_event https://codereview.chromium.org/2159323002/diff/1/base/trace_event/persistent... base/trace_event/persistent_async_event.h:44: } } // namespace base https://codereview.chromium.org/2159323002/diff/1/media/blink/video_frame_com... File media/blink/video_frame_compositor.cc (right): https://codereview.chromium.org/2159323002/diff/1/media/blink/video_frame_com... media/blink/video_frame_compositor.cc:48: std::stringstream s; Chrome has a handy StringPrintf library, but PersistentAsyncEvent should allow passing a const void* to its constructor to use as the id, and allow the trace macros to stringify it. https://codereview.chromium.org/2159323002/diff/1/media/blink/video_frame_com... File media/blink/video_frame_compositor.h (right): https://codereview.chromium.org/2159323002/diff/1/media/blink/video_frame_com... media/blink/video_frame_compositor.h:138: // Persistent async event You can remove this comment since it adds no information. https://codereview.chromium.org/2159323002/diff/1/tools/perf/benchmarks/batto... File tools/perf/benchmarks/battor.py (right): https://codereview.chromium.org/2159323002/diff/1/tools/perf/benchmarks/batto... tools/perf/benchmarks/battor.py:92: page_set = page_sets.Typical10MobilePageSet This doesn't look like part of this CL.
On 2016/08/10 at 18:30:00, benjhayden wrote: > Is this CL still happening? > > charliea: I'm thinking about skyostil's BlameContext. It seems appropriate for objects like the VFC to explicitly own a PersistentAsyncEvent instead of building something like a meta-log to own it. > > https://codereview.chromium.org/2159323002/diff/1/base/trace_event/persistent... > File base/trace_event/persistent_async_event.cc (right): > > https://codereview.chromium.org/2159323002/diff/1/base/trace_event/persistent... > base/trace_event/persistent_async_event.cc:25: gettimeofday(&start_time_, 0); > charliea can help you figure out what function to use to get the time. > > https://codereview.chromium.org/2159323002/diff/1/base/trace_event/persistent... > base/trace_event/persistent_async_event.cc:47: if (active_) > braces should go on the same line > > https://codereview.chromium.org/2159323002/diff/1/base/trace_event/persistent... > base/trace_event/persistent_async_event.cc:64: } > } // namespace trace_event > > https://codereview.chromium.org/2159323002/diff/1/base/trace_event/persistent... > File base/trace_event/persistent_async_event.h (right): > > https://codereview.chromium.org/2159323002/diff/1/base/trace_event/persistent... > base/trace_event/persistent_async_event.h:23: public: > These should only be indented by 1 space. > > https://codereview.chromium.org/2159323002/diff/1/base/trace_event/persistent... > base/trace_event/persistent_async_event.h:36: protected: > only indent by 1 space > > https://codereview.chromium.org/2159323002/diff/1/base/trace_event/persistent... > base/trace_event/persistent_async_event.h:43: } > } // namespace trace_event > > https://codereview.chromium.org/2159323002/diff/1/base/trace_event/persistent... > base/trace_event/persistent_async_event.h:44: } > } // namespace base > > https://codereview.chromium.org/2159323002/diff/1/media/blink/video_frame_com... > File media/blink/video_frame_compositor.cc (right): > > https://codereview.chromium.org/2159323002/diff/1/media/blink/video_frame_com... > media/blink/video_frame_compositor.cc:48: std::stringstream s; > Chrome has a handy StringPrintf library, but PersistentAsyncEvent should allow passing a const void* to its constructor to use as the id, and allow the trace macros to stringify it. > > https://codereview.chromium.org/2159323002/diff/1/media/blink/video_frame_com... > File media/blink/video_frame_compositor.h (right): > > https://codereview.chromium.org/2159323002/diff/1/media/blink/video_frame_com... > media/blink/video_frame_compositor.h:138: // Persistent async event > You can remove this comment since it adds no information. > > https://codereview.chromium.org/2159323002/diff/1/tools/perf/benchmarks/batto... > File tools/perf/benchmarks/battor.py (right): > > https://codereview.chromium.org/2159323002/diff/1/tools/perf/benchmarks/batto... > tools/perf/benchmarks/battor.py:92: page_set = page_sets.Typical10MobilePageSet > This doesn't look like part of this CL. The CL is probably still going to happen in some form, although it's "on hold" right now until we finalize the design for the PersistentAsyncEvent.
alexandermont@chromium.org changed reviewers: + agrieve@chromium.org, dalecurtis@chromium.org
Description was changed from ========== Add PersistentAsyncEvent (PROTOTYPE CODE; DO NOT SUBMIT) CQ_INCLUDE_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq ========== to ========== Add PersistentAsyncEvent CQ_INCLUDE_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq ==========
Thank you so much for taking this on! This basically looks about right to me, though I might defer if somebody else feels strongly that it should work differently. I just have a few nits and suggestions. https://codereview.chromium.org/2159323002/diff/40001/base/trace_event/persis... File base/trace_event/persistent_async_event.h (right): https://codereview.chromium.org/2159323002/diff/40001/base/trace_event/persis... base/trace_event/persistent_async_event.h:17: // Thus, we must makes subclasses that have hard-coded categories "must make" not "makes" https://codereview.chromium.org/2159323002/diff/40001/base/trace_event/persis... base/trace_event/persistent_async_event.h:29: virtual void RecordStartEvent(); The default implementations of these 3 methods don't seem useful. Would it be better to leave them pure virtual in order to ensure that subclasses don't forget to implement them with the right strings? Also, shouldn't these methods be protected so that clients can only call Start() and Stop()? https://codereview.chromium.org/2159323002/diff/40001/base/trace_event/persis... base/trace_event/persistent_async_event.h:42: } "Terminate namespaces with comments as shown in the given examples." per https://google.github.io/styleguide/cppguide.html#Namespaces https://codereview.chromium.org/2159323002/diff/40001/base/trace_event/vfc_pe... File base/trace_event/vfc_persistent_async_event.h (right): https://codereview.chromium.org/2159323002/diff/40001/base/trace_event/vfc_pe... base/trace_event/vfc_persistent_async_event.h:5: #ifndef BASE_TRACE_EVENT_VFC_PERSISTENT_ASYNC_H_ Any reason not to spell out the acronym "VideoFrameCompositorPersistentAsyncEvent" for clarity? Alternatively, you could use the event name "VideoPlaybackPersistentAsyncEvent". https://codereview.chromium.org/2159323002/diff/40001/base/trace_event/vfc_pe... base/trace_event/vfc_persistent_async_event.h:16: class BASE_EXPORT VFCPersistentAsyncEvent comment describing what this class is? https://codereview.chromium.org/2159323002/diff/40001/base/trace_event/vfc_pe... base/trace_event/vfc_persistent_async_event.h:18: public: 1 space indent per https://google.github.io/styleguide/cppguide.html#Class_Format https://codereview.chromium.org/2159323002/diff/40001/media/blink/video_frame... File media/blink/video_frame_compositor.cc (right): https://codereview.chromium.org/2159323002/diff/40001/media/blink/video_frame... media/blink/video_frame_compositor.cc:45: new base::trace_event::VFCPersistentAsyncEvent((size_t)(this_address)); Please use a C++ cast instead of C cast per style guide. https://google.github.io/styleguide/cppguide.html#Casting Or you could have the VFCPAE take const VFC*. https://codereview.chromium.org/2159323002/diff/40001/media/blink/video_frame... media/blink/video_frame_compositor.cc:46: base::trace_event::TraceLog::GetInstance()->AddEnabledStateObserver( Can you do this in the PersistentAsyncEvent constructor?
Could you modify the CL description to have a second paragraph that describes what a PersistentAsyncEvent is and why it's useful? Other than that, there are still a decent # of formatting problems that should be fixed with `git cl format`, so I'll hold off on further review until that's done. https://codereview.chromium.org/2159323002/diff/40001/base/trace_event/persis... File base/trace_event/persistent_async_event.cc (right): https://codereview.chromium.org/2159323002/diff/40001/base/trace_event/persis... base/trace_event/persistent_async_event.cc:11: // test Huh? https://codereview.chromium.org/2159323002/diff/40001/base/trace_event/persis... base/trace_event/persistent_async_event.cc:13: namespace base { As strange as it seems, I think there's normall a blank line after the start of a new namespace: https://cs.chromium.org/chromium/src/base/trace_event/trace_log.h?q=base/trac... https://codereview.chromium.org/2159323002/diff/40001/base/trace_event/persis... base/trace_event/persistent_async_event.cc:24: start_time_ = base::TimeTicks::Now().ToInternalValue(); Generally, if you ever find yourself using "ToInternalValue" of TimeTicks, something's gone awry. I think here, we should really be storing the time as a base::TimeTicks, not an int64_t. https://codereview.chromium.org/2159323002/diff/40001/base/trace_event/persis... File base/trace_event/persistent_async_event.h (right): https://codereview.chromium.org/2159323002/diff/40001/base/trace_event/persis... base/trace_event/persistent_async_event.h:16: // (strings or literals), as documented in trace_event_common.h. nit: aren't "string literals" a thing? "(i.e. literals)" might be more correct than "(strings or literals)", unless I'm misunderstanding something https://codereview.chromium.org/2159323002/diff/40001/base/trace_event/persis... base/trace_event/persistent_async_event.h:22: public: nit: I think the whitespace is off here. Would you mind running "git cl format" and reuploading? (Doing that before every upload is a good habit to get into.) https://codereview.chromium.org/2159323002/diff/40001/base/trace_event/persis... base/trace_event/persistent_async_event.h:36: I don't think we usually add a blank line before the first field. (see: https://cs.chromium.org/chromium/src/base/trace_event/trace_log.h?q=base/trac...) (`git cl format` might fix this) https://codereview.chromium.org/2159323002/diff/40001/base/trace_event/persis... base/trace_event/persistent_async_event.h:39: int64_t start_time_; Why are we using int64_t to represent time instead of base::TimeTicks?
Description was changed from ========== Add PersistentAsyncEvent CQ_INCLUDE_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq ========== to ========== Add PersistentAsyncEvent class. The PersistentAsyncEvent class handles events that start and stop. A user creates a PersistentAsyncEvent object, then calls Start() on it to indicate that the event starts, and Stop() on it to indicate that the event stops. PersistentAsyncEvent also handles the case where Start() was called before tracing started. CQ_INCLUDE_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq ==========
https://codereview.chromium.org/2159323002/diff/40001/base/trace_event/persis... File base/trace_event/persistent_async_event.cc (right): https://codereview.chromium.org/2159323002/diff/40001/base/trace_event/persis... base/trace_event/persistent_async_event.cc:11: // test On 2016/08/29 at 17:27:27, charliea wrote: > Huh? Deleted. I don't remember why i put this comment there https://codereview.chromium.org/2159323002/diff/40001/base/trace_event/persis... base/trace_event/persistent_async_event.cc:13: namespace base { On 2016/08/29 at 17:27:27, charliea wrote: > As strange as it seems, I think there's normall a blank line after the start of a new namespace: https://cs.chromium.org/chromium/src/base/trace_event/trace_log.h?q=base/trac... Done https://codereview.chromium.org/2159323002/diff/40001/base/trace_event/persis... base/trace_event/persistent_async_event.cc:24: start_time_ = base::TimeTicks::Now().ToInternalValue(); On 2016/08/29 at 17:27:27, charliea wrote: > Generally, if you ever find yourself using "ToInternalValue" of TimeTicks, something's gone awry. I think here, we should really be storing the time as a base::TimeTicks, not an int64_t. The TRACE_EVENT_ASYNC_BEGIN_WITH_TIMESTAMP0 macro takes an int64_t, not a base::TimeTicks. What should we be doing here? Should we be storing it as a base::TimeTicks in the PersistentAsyncEvent, and then calling ToInternalValue on it right before putting it in the macro? Or is there something else we should be doing? https://codereview.chromium.org/2159323002/diff/40001/base/trace_event/persis... File base/trace_event/persistent_async_event.h (right): https://codereview.chromium.org/2159323002/diff/40001/base/trace_event/persis... base/trace_event/persistent_async_event.h:16: // (strings or literals), as documented in trace_event_common.h. On 2016/08/29 at 17:27:27, charliea wrote: > nit: aren't "string literals" a thing? "(i.e. literals)" might be more correct than "(strings or literals)", unless I'm misunderstanding something Done https://codereview.chromium.org/2159323002/diff/40001/base/trace_event/persis... base/trace_event/persistent_async_event.h:17: // Thus, we must makes subclasses that have hard-coded categories On 2016/08/28 at 05:17:20, benjhayden wrote: > "must make" not "makes" Done https://codereview.chromium.org/2159323002/diff/40001/base/trace_event/persis... base/trace_event/persistent_async_event.h:22: public: On 2016/08/29 at 17:27:27, charliea wrote: > nit: I think the whitespace is off here. Would you mind running "git cl format" and reuploading? (Doing that before every upload is a good habit to get into.) Done https://codereview.chromium.org/2159323002/diff/40001/base/trace_event/persis... base/trace_event/persistent_async_event.h:29: virtual void RecordStartEvent(); On 2016/08/28 at 05:17:20, benjhayden wrote: > The default implementations of these 3 methods don't seem useful. Would it be better to leave them pure virtual in order to ensure that subclasses don't forget to implement them with the right strings? > > Also, shouldn't these methods be protected so that clients can only call Start() and Stop()? Done https://codereview.chromium.org/2159323002/diff/40001/base/trace_event/persis... base/trace_event/persistent_async_event.h:36: On 2016/08/29 at 17:27:27, charliea wrote: > I don't think we usually add a blank line before the first field. (see: https://cs.chromium.org/chromium/src/base/trace_event/trace_log.h?q=base/trac...) > > (`git cl format` might fix this) Done https://codereview.chromium.org/2159323002/diff/40001/base/trace_event/persis... base/trace_event/persistent_async_event.h:39: int64_t start_time_; On 2016/08/29 at 17:27:27, charliea wrote: > Why are we using int64_t to represent time instead of base::TimeTicks? See above. TRACE_EVENT_ASYNC_BEGIN_WITH_TIMESTAMP0 needs an int64_t. https://codereview.chromium.org/2159323002/diff/40001/base/trace_event/persis... base/trace_event/persistent_async_event.h:42: } On 2016/08/28 at 05:17:21, benjhayden wrote: > "Terminate namespaces with comments as shown in the given examples." per https://google.github.io/styleguide/cppguide.html#Namespaces Done https://codereview.chromium.org/2159323002/diff/40001/base/trace_event/vfc_pe... File base/trace_event/vfc_persistent_async_event.h (right): https://codereview.chromium.org/2159323002/diff/40001/base/trace_event/vfc_pe... base/trace_event/vfc_persistent_async_event.h:5: #ifndef BASE_TRACE_EVENT_VFC_PERSISTENT_ASYNC_H_ On 2016/08/28 at 05:17:21, benjhayden wrote: > Any reason not to spell out the acronym "VideoFrameCompositorPersistentAsyncEvent" for clarity? > Alternatively, you could use the event name "VideoPlaybackPersistentAsyncEvent". Done https://codereview.chromium.org/2159323002/diff/40001/base/trace_event/vfc_pe... base/trace_event/vfc_persistent_async_event.h:16: class BASE_EXPORT VFCPersistentAsyncEvent On 2016/08/28 at 05:17:21, benjhayden wrote: > comment describing what this class is? Done https://codereview.chromium.org/2159323002/diff/40001/base/trace_event/vfc_pe... base/trace_event/vfc_persistent_async_event.h:18: public: On 2016/08/28 at 05:17:21, benjhayden wrote: > 1 space indent per https://google.github.io/styleguide/cppguide.html#Class_Format Done https://codereview.chromium.org/2159323002/diff/40001/media/blink/video_frame... File media/blink/video_frame_compositor.cc (right): https://codereview.chromium.org/2159323002/diff/40001/media/blink/video_frame... media/blink/video_frame_compositor.cc:45: new base::trace_event::VFCPersistentAsyncEvent((size_t)(this_address)); On 2016/08/28 at 05:17:21, benjhayden wrote: > Please use a C++ cast instead of C cast per style guide. https://google.github.io/styleguide/cppguide.html#Casting > Or you could have the VFCPAE take const VFC*. Done https://codereview.chromium.org/2159323002/diff/40001/media/blink/video_frame... media/blink/video_frame_compositor.cc:46: base::trace_event::TraceLog::GetInstance()->AddEnabledStateObserver( On 2016/08/28 at 05:17:21, benjhayden wrote: > Can you do this in the PersistentAsyncEvent constructor? Done
https://codereview.chromium.org/2159323002/diff/120001/media/blink/video_fram... File media/blink/video_frame_compositor.cc (right): https://codereview.chromium.org/2159323002/diff/120001/media/blink/video_fram... media/blink/video_frame_compositor.cc:43: persistent_async_ = Move into constructor syntax. https://codereview.chromium.org/2159323002/diff/120001/media/blink/video_fram... File media/blink/video_frame_compositor.h (right): https://codereview.chromium.org/2159323002/diff/120001/media/blink/video_fram... media/blink/video_frame_compositor.h:140: base::trace_event::VideoPlaybackPersistentAsyncEvent* persistent_async_ = No header initialization since the rest of these are constructor initialized.
https://codereview.chromium.org/2159323002/diff/120001/media/blink/video_fram... File media/blink/video_frame_compositor.cc (right): https://codereview.chromium.org/2159323002/diff/120001/media/blink/video_fram... media/blink/video_frame_compositor.cc:43: persistent_async_ = On 2016/08/29 at 19:08:48, DaleCurtis wrote: > Move into constructor syntax. Done https://codereview.chromium.org/2159323002/diff/120001/media/blink/video_fram... File media/blink/video_frame_compositor.h (right): https://codereview.chromium.org/2159323002/diff/120001/media/blink/video_fram... media/blink/video_frame_compositor.h:140: base::trace_event::VideoPlaybackPersistentAsyncEvent* persistent_async_ = On 2016/08/29 at 19:08:48, DaleCurtis wrote: > No header initialization since the rest of these are constructor initialized. Done
https://codereview.chromium.org/2159323002/diff/140001/media/blink/video_fram... File media/blink/video_frame_compositor.h (right): https://codereview.chromium.org/2159323002/diff/140001/media/blink/video_fram... media/blink/video_frame_compositor.h:165: base::trace_event::VideoPlaybackPersistentAsyncEvent* persistent_async_; Seems like this should be an std::unique_ptr<> unless you're intending Stop() to self-delete.
https://codereview.chromium.org/2159323002/diff/140001/base/trace_event/persi... File base/trace_event/persistent_async_event.cc (right): https://codereview.chromium.org/2159323002/diff/140001/base/trace_event/persi... base/trace_event/persistent_async_event.cc:20: PersistentAsyncEvent::~PersistentAsyncEvent() {} Do you need to RemoveEnabledStateObserver() to prevent use-after-free? https://codereview.chromium.org/2159323002/diff/140001/base/trace_event/persi... base/trace_event/persistent_async_event.cc:40: // TODO(alexandermont): How do we call RecordStopEvent before log disabled? IIRC, primiano or oystein suggested trying to fix TraceLog::SetDisabledWhileLocked() to call OnTraceLogDisabled() before setting mode_ = DISABLED? If that would be more complicated than just moving a few lines of code, then it can wait for another CL. https://codereview.chromium.org/2159323002/diff/140001/base/trace_event/persi... File base/trace_event/persistent_async_event.h (right): https://codereview.chromium.org/2159323002/diff/140001/base/trace_event/persi... base/trace_event/persistent_async_event.h:38: void* id_; Fields should be private, with public/protected accessors as necessary. https://codereview.chromium.org/2159323002/diff/140001/base/trace_event/video... File base/trace_event/video_playback_pae.cc (right): https://codereview.chromium.org/2159323002/diff/140001/base/trace_event/video... base/trace_event/video_playback_pae.cc:8: #include "base/time/time.h" Is this include still needed? https://codereview.chromium.org/2159323002/diff/140001/base/trace_event/video... base/trace_event/video_playback_pae.cc:9: #include "base/trace_event/trace_event.h" Is this include still needed? https://codereview.chromium.org/2159323002/diff/140001/base/trace_event/video... File base/trace_event/video_playback_pae.h (right): https://codereview.chromium.org/2159323002/diff/140001/base/trace_event/video... base/trace_event/video_playback_pae.h:9: #include "base/time/time.h" Is this include still needed? https://codereview.chromium.org/2159323002/diff/140001/base/trace_event/video... base/trace_event/video_playback_pae.h:11: #include "base/trace_event/trace_event.h" Is this include still needed? https://codereview.chromium.org/2159323002/diff/140001/media/blink/video_fram... File media/blink/video_frame_compositor.cc (right): https://codereview.chromium.org/2159323002/diff/140001/media/blink/video_fram... media/blink/video_frame_compositor.cc:11: #include "base/trace_event/trace_event_impl.h" Is this include still needed? https://codereview.chromium.org/2159323002/diff/140001/media/blink/video_fram... media/blink/video_frame_compositor.cc:12: #include "base/trace_event/trace_log.h" Is this include still needed?
https://codereview.chromium.org/2159323002/diff/140001/base/trace_event/persi... File base/trace_event/persistent_async_event.cc (right): https://codereview.chromium.org/2159323002/diff/140001/base/trace_event/persi... base/trace_event/persistent_async_event.cc:20: PersistentAsyncEvent::~PersistentAsyncEvent() {} On 2016/08/30 at 04:17:23, benjhayden wrote: > Do you need to RemoveEnabledStateObserver() to prevent use-after-free? Done https://codereview.chromium.org/2159323002/diff/140001/base/trace_event/persi... base/trace_event/persistent_async_event.cc:40: // TODO(alexandermont): How do we call RecordStopEvent before log disabled? On 2016/08/30 at 04:17:23, benjhayden wrote: > IIRC, primiano or oystein suggested trying to fix TraceLog::SetDisabledWhileLocked() to call OnTraceLogDisabled() before setting mode_ = DISABLED? If that would be more complicated than just moving a few lines of code, then it can wait for another CL. Done https://codereview.chromium.org/2159323002/diff/140001/base/trace_event/persi... File base/trace_event/persistent_async_event.h (right): https://codereview.chromium.org/2159323002/diff/140001/base/trace_event/persi... base/trace_event/persistent_async_event.h:38: void* id_; On 2016/08/30 at 04:17:23, benjhayden wrote: > Fields should be private, with public/protected accessors as necessary. I'm not exactly sure what this means. Let's talk about it this afternoon https://codereview.chromium.org/2159323002/diff/140001/base/trace_event/video... File base/trace_event/video_playback_pae.cc (right): https://codereview.chromium.org/2159323002/diff/140001/base/trace_event/video... base/trace_event/video_playback_pae.cc:8: #include "base/time/time.h" On 2016/08/30 at 04:17:24, benjhayden wrote: > Is this include still needed? Removed https://codereview.chromium.org/2159323002/diff/140001/base/trace_event/video... base/trace_event/video_playback_pae.cc:9: #include "base/trace_event/trace_event.h" On 2016/08/30 at 04:17:23, benjhayden wrote: > Is this include still needed? This is still needed because TRACE_EVENT... macros are in this header file. But base/macros.h is not. https://codereview.chromium.org/2159323002/diff/140001/base/trace_event/video... File base/trace_event/video_playback_pae.h (right): https://codereview.chromium.org/2159323002/diff/140001/base/trace_event/video... base/trace_event/video_playback_pae.h:9: #include "base/time/time.h" On 2016/08/30 at 04:17:24, benjhayden wrote: > Is this include still needed? Removed https://codereview.chromium.org/2159323002/diff/140001/base/trace_event/video... base/trace_event/video_playback_pae.h:11: #include "base/trace_event/trace_event.h" On 2016/08/30 at 04:17:24, benjhayden wrote: > Is this include still needed? Removed https://codereview.chromium.org/2159323002/diff/140001/media/blink/video_fram... File media/blink/video_frame_compositor.cc (right): https://codereview.chromium.org/2159323002/diff/140001/media/blink/video_fram... media/blink/video_frame_compositor.cc:11: #include "base/trace_event/trace_event_impl.h" On 2016/08/30 at 04:17:24, benjhayden wrote: > Is this include still needed? Removed https://codereview.chromium.org/2159323002/diff/140001/media/blink/video_fram... media/blink/video_frame_compositor.cc:12: #include "base/trace_event/trace_log.h" On 2016/08/30 at 04:17:24, benjhayden wrote: > Is this include still needed? Removed https://codereview.chromium.org/2159323002/diff/140001/media/blink/video_fram... File media/blink/video_frame_compositor.h (right): https://codereview.chromium.org/2159323002/diff/140001/media/blink/video_fram... media/blink/video_frame_compositor.h:165: base::trace_event::VideoPlaybackPersistentAsyncEvent* persistent_async_; On 2016/08/29 at 21:43:15, DaleCurtis wrote: > Seems like this should be an std::unique_ptr<> unless you're intending Stop() to self-delete. Done
https://codereview.chromium.org/2159323002/diff/160001/media/blink/video_fram... File media/blink/video_frame_compositor.cc (left): https://codereview.chromium.org/2159323002/diff/160001/media/blink/video_fram... media/blink/video_frame_compositor.cc:108: TRACE_EVENT_ASYNC_BEGIN0("media,rail", "VideoPlayback", Do we really need a Start()/Stop() API or should this just construct an ASYNC event during VFC::Start() and destruct it during VFC::Stop()? The recording can be handled by the constructor and destructor respectively.
https://codereview.chromium.org/2159323002/diff/160001/media/blink/video_fram... File media/blink/video_frame_compositor.cc (left): https://codereview.chromium.org/2159323002/diff/160001/media/blink/video_fram... media/blink/video_frame_compositor.cc:108: TRACE_EVENT_ASYNC_BEGIN0("media,rail", "VideoPlayback", On 2016/08/30 at 19:09:49, DaleCurtis wrote: > Do we really need a Start()/Stop() API or should this just construct an ASYNC event during VFC::Start() and destruct it during VFC::Stop()? The recording can be handled by the constructor and destructor respectively. I don't really see an advantage of doing it the way you're suggesting. It's possible to start and stop an event multiple times - and sometimes this will happen, for instance if you're starting and stopping the same video multiple times. Also, destructors can get called when you didn't do anything that makes it obvious that the destructor was called (like calling delete) - if an object goes out of scope that could trigger a destructor call. (Similar things can happen with constructors as well - e.g. if the PersistentAsyncEvent is a member of the class, then the constructor will be called when the class constructor is called.) Having the recording happening in the constructor and destructor seems like it might make it less clear to the reader of the code exactly when recording starts and stops.
media/ lgtm, i defer to base/ owners for preference on api surface. https://codereview.chromium.org/2159323002/diff/160001/media/blink/video_fram... File media/blink/video_frame_compositor.cc (left): https://codereview.chromium.org/2159323002/diff/160001/media/blink/video_fram... media/blink/video_frame_compositor.cc:108: TRACE_EVENT_ASYNC_BEGIN0("media,rail", "VideoPlayback", On 2016/08/30 at 19:34:36, alexandermont wrote: > On 2016/08/30 at 19:09:49, DaleCurtis wrote: > > Do we really need a Start()/Stop() API or should this just construct an ASYNC event during VFC::Start() and destruct it during VFC::Stop()? The recording can be handled by the constructor and destructor respectively. > > I don't really see an advantage of doing it the way you're suggesting. It's possible to start and stop an event multiple times - and sometimes this will happen, for instance if you're starting and stopping the same video multiple times. Also, destructors can get called when you didn't do anything that makes it obvious that the destructor was called (like calling delete) - if an object goes out of scope that could trigger a destructor call. (Similar things can happen with constructors as well - e.g. if the PersistentAsyncEvent is a member of the class, then the constructor will be called when the class constructor is called.) Having the recording happening in the constructor and destructor seems like it might make it less clear to the reader of the code exactly when recording starts and stops. Generally I find it preferable to avoid unnecessary APIs for simplicity. We're all well versed with smart pointers, and even have things like ElapsedTimer, ScopedHistogramTimer, etc which perform in a similar vein. It's not media/ code so I don't feel strongly, but I encourage you to avoid unnecessary function calls for long term cleanliness. See a recent example of a similar cleanup I did: https://codereview.chromium.org/2076793002/
Dale is technically right, it would be possible to frame PAE as a smart pointer, and it would reduce PAE's public methods, but I think the convention in tracing is to be very explicit about when async events begin and end. Tracing does use smart pointers for synchronous thread slices. I think that smart pointers make more sense and are more typically used in synchronous contexts than asynchronous ones. https://codereview.chromium.org/2159323002/diff/140001/base/trace_event/persi... File base/trace_event/persistent_async_event.h (right): https://codereview.chromium.org/2159323002/diff/140001/base/trace_event/persi... base/trace_event/persistent_async_event.h:38: void* id_; On 2016/08/30 at 19:01:22, alexandermont wrote: > On 2016/08/30 at 04:17:23, benjhayden wrote: > > Fields should be private, with public/protected accessors as necessary. > > I'm not exactly sure what this means. Let's talk about it this afternoon https://google.github.io/styleguide/cppguide.html#Access_Control Data members should be private so that they are encapsulated by the class. https://en.wikipedia.org/wiki/Encapsulation_(computer_programming) If the inheritance hierarchy were much broader and deeper than this, then it would be very difficult to reason about data and control flow if any class in the chain could modify any field at any time, you would need to read every class in the hierarchy fully and thoroughly. Sorry I had to code today. If this is still unclear, then I can make time tomorrow to talk about it. https://codereview.chromium.org/2159323002/diff/160001/base/trace_event/persi... File base/trace_event/persistent_async_event.h (right): https://codereview.chromium.org/2159323002/diff/160001/base/trace_event/persi... base/trace_event/persistent_async_event.h:27: void Start(); Can you change Start to Begin and Stop to End to be consistent with the trace macros, both these public methods and the protected virtual methods? https://codereview.chromium.org/2159323002/diff/160001/media/blink/video_fram... File media/blink/video_frame_compositor.h (right): https://codereview.chromium.org/2159323002/diff/160001/media/blink/video_fram... media/blink/video_frame_compositor.h:164: // Persistent async event Please remove content-free comments.
Another reason that we want Start/Stop to be explicit methods is that we might want to add args to them, to allow VFC or other PAEs to log additional information with the async event.
https://codereview.chromium.org/2159323002/diff/160001/base/trace_event/persi... File base/trace_event/persistent_async_event.cc (right): https://codereview.chromium.org/2159323002/diff/160001/base/trace_event/persi... base/trace_event/persistent_async_event.cc:36: if (active_) { Looks like it's okay to leave off the brace for one-line conditionals: https://cs.chromium.org/search/?q=if.*%5C)+f:base&sq=package:chromium&type=cs https://codereview.chromium.org/2159323002/diff/160001/base/trace_event/persi... File base/trace_event/persistent_async_event.h (right): https://codereview.chromium.org/2159323002/diff/160001/base/trace_event/persi... base/trace_event/persistent_async_event.h:27: void Start(); On 2016/08/31 03:12:53, benjhayden wrote: > Can you change Start to Begin and Stop to End to be consistent with the trace > macros, both these public methods and the protected virtual methods? +1 https://codereview.chromium.org/2159323002/diff/160001/base/trace_event/persi... base/trace_event/persistent_async_event.h:40: int64_t start_time_; As you mentioned, I think this should be stored as a TimeTicks. Only convert it to int64_t when you call the actual macro. https://codereview.chromium.org/2159323002/diff/160001/base/trace_event/video... File base/trace_event/video_playback_pae.h (right): https://codereview.chromium.org/2159323002/diff/160001/base/trace_event/video... base/trace_event/video_playback_pae.h:15: // Class that provides a persistent async event for video playback. I think "Class that..." class headers are generally discouraged (even if some of them do exist). "A persistent async event for video playback." should be sufficient (even if it is a sentence fragment) https://codereview.chromium.org/2159323002/diff/160001/base/trace_event/video... base/trace_event/video_playback_pae.h:17: class BASE_EXPORT VideoPlaybackPersistentAsyncEvent +oysteine@chromium.org, fmeawad@chromium.org What are your thoughts on calling this "VideoPlaybackEvent" rather than the wordy "VideoPlaybackPersistentAsyncEvent"? Then we could have the filename match the class name better too (video_playback_event.h, not video_playback_pae.h)
https://codereview.chromium.org/2159323002/diff/160001/base/trace_event/persi... File base/trace_event/persistent_async_event.cc (right): https://codereview.chromium.org/2159323002/diff/160001/base/trace_event/persi... base/trace_event/persistent_async_event.cc:36: if (active_) { On 2016/08/31 at 17:30:38, charliea wrote: > Looks like it's okay to leave off the brace for one-line conditionals: > > https://cs.chromium.org/search/?q=if.*%5C)+f:base&sq=package:chromium&type=cs Done https://codereview.chromium.org/2159323002/diff/160001/base/trace_event/persi... File base/trace_event/persistent_async_event.h (right): https://codereview.chromium.org/2159323002/diff/160001/base/trace_event/persi... base/trace_event/persistent_async_event.h:27: void Start(); On 2016/08/31 at 17:30:38, charliea wrote: > On 2016/08/31 03:12:53, benjhayden wrote: > > Can you change Start to Begin and Stop to End to be consistent with the trace > > macros, both these public methods and the protected virtual methods? > > +1 Done https://codereview.chromium.org/2159323002/diff/160001/base/trace_event/persi... base/trace_event/persistent_async_event.h:40: int64_t start_time_; On 2016/08/31 at 17:30:38, charliea wrote: > As you mentioned, I think this should be stored as a TimeTicks. Only convert it to int64_t when you call the actual macro. Done https://codereview.chromium.org/2159323002/diff/160001/base/trace_event/video... File base/trace_event/video_playback_pae.h (right): https://codereview.chromium.org/2159323002/diff/160001/base/trace_event/video... base/trace_event/video_playback_pae.h:15: // Class that provides a persistent async event for video playback. On 2016/08/31 at 17:30:38, charliea wrote: > I think "Class that..." class headers are generally discouraged (even if some of them do exist). "A persistent async event for video playback." should be sufficient (even if it is a sentence fragment) Done https://codereview.chromium.org/2159323002/diff/160001/base/trace_event/video... base/trace_event/video_playback_pae.h:17: class BASE_EXPORT VideoPlaybackPersistentAsyncEvent On 2016/08/31 at 17:30:38, charliea wrote: > +oysteine@chromium.org, fmeawad@chromium.org > > What are your thoughts on calling this "VideoPlaybackEvent" rather than the wordy "VideoPlaybackPersistentAsyncEvent"? Then we could have the filename match the class name better too (video_playback_event.h, not video_playback_pae.h) I think that could be a good idea. Let's wait for confirmation from oystein or fmeawad before changing the name in this CL https://codereview.chromium.org/2159323002/diff/160001/media/blink/video_fram... File media/blink/video_frame_compositor.cc (left): https://codereview.chromium.org/2159323002/diff/160001/media/blink/video_fram... media/blink/video_frame_compositor.cc:108: TRACE_EVENT_ASYNC_BEGIN0("media,rail", "VideoPlayback", On 2016/08/30 at 19:42:31, DaleCurtis wrote: > On 2016/08/30 at 19:34:36, alexandermont wrote: > > On 2016/08/30 at 19:09:49, DaleCurtis wrote: > > > Do we really need a Start()/Stop() API or should this just construct an ASYNC event during VFC::Start() and destruct it during VFC::Stop()? The recording can be handled by the constructor and destructor respectively. > > > > I don't really see an advantage of doing it the way you're suggesting. It's possible to start and stop an event multiple times - and sometimes this will happen, for instance if you're starting and stopping the same video multiple times. Also, destructors can get called when you didn't do anything that makes it obvious that the destructor was called (like calling delete) - if an object goes out of scope that could trigger a destructor call. (Similar things can happen with constructors as well - e.g. if the PersistentAsyncEvent is a member of the class, then the constructor will be called when the class constructor is called.) Having the recording happening in the constructor and destructor seems like it might make it less clear to the reader of the code exactly when recording starts and stops. > > Generally I find it preferable to avoid unnecessary APIs for simplicity. We're all well versed with smart pointers, and even have things like ElapsedTimer, ScopedHistogramTimer, etc which perform in a similar vein. It's not media/ code so I don't feel strongly, but I encourage you to avoid unnecessary function calls for long term cleanliness. See a recent example of a similar cleanup I did: https://codereview.chromium.org/2076793002/ See discussion below in comments. Given that future subclasses may add arguments to the Start() and Stop() calls, we think it's best to leave them as specific calls. https://codereview.chromium.org/2159323002/diff/160001/media/blink/video_fram... File media/blink/video_frame_compositor.h (right): https://codereview.chromium.org/2159323002/diff/160001/media/blink/video_fram... media/blink/video_frame_compositor.h:164: // Persistent async event On 2016/08/31 at 03:12:53, benjhayden wrote: > Please remove content-free comments. Done
I am slowly catching up with reviews. What's the status of this? Should I review it? Is it still being discussed and should I wait?
On 2016/08/31 at 19:16:25, primiano wrote: > I am slowly catching up with reviews. What's the status of this? Should I review it? Is it still being discussed and should I wait? This should still be reviewed. (The email chain might say it's prototype code because that's what it was when I started this CL, but it's not prototype code anymore. We have discussed the design and this CL will be submitted once code review is finished.)
https://codereview.chromium.org/2159323002/diff/160001/base/trace_event/video... File base/trace_event/video_playback_pae.h (right): https://codereview.chromium.org/2159323002/diff/160001/base/trace_event/video... base/trace_event/video_playback_pae.h:17: class BASE_EXPORT VideoPlaybackPersistentAsyncEvent On 2016/08/31 18:31:59, alexandermont wrote: > On 2016/08/31 at 17:30:38, charliea wrote: > > mailto:+oysteine@chromium.org, mailto:fmeawad@chromium.org > > > > What are your thoughts on calling this "VideoPlaybackEvent" rather than the > wordy "VideoPlaybackPersistentAsyncEvent"? Then we could have the filename match > the class name better too (video_playback_event.h, not video_playback_pae.h) > > I think that could be a good idea. Let's wait for confirmation from oystein or > fmeawad before changing the name in this CL (Pinged them directly about this)
fmeawad@chromium.org changed reviewers: + fmeawad@chromium.org
Some comments, I will leave the naming question to oysteine :) https://codereview.chromium.org/2159323002/diff/180001/base/trace_event/persi... File base/trace_event/persistent_async_event.cc (right): https://codereview.chromium.org/2159323002/diff/180001/base/trace_event/persi... base/trace_event/persistent_async_event.cc:40: void PersistentAsyncEvent::OnTraceLogDisabled() { I think we decided that we are going to leave the auto-close for post processing and I believe that this will fail because we do not yet have onBeforeTraceLogDisabled? https://codereview.chromium.org/2159323002/diff/180001/base/trace_event/persi... File base/trace_event/persistent_async_event.h (right): https://codereview.chromium.org/2159323002/diff/180001/base/trace_event/persi... base/trace_event/persistent_async_event.h:41: bool active_; Should this be atomic, can OnTraceLogEnabled be called on a different thread from the one the object was created on?
Also, try to add a test if possible.
oysteine@chromium.org changed reviewers: + oysteine@chromium.org
https://codereview.chromium.org/2159323002/diff/160001/base/trace_event/video... File base/trace_event/video_playback_pae.h (right): https://codereview.chromium.org/2159323002/diff/160001/base/trace_event/video... base/trace_event/video_playback_pae.h:17: class BASE_EXPORT VideoPlaybackPersistentAsyncEvent On 2016/09/02 19:24:42, charliea wrote: > On 2016/08/31 18:31:59, alexandermont wrote: > > On 2016/08/31 at 17:30:38, charliea wrote: > > > mailto:+oysteine@chromium.org, mailto:fmeawad@chromium.org > > > > > > What are your thoughts on calling this "VideoPlaybackEvent" rather than the > > wordy "VideoPlaybackPersistentAsyncEvent"? Then we could have the filename > match > > the class name better too (video_playback_event.h, not video_playback_pae.h) > > > > I think that could be a good idea. Let's wait for confirmation from oystein or > > fmeawad before changing the name in this CL > > (Pinged them directly about this) Hrm, I don't think base/trace_event should reference or know about "video" or "media" in any way. Can you generalize this, and pass the event name/category in from somewhere else?
https://codereview.chromium.org/2159323002/diff/160001/base/trace_event/video... File base/trace_event/video_playback_pae.h (right): https://codereview.chromium.org/2159323002/diff/160001/base/trace_event/video... base/trace_event/video_playback_pae.h:17: class BASE_EXPORT VideoPlaybackPersistentAsyncEvent On 2016/09/02 at 19:55:00, oystein wrote: > On 2016/09/02 19:24:42, charliea wrote: > > On 2016/08/31 18:31:59, alexandermont wrote: > > > On 2016/08/31 at 17:30:38, charliea wrote: > > > > mailto:+oysteine@chromium.org, mailto:fmeawad@chromium.org > > > > > > > > What are your thoughts on calling this "VideoPlaybackEvent" rather than the > > > wordy "VideoPlaybackPersistentAsyncEvent"? Then we could have the filename > > match > > > the class name better too (video_playback_event.h, not video_playback_pae.h) > > > > > > I think that could be a good idea. Let's wait for confirmation from oystein or > > > fmeawad before changing the name in this CL > > > > (Pinged them directly about this) > > Hrm, I don't think base/trace_event should reference or know about "video" or "media" in any way. Can you generalize this, and pass the event name/category in from somewhere else? The base PersistentAsyncEvent is the generalization of this class. I think it would make more sense to put this class in media/blink/ next to its sole user instead of parameterizing the strings. See how FrameBlameContext lives in content/renderer/.
On 2016/09/02 at 19:43:47, fmeawad wrote: > Also, try to add a test if possible. +1 It'd be great to test that OnTraceLogEnabled() calls RecordBeginEventWithTimestamp() if active_, and that it doesn't when !active_. Is there any other specific behavior that should be tested?
https://codereview.chromium.org/2159323002/diff/180001/base/trace_event/persi... File base/trace_event/persistent_async_event.cc (right): https://codereview.chromium.org/2159323002/diff/180001/base/trace_event/persi... base/trace_event/persistent_async_event.cc:40: void PersistentAsyncEvent::OnTraceLogDisabled() { On 2016/09/02 at 19:43:25, fmeawad wrote: > I think we decided that we are going to leave the auto-close for post processing and I believe that this will fail because we do not yet have onBeforeTraceLogDisabled? Fadi, the latest patch changes TraceLog::SetDisabledWhileLocked() so that OnTraceLogDisabled is called before the TraceLog is disabled, so this should work IIUC, at least for synchronous observers though not async observers.
lgtm w/ nit, but fmeawad and oystein@ are the people that really need to be happy https://codereview.chromium.org/2159323002/diff/180001/base/trace_event/video... File base/trace_event/video_playback_pae.h (right): https://codereview.chromium.org/2159323002/diff/180001/base/trace_event/video... base/trace_event/video_playback_pae.h:17: nit: please delete extra line https://codereview.chromium.org/2159323002/diff/180001/media/blink/video_fram... File media/blink/video_frame_compositor.h (right): https://codereview.chromium.org/2159323002/diff/180001/media/blink/video_fram... media/blink/video_frame_compositor.h:165: persistent_async_; I think video_event_ might be a better name for this. persistent_async_ has to do with tracing implementation details, whereas video_event_ is what this thing actually is.
https://codereview.chromium.org/2159323002/diff/180001/base/trace_event/persi... File base/trace_event/persistent_async_event.h (right): https://codereview.chromium.org/2159323002/diff/180001/base/trace_event/persi... base/trace_event/persistent_async_event.h:41: bool active_; On 2016/09/02 at 19:43:25, fmeawad wrote: > Should this be atomic, can OnTraceLogEnabled be called on a different thread from the one the object was created on? If a PersistentAsyncEvent subclass is not on the same thread as the TraceLog, then it would need to use AsyncEnabledStateObserver instead of the EnabledStateObserver that PAE extends. So, yes, an AsyncPersistentAsyncEvent *would* need to make active_ atomic.
It looks like this CL would fix the tests that were broken by moving the `mode_=DISABLED`: https://codereview.chromium.org/2327333002/ So, wait for that CL to land, then you can make PAE use OnBeforeTraceLogDisabled instead of OnTraceLogDisabled, and move the `mode_=DISABLED` line back to where it was.
I am very late for the party, sorry for commenting only now. Let's see if I can be useful here. Might be easier having a quick VC, but in the meantime dumping my brain to kill timezone latency. So, overall my feeling is that this CL is dealing with the use case: we want to guarantee some events to be there and not miss them if tracing is started after the thing has happened (or stopped before). In this regard I am very positive about having an easy to use building block to support this. Happy to help to figure out a path to make this happen ASAP. I have two questions about the use case: 1) Do you really need a couple (begin,end) of ASYNC events here? If this was a duration event (which is *one* event that embeds start and duration) would it be ok for your use case? 2) It's not clear to me whether you want to guarantee the presence of begin/end events even if the ring buffer wraps? If I read correctly the code here you are *not* guaranteeing that the "begin" will show up in the trace, because it's emitted OnTraceLogEnabled and therefore, traffic on tracing, could push it out. Is this a best-effort approach? Implementation-wise: Honestly not really like too much the approach here. It seems that all this PAE gives is timed callbacks to invoke TRACE_EVENT macros. IMHO exposes too much of the internals (and weaknesses) of the current tracing design. I'd like the PersistentAsyncEvent to be as simple as possible to capture the use case we need, so that we can reshuffle the internals and clean it up easily in future, without exposing a public-facing change (which is always a pain in tracing land). More concretely, my proposal API wise, is: - make PersistentAsyncEvent simpler with no need of overriding it. - Have the ctor take a category name, event name (and optional id?) as arguments, e.g.: PersistentAsyncEvent(const char* category, const char* event_name, const void* id = nullptr); - Do the TRACE_EVENT_...ADD internally without bouncing that responsibility to the client. - Have just a public Begin(optional timestamp) and End(optional timestamp) public methods. This give us (tracing peeps) a lot of degree of freedom to redesign this in future in a way that still honors the contract. This building block becomes very simple to use. The major difference here is that we would NOT expose any timings. Least but not last. I am not too enthusiastic to introduce a high number of EnabledStateObservers, as right now they have a performance cost on tracing start/stop. They were designed to expose hooks to a limited number of tracing-related controllers (e.g., netlog, devtools, memory infra manager). There might be a better and simpler way to achieve all this using metadata events (forget about the "metadata" name, they just have a bad naming.). Let's discuss this option more later, I need to first understand the use case. In any case, happy to exploit EnabledStateObservers to make progress as short term solution, as long as it remains an internal tracing impl. detail and is not exposed outside.
https://codereview.chromium.org/2159323002/diff/180001/base/trace_event/video... File base/trace_event/video_playback_pae.h (right): https://codereview.chromium.org/2159323002/diff/180001/base/trace_event/video... base/trace_event/video_playback_pae.h:17: On 2016/09/08 at 18:26:34, charliea wrote: > nit: please delete extra line Done
https://codereview.chromium.org/2159323002/diff/220001/base/trace_event/persi... File base/trace_event/persistent_async_event.cc (right): https://codereview.chromium.org/2159323002/diff/220001/base/trace_event/persi... base/trace_event/persistent_async_event.cc:16: const char* category, const char* event_name): active_(false), active_(false) should go on the next line. https://codereview.chromium.org/2159323002/diff/220001/base/trace_event/persi... base/trace_event/persistent_async_event.cc:17: id_(static_cast<void*>(this)), I thought you were going to take the VFC* in the ctor as id. I suppose using the PAE* as the id would work ok, but in that case, it doesn't need to be a separate field. You can say static_cast<void*>(this) directly in the TRACE_EVENT_* macros to save a word of memory and a couple lines of code. https://codereview.chromium.org/2159323002/diff/220001/base/trace_event/persi... base/trace_event/persistent_async_event.cc:45: // TODO(alexandermont): How do we call RecordStopEvent before log disabled? Wait for this CL to land then use OnBeforeTraceLogDisabled. https://codereview.chromium.org/2327333002
Didn't manage to look at the new patchset yet. But FYI I just got cc-ed to crbug.com/646649, which is fun. As I was saying in our VC today that is precisely what I feared. EnabledStateObserver is a crazy footgun and we should rip it ASAP. That's why I don't want you to expose any callback. do you see now? Somebody shoot in their feet with these OnTraceLogEnabled callbacks few hours after our discussion :)
https://codereview.chromium.org/2159323002/diff/220001/base/trace_event/persi... File base/trace_event/persistent_async_event.cc (right): https://codereview.chromium.org/2159323002/diff/220001/base/trace_event/persi... base/trace_event/persistent_async_event.cc:17: id_(static_cast<void*>(this)), On 2016/09/14 01:03:33, benjhayden wrote: > I thought you were going to take the VFC* in the ctor as id. > I suppose using the PAE* as the id would work ok, but in that case, it doesn't > need to be a separate field. You can say static_cast<void*>(this) directly in > the TRACE_EVENT_* macros to save a word of memory and a couple lines of code. RIght. as this code is right now, this id_ field seems useless (unless you make it overridable in the ctor). https://codereview.chromium.org/2159323002/diff/220001/base/trace_event/persi... base/trace_event/persistent_async_event.cc:24: base::trace_event::TraceLog::GetInstance()->RemoveEnabledStateObserver(this); You cannot use the standard EnabledStateObserver for non-long-lived objects. More context in crbug.com/610629 . Switch to AsyncEnabledStateObserver, which will require a weakptr here. https://codereview.chromium.org/2159323002/diff/220001/base/trace_event/persi... base/trace_event/persistent_async_event.cc:28: TRACE_EVENT_COPY_ASYNC_BEGIN0(category_, event_name_, id_); Why _COPY_? Aren't the argument supposed to be long lived strings? (Add a comment there) https://codereview.chromium.org/2159323002/diff/220001/base/trace_event/persi... base/trace_event/persistent_async_event.cc:47: TRACE_EVENT_COPY_ASYNC_END0(category_, event_name_, id_); This one doesn't work, it's too late to add events here. Aren't async event auto-closed by the importer? > Wait for this CL to land then use OnBeforeTraceLogDisabled. https://codereview.chromium.org/2327333002 I am hoping that we don't need that CL. Another callback from tracing is going to be another footgun. https://codereview.chromium.org/2159323002/diff/220001/base/trace_event/persi... File base/trace_event/persistent_async_event.h (right): https://codereview.chromium.org/2159323002/diff/220001/base/trace_event/persi... base/trace_event/persistent_async_event.h:13: nit: remove this extra space https://codereview.chromium.org/2159323002/diff/220001/base/trace_event/persi... base/trace_event/persistent_async_event.h:17: : public trace_event::TraceLog::EnabledStateObserver { Can this object be created and destroyed on different threads? I hope not, in which case please add a ThreadChecker to this class to prevent this. https://codereview.chromium.org/2159323002/diff/220001/base/trace_event/persi... base/trace_event/persistent_async_event.h:25: void OnTraceLogEnabled() override; nit: add // EnabledStateObserver implementation Also you might probably want to override these in the private section, so you remove the temptation from clients to directly call the OnTraceLog methods https://codereview.chromium.org/2159323002/diff/220001/base/trace_event/persi... base/trace_event/persistent_async_event.h:29: bool active_; Active seems a redundant field here. If I read the code correctly active_ == (start_time_ != 0) https://codereview.chromium.org/2159323002/diff/220001/base/trace_event/persi... base/trace_event/persistent_async_event.h:31: const char* category_; const char* const category_; ditto for event_name_ https://codereview.chromium.org/2159323002/diff/220001/base/trace_event/persi... base/trace_event/persistent_async_event.h:34: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2159323002/diff/220001/base/trace_event/trace... File base/trace_event/trace_log.cc (right): https://codereview.chromium.org/2159323002/diff/220001/base/trace_event/trace... base/trace_event/trace_log.cc:870: mode_ = DISABLED; ???
https://codereview.chromium.org/2159323002/diff/220001/base/trace_event/persi... File base/trace_event/persistent_async_event.cc (right): https://codereview.chromium.org/2159323002/diff/220001/base/trace_event/persi... base/trace_event/persistent_async_event.cc:16: const char* category, const char* event_name): active_(false), On 2016/09/14 at 01:03:33, benjhayden wrote: > active_(false) should go on the next line. Done https://codereview.chromium.org/2159323002/diff/220001/base/trace_event/persi... base/trace_event/persistent_async_event.cc:17: id_(static_cast<void*>(this)), On 2016/09/14 at 09:38:53, Primiano Tucci wrote: > On 2016/09/14 01:03:33, benjhayden wrote: > > I thought you were going to take the VFC* in the ctor as id. > > I suppose using the PAE* as the id would work ok, but in that case, it doesn't > > need to be a separate field. You can say static_cast<void*>(this) directly in > > the TRACE_EVENT_* macros to save a word of memory and a couple lines of code. > > RIght. as this code is right now, this id_ field seems useless (unless you make it overridable in the ctor). Done https://codereview.chromium.org/2159323002/diff/220001/base/trace_event/persi... base/trace_event/persistent_async_event.cc:24: base::trace_event::TraceLog::GetInstance()->RemoveEnabledStateObserver(this); On 2016/09/14 at 09:38:53, Primiano Tucci wrote: > You cannot use the standard EnabledStateObserver for non-long-lived objects. More context in crbug.com/610629 . > Switch to AsyncEnabledStateObserver, which will require a weakptr here. Done https://codereview.chromium.org/2159323002/diff/220001/base/trace_event/persi... base/trace_event/persistent_async_event.cc:28: TRACE_EVENT_COPY_ASYNC_BEGIN0(category_, event_name_, id_); On 2016/09/14 at 09:38:53, Primiano Tucci wrote: > Why _COPY_? Aren't the argument supposed to be long lived strings? (Add a comment there) Done https://codereview.chromium.org/2159323002/diff/220001/base/trace_event/persi... base/trace_event/persistent_async_event.cc:47: TRACE_EVENT_COPY_ASYNC_END0(category_, event_name_, id_); On 2016/09/14 at 09:38:53, Primiano Tucci wrote: > This one doesn't work, it's too late to add events here. > Aren't async event auto-closed by the importer? > > > Wait for this CL to land then use OnBeforeTraceLogDisabled. > https://codereview.chromium.org/2327333002 > > I am hoping that we don't need that CL. Another callback from tracing is going to be another footgun. Will hold off until we decide whether we are in fact using that CL. https://codereview.chromium.org/2159323002/diff/220001/base/trace_event/persi... File base/trace_event/persistent_async_event.h (right): https://codereview.chromium.org/2159323002/diff/220001/base/trace_event/persi... base/trace_event/persistent_async_event.h:13: On 2016/09/14 at 09:38:54, Primiano Tucci wrote: > nit: remove this extra space Done https://codereview.chromium.org/2159323002/diff/220001/base/trace_event/persi... base/trace_event/persistent_async_event.h:17: : public trace_event::TraceLog::EnabledStateObserver { On 2016/09/14 at 09:38:54, Primiano Tucci wrote: > Can this object be created and destroyed on different threads? I hope not, in which case please add a ThreadChecker to this class to prevent this. Done https://codereview.chromium.org/2159323002/diff/220001/base/trace_event/persi... base/trace_event/persistent_async_event.h:25: void OnTraceLogEnabled() override; On 2016/09/14 at 09:38:54, Primiano Tucci wrote: > nit: add // EnabledStateObserver implementation > > Also you might probably want to override these in the private section, so you remove the temptation from clients to directly call the OnTraceLog methods Added comment. How could you put these in the private section? Don't these need to be public since the EnabledStateObserver calls them? https://codereview.chromium.org/2159323002/diff/220001/base/trace_event/persi... base/trace_event/persistent_async_event.h:29: bool active_; On 2016/09/14 at 09:38:54, Primiano Tucci wrote: > Active seems a redundant field here. If I read the code correctly active_ == (start_time_ != 0) Well, actually, start_time doesn't get initialized at the beginning. It only gets initialized after Start() is called and active_ is set. https://codereview.chromium.org/2159323002/diff/220001/base/trace_event/persi... base/trace_event/persistent_async_event.h:31: const char* category_; On 2016/09/14 at 09:38:54, Primiano Tucci wrote: > const char* const category_; > ditto for event_name_ I'll put that in there, but what does it do? https://codereview.chromium.org/2159323002/diff/220001/base/trace_event/persi... base/trace_event/persistent_async_event.h:34: }; On 2016/09/14 at 09:38:54, Primiano Tucci wrote: > DISALLOW_COPY_AND_ASSIGN Done https://codereview.chromium.org/2159323002/diff/220001/base/trace_event/trace... File base/trace_event/trace_log.cc (right): https://codereview.chromium.org/2159323002/diff/220001/base/trace_event/trace... base/trace_event/trace_log.cc:870: mode_ = DISABLED; On 2016/09/14 at 09:38:54, Primiano Tucci wrote: > ??? Reverted this change. Originally this was there because we were going to effectively change OnTraceLogDisabled to be called before the trace log is disabled; however it turned out that that made some unit tests fail. Now we're going to change it to use OnBeforeTraceLogDisabled (unless we're going to change it to something else...)
(I defer completely to the tracing OWNERS, primiano@ and oystein@, here)
Some final batch of comments, looks better now thanks. Final bikeshedding on the name (yay)?: 1) Now that you have the enum, I'd drop "Async" from the name, as this really conceptually can be used in future for other events. 2) Is Persistent the right name? Persisent might suggest that the event persists even if the ring buffer wraps over, which is not true here. What you want to convey here is the fact that the event is recorded even if tracing begins later. Don't have a strong preference atm (Oystein, ideas?), but persistent is misleading. Maybe "AlwaysRecordedEvent"? Happy to hear other suggestions. https://codereview.chromium.org/2159323002/diff/220001/base/trace_event/persi... File base/trace_event/persistent_async_event.h (right): https://codereview.chromium.org/2159323002/diff/220001/base/trace_event/persi... base/trace_event/persistent_async_event.h:25: void OnTraceLogEnabled() override; On 2016/09/14 20:14:57, alexandermont wrote: > On 2016/09/14 at 09:38:54, Primiano Tucci wrote: > > nit: add // EnabledStateObserver implementation > > > > Also you might probably want to override these in the private section, so you > remove the temptation from clients to directly call the OnTraceLog methods > > Added comment. How could you put these in the private section? Don't these need > to be public since the EnabledStateObserver calls them? The TraceLog calls the methods on the base class, where they are public. Making them private here would have still made it work, as they reach here by virtual dispatching and don't need to be really public. It's just a style and bit of paranoy thing here. Anyways, not sure what is the recommended style with this, so fine if you leave them where they are. Will start a thread on chromium-dev as I'm curious at this point :) https://codereview.chromium.org/2159323002/diff/220001/base/trace_event/persi... base/trace_event/persistent_async_event.h:29: bool active_; On 2016/09/14 20:14:57, alexandermont wrote: > On 2016/09/14 at 09:38:54, Primiano Tucci wrote: > > Active seems a redundant field here. If I read the code correctly active_ == > (start_time_ != 0) > > Well, actually, start_time doesn't get initialized at the beginning. It only > gets initialized after Start() is called and active_ is set. It does, implicitly. TimeTicks is a non-POD type and its ctor explicitly zero-initializes it. https://cs.chromium.org/chromium/src/base/time/time.h?rcl=0&l=706 active_ here seems a redundant state field. https://codereview.chromium.org/2159323002/diff/220001/base/trace_event/persi... base/trace_event/persistent_async_event.h:31: const char* category_; On 2016/09/14 20:14:57, alexandermont wrote: > On 2016/09/14 at 09:38:54, Primiano Tucci wrote: > > const char* const category_; > > ditto for event_name_ > > I'll put that in there, but what does it do? const char* foo -> foo is a pointer to const char. You cannot change the content of the pointed string but you can still change the value of the pointer itself. const char* const foo -> foo is a pointer to const char (as ABOVE) AND the pointer itself is const. Practically is just a paranoid documentation thing to say: we don't expect that the category_ changes after the constructor. On a more practical side, it may allows the compiler to make further optimizations, because it can rely on that value to be const forever. https://codereview.chromium.org/2159323002/diff/240001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2159323002/diff/240001/base/BUILD.gn#newcode1976 base/BUILD.gn:1976: "trace_event/persistent_async_event_unittest.cc", the gn file mentions this unittest, but it's not part of the cl :) https://codereview.chromium.org/2159323002/diff/240001/base/trace_event/persi... File base/trace_event/persistent_async_event.cc (right): https://codereview.chromium.org/2159323002/diff/240001/base/trace_event/persi... base/trace_event/persistent_async_event.cc:12: nit: remove this extra line https://codereview.chromium.org/2159323002/diff/240001/base/trace_event/persi... base/trace_event/persistent_async_event.cc:21: DCHECK(thread_checker_.CalledOnValidThread()); remove this line, as it is tautological. The threadchecker learns its "valid" thread on ctor. But this is the ctor so the thread where you are in can't possibly be anything other than the "valid" one :) https://codereview.chromium.org/2159323002/diff/240001/base/trace_event/persi... base/trace_event/persistent_async_event.cc:33: DCHECK(thread_checker_.CalledOnValidThread()); oh I didn't think about enforce begin/end to be also forced to be called on the right thread. Ok if works for your use case (in which case it's perfect as it is) If in future we need begin/end to be called on different thread we can make the start_time a base::subtle::atomicint and use a release-store here (and an acquire load in OnTraceLogEnabled) https://codereview.chromium.org/2159323002/diff/240001/base/trace_event/persi... base/trace_event/persistent_async_event.cc:34: // We need to use the COPY version of the macro because the category and nit: add a line between comments and the previous line (i.e. here between 33 and 34) https://codereview.chromium.org/2159323002/diff/240001/base/trace_event/persi... base/trace_event/persistent_async_event.cc:37: TRACE_EVENT_COPY_ASYNC_BEGIN0(category_, event_name_, I dupe my previou (unresolved) question: Why _COPY_ and not just RACE_EVENT_COPY_BEGIN0 ? The args here seem to be constant strings. https://codereview.chromium.org/2159323002/diff/240001/base/trace_event/persi... base/trace_event/persistent_async_event.cc:46: active_ = false; just clear start_Time_? https://codereview.chromium.org/2159323002/diff/240001/base/trace_event/persi... base/trace_event/persistent_async_event.cc:52: TRACE_EVENT_COPY_ASYNC_BEGIN_WITH_TIMESTAMP0( ditto about s/_COPY_// https://codereview.chromium.org/2159323002/diff/240001/base/trace_event/persi... base/trace_event/persistent_async_event.cc:54: start_time_.ToInternalValue()); I think here you want InMicroseconds() not ToInternalValue. As the name suggests you are not supposed to rely on the internal representation of timeticks. https://codereview.chromium.org/2159323002/diff/240001/base/trace_event/persi... File base/trace_event/persistent_async_event.h (right): https://codereview.chromium.org/2159323002/diff/240001/base/trace_event/persi... base/trace_event/persistent_async_event.h:11: #include "base/trace_event/trace_event.h" IWYU (include what you use): add an include base/memory/weak_ptr.h https://codereview.chromium.org/2159323002/diff/240001/base/trace_event/persi... base/trace_event/persistent_async_event.h:11: #include "base/trace_event/trace_event.h" don't think you need to include trace_event here anymore https://codereview.chromium.org/2159323002/diff/240001/base/trace_event/persi... base/trace_event/persistent_async_event.h:16: class BASE_EXPORT PersistentAsyncEvent Add a comment explaining what this class does and which problem solves. I think the thing you want to clarify is that if you call begin() and end() before tracing start you never see the event. but on the other end you are guaranteed that an event that is started before tracing will be there. https://codereview.chromium.org/2159323002/diff/240001/base/trace_event/persi... base/trace_event/persistent_async_event.h:17: : public trace_event::TraceLog::AsyncEnabledStateObserver { you don't need trace_event:: here as you are already in base::trace_event https://codereview.chromium.org/2159323002/diff/240001/base/trace_event/persi... base/trace_event/persistent_async_event.h:23: PersistentAsyncEvent(PersistentAsyncEvent::Type type, you don't need PersistentAsyncEvent:: here I believe, just Type https://codereview.chromium.org/2159323002/diff/240001/base/trace_event/persi... base/trace_event/persistent_async_event.h:23: PersistentAsyncEvent(PersistentAsyncEvent::Type type, Can you add a comment saying // As in the rest of the tracing macros, the const char* arguments here must be pointers to indefinitely lived strings. to remove the temptation to pass c_str() args https://codereview.chromium.org/2159323002/diff/240001/base/trace_event/persi... base/trace_event/persistent_async_event.h:30: // EnabledStateObserver implementation nit, +Async. This is now extending AsyncEnabledStateObserver (It was a trap, I first asked you to add a comment here and then to use AsyncEnabledStateObserver :P) https://codereview.chromium.org/2159323002/diff/240001/base/trace_event/persi... base/trace_event/persistent_async_event.h:44: DISALLOW_COPY_AND_ASSIGN(PersistentAsyncEvent); nit: add a blankline between fields and DISALLOW... https://codereview.chromium.org/2159323002/diff/240001/base/trace_event/trace... File base/trace_event/trace_event.gypi (right): https://codereview.chromium.org/2159323002/diff/240001/base/trace_event/trace... base/trace_event/trace_event.gypi:84: 'trace_event/persistent_async_event_unittest.cc', FYI gyp is deprecated and there is no need to maintain gyp files anymore. https://codereview.chromium.org/2159323002/diff/240001/media/blink/video_fram... File media/blink/video_frame_compositor.cc (right): https://codereview.chromium.org/2159323002/diff/240001/media/blink/video_fram... media/blink/video_frame_compositor.cc:41: base::trace_event::PersistentAsyncEvent::Type::ASYNC, Type is an enum, not a C++11 enum class. I'm fine both ways. So either you make Type an "enum class", or if you keep an enum the "::Type" part here should go away (i'm surprised this compiles. Does it?) https://codereview.chromium.org/2159323002/diff/240001/media/blink/video_fram... File media/blink/video_frame_compositor.h (right): https://codereview.chromium.org/2159323002/diff/240001/media/blink/video_fram... media/blink/video_frame_compositor.h:164: std::unique_ptr<base::trace_event::PersistentAsyncEvent> persistent_async_; since this seems to be initialized only in the ctor, why don't you just make it a field object (i.e. drop the unique_ptr)? Not sure what the unique_ptr is buying here, other than an extra malloc and free pair.
Code updated. Note that the importer does *not* appear to perform auto-closing. When I build this in Chrome and try it, it correctly opens the trace event if video is started before tracing starts, but no event is visible in the trace if video ends after tracing ends. https://codereview.chromium.org/2159323002/diff/240001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2159323002/diff/240001/base/BUILD.gn#newcode1976 base/BUILD.gn:1976: "trace_event/persistent_async_event_unittest.cc", On 2016/09/15 at 10:08:29, Primiano Tucci wrote: > the gn file mentions this unittest, but it's not part of the cl :) Removed https://codereview.chromium.org/2159323002/diff/240001/base/trace_event/persi... File base/trace_event/persistent_async_event.cc (right): https://codereview.chromium.org/2159323002/diff/240001/base/trace_event/persi... base/trace_event/persistent_async_event.cc:12: On 2016/09/15 at 10:08:29, Primiano Tucci wrote: > nit: remove this extra line Done https://codereview.chromium.org/2159323002/diff/240001/base/trace_event/persi... base/trace_event/persistent_async_event.cc:21: DCHECK(thread_checker_.CalledOnValidThread()); On 2016/09/15 at 10:08:29, Primiano Tucci wrote: > remove this line, as it is tautological. > The threadchecker learns its "valid" thread on ctor. > But this is the ctor so the thread where you are in can't possibly be anything other than the "valid" one :) Done https://codereview.chromium.org/2159323002/diff/240001/base/trace_event/persi... base/trace_event/persistent_async_event.cc:33: DCHECK(thread_checker_.CalledOnValidThread()); On 2016/09/15 at 10:08:29, Primiano Tucci wrote: > oh I didn't think about enforce begin/end to be also forced to be called on the right thread. Ok if works for your use case (in which case it's perfect as it is) > If in future we need begin/end to be called on different thread we can make the start_time a base::subtle::atomicint and use a release-store here (and an acquire load in OnTraceLogEnabled) okay https://codereview.chromium.org/2159323002/diff/240001/base/trace_event/persi... base/trace_event/persistent_async_event.cc:37: TRACE_EVENT_COPY_ASYNC_BEGIN0(category_, event_name_, On 2016/09/15 at 10:08:29, Primiano Tucci wrote: > I dupe my previou (unresolved) question: > Why _COPY_ and not just RACE_EVENT_COPY_BEGIN0 ? > The args here seem to be constant strings. Fixed https://codereview.chromium.org/2159323002/diff/240001/base/trace_event/persi... base/trace_event/persistent_async_event.cc:46: active_ = false; On 2016/09/15 at 10:08:29, Primiano Tucci wrote: > just clear start_Time_? Done https://codereview.chromium.org/2159323002/diff/240001/base/trace_event/persi... base/trace_event/persistent_async_event.cc:54: start_time_.ToInternalValue()); On 2016/09/15 at 10:08:29, Primiano Tucci wrote: > I think here you want InMicroseconds() not ToInternalValue. > As the name suggests you are not supposed to rely on the internal representation of timeticks. This is a TimeTicks value, not a TimeDelta value. Only TimeDelta has the InMicroseconds() function; TimeTicks does not. Am I supposed to convert the TimeTicks to a TimeDelta somehow? https://codereview.chromium.org/2159323002/diff/240001/base/trace_event/persi... File base/trace_event/persistent_async_event.h (right): https://codereview.chromium.org/2159323002/diff/240001/base/trace_event/persi... base/trace_event/persistent_async_event.h:11: #include "base/trace_event/trace_event.h" On 2016/09/15 at 10:08:29, Primiano Tucci wrote: > IWYU (include what you use): add an include base/memory/weak_ptr.h Done. But you do still need to include trace_event: it gives compiler errors otherwise. https://codereview.chromium.org/2159323002/diff/240001/base/trace_event/persi... base/trace_event/persistent_async_event.h:16: class BASE_EXPORT PersistentAsyncEvent On 2016/09/15 at 10:08:29, Primiano Tucci wrote: > Add a comment explaining what this class does and which problem solves. > I think the thing you want to clarify is that if you call begin() and end() before tracing start you never see the event. but on the other end you are guaranteed that an event that is started before tracing will be there. Done https://codereview.chromium.org/2159323002/diff/240001/base/trace_event/persi... base/trace_event/persistent_async_event.h:17: : public trace_event::TraceLog::AsyncEnabledStateObserver { On 2016/09/15 at 10:08:30, Primiano Tucci wrote: > you don't need trace_event:: here as you are already in base::trace_event Done https://codereview.chromium.org/2159323002/diff/240001/base/trace_event/persi... base/trace_event/persistent_async_event.h:23: PersistentAsyncEvent(PersistentAsyncEvent::Type type, On 2016/09/15 at 10:08:30, Primiano Tucci wrote: > Can you add a comment saying > // As in the rest of the tracing macros, the const char* arguments here must be pointers to indefinitely lived strings. > > to remove the temptation to pass c_str() args Done https://codereview.chromium.org/2159323002/diff/240001/base/trace_event/persi... base/trace_event/persistent_async_event.h:30: // EnabledStateObserver implementation On 2016/09/15 at 10:08:30, Primiano Tucci wrote: > nit, +Async. This is now extending AsyncEnabledStateObserver > (It was a trap, I first asked you to add a comment here and then to use AsyncEnabledStateObserver :P) Done https://codereview.chromium.org/2159323002/diff/240001/base/trace_event/persi... base/trace_event/persistent_async_event.h:44: DISALLOW_COPY_AND_ASSIGN(PersistentAsyncEvent); On 2016/09/15 at 10:08:30, Primiano Tucci wrote: > nit: add a blankline between fields and DISALLOW... Done https://codereview.chromium.org/2159323002/diff/240001/base/trace_event/trace... File base/trace_event/trace_event.gypi (right): https://codereview.chromium.org/2159323002/diff/240001/base/trace_event/trace... base/trace_event/trace_event.gypi:84: 'trace_event/persistent_async_event_unittest.cc', On 2016/09/15 at 10:08:30, Primiano Tucci wrote: > FYI gyp is deprecated and there is no need to maintain gyp files anymore. Reverted https://codereview.chromium.org/2159323002/diff/240001/media/blink/video_fram... File media/blink/video_frame_compositor.h (right): https://codereview.chromium.org/2159323002/diff/240001/media/blink/video_fram... media/blink/video_frame_compositor.h:164: std::unique_ptr<base::trace_event::PersistentAsyncEvent> persistent_async_; On 2016/09/15 at 10:08:30, Primiano Tucci wrote: > since this seems to be initialized only in the ctor, why don't you just make it a field object (i.e. drop the unique_ptr)? > Not sure what the unique_ptr is buying here, other than an extra malloc and free pair. Done
Ok I like AutoOpenCloseEvent The fact that they don't auto-close is *really* unfortunate. Mind checking with somebody who knows TraceViewer (fmeawad, charliea) if that is WAI and if that can be fixed? Otherwise sounds like we'll have to do that onTraceLogBeforeDisabled thing that I really hate :( https://codereview.chromium.org/2159323002/diff/240001/base/trace_event/persi... File base/trace_event/persistent_async_event.cc (right): https://codereview.chromium.org/2159323002/diff/240001/base/trace_event/persi... base/trace_event/persistent_async_event.cc:54: start_time_.ToInternalValue()); On 2016/09/15 21:05:12, alexandermont wrote: > On 2016/09/15 at 10:08:29, Primiano Tucci wrote: > > I think here you want InMicroseconds() not ToInternalValue. > > As the name suggests you are not supposed to rely on the internal > representation of timeticks. > > This is a TimeTicks value, not a TimeDelta value. Only TimeDelta has the > InMicroseconds() function; TimeTicks does not. Am I supposed to convert the > TimeTicks to a TimeDelta somehow? ah right, my bad. do whatever we do for the other events (no idea where we take the time from) and be consistent with that.
anyways, this as-is LGTM. but we have to figure out how to deal with closing events.
lgtm w/minors https://codereview.chromium.org/2159323002/diff/260001/base/trace_event/auto_... File base/trace_event/auto_open_close_event.cc (right): https://codereview.chromium.org/2159323002/diff/260001/base/trace_event/auto_... base/trace_event/auto_open_close_event.cc:31: // We need to use the COPY version of the macro because the category and Outdated comment. https://codereview.chromium.org/2159323002/diff/260001/base/trace_event/auto_... base/trace_event/auto_open_close_event.cc:34: TRACE_EVENT_ASYNC_BEGIN0(category_, event_name_, Maybe use TRACE_EVENT_ASYNC_BEGIN_WITH_TIMESTAMP0 as well here, since you're reading the timestamp anyway? https://codereview.chromium.org/2159323002/diff/260001/base/trace_event/auto_... File base/trace_event/auto_open_close_event.h (right): https://codereview.chromium.org/2159323002/diff/260001/base/trace_event/auto_... base/trace_event/auto_open_close_event.h:29: enum Type { This seems unused? https://codereview.chromium.org/2159323002/diff/260001/base/trace_event/auto_... base/trace_event/auto_open_close_event.h:47: // the OnBeforeTraceLogDisabled CL lands. Should this TODO be here, if we don't think there's an actual need for auto-close? And I agree there shouldn't be, the trace-viewer should handle it and if it doesn't, that sounds like a bug. The trace-viewer shouldn't silently ignore async events that only have a begin.
https://codereview.chromium.org/2159323002/diff/260001/base/trace_event/auto_... File base/trace_event/auto_open_close_event.cc (right): https://codereview.chromium.org/2159323002/diff/260001/base/trace_event/auto_... base/trace_event/auto_open_close_event.cc:31: // We need to use the COPY version of the macro because the category and On 2016/09/16 at 22:22:16, oystein wrote: > Outdated comment. Removed https://codereview.chromium.org/2159323002/diff/260001/base/trace_event/auto_... base/trace_event/auto_open_close_event.cc:34: TRACE_EVENT_ASYNC_BEGIN0(category_, event_name_, On 2016/09/16 at 22:22:16, oystein wrote: > Maybe use TRACE_EVENT_ASYNC_BEGIN_WITH_TIMESTAMP0 as well here, since you're reading the timestamp anyway? Done https://codereview.chromium.org/2159323002/diff/260001/base/trace_event/auto_... File base/trace_event/auto_open_close_event.h (right): https://codereview.chromium.org/2159323002/diff/260001/base/trace_event/auto_... base/trace_event/auto_open_close_event.h:29: enum Type { On 2016/09/16 at 22:22:16, oystein wrote: > This seems unused? Our plan is to eventually add additional event types to here. We wanted to set up the API to allow that so we don't change the API when we do that. https://codereview.chromium.org/2159323002/diff/260001/base/trace_event/auto_... base/trace_event/auto_open_close_event.h:47: // the OnBeforeTraceLogDisabled CL lands. On 2016/09/16 at 22:22:16, oystein wrote: > Should this TODO be here, if we don't think there's an actual need for auto-close? And I agree there shouldn't be, the trace-viewer should handle it and if it doesn't, that sounds like a bug. The trace-viewer shouldn't silently ignore async events that only have a begin. Removed
The CQ bit was checked by alexandermont@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org, charliea@chromium.org, primiano@chromium.org, oysteine@chromium.org Link to the patchset: https://codereview.chromium.org/2159323002/#ps280001 (title: "code review changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Your CL was about to rely on recently removed CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is no longer supported:
tryserver.chromium.perf
For more details, see http://crbug.com/617627.
Description was changed from ========== Add PersistentAsyncEvent class. The PersistentAsyncEvent class handles events that start and stop. A user creates a PersistentAsyncEvent object, then calls Start() on it to indicate that the event starts, and Stop() on it to indicate that the event stops. PersistentAsyncEvent also handles the case where Start() was called before tracing started. CQ_INCLUDE_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq ========== to ========== Add PersistentAsyncEvent class. The PersistentAsyncEvent class handles events that start and stop. A user creates a PersistentAsyncEvent object, then calls Start() on it to indicate that the event starts, and Stop() on it to indicate that the event stops. PersistentAsyncEvent also handles the case where Start() was called before tracing started. ==========
The CQ bit was checked by alexandermont@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2016/09/19 at 20:48:12, commit-bot wrote: > Try jobs failed on following builders: > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) It says missing base/BUILD.gn OWNER. Added some OWNERs for this file.
alexandermont@chromium.org changed reviewers: + nyquist@chromium.org
primiano@chromium.org changed reviewers: + thakis@chromium.org - agrieve@chromium.org, nyquist@chromium.org
+thakis for 2 lines base/BUILD.gn OWNERS stamp. - agrieve, nyquist as the base/OWNERS says "For Android-specific changes" which is not the case here. https://codereview.chromium.org/2159323002/diff/280001/base/trace_event/auto_... File base/trace_event/auto_open_close_event.h (right): https://codereview.chromium.org/2159323002/diff/280001/base/trace_event/auto_... base/trace_event/auto_open_close_event.h:17: /** Nit: in general I see always // style comments. The style guide says you can use both as long you are consistent, which is not the case here :) More importantly all the code in //base seems to use //, so let's just stick with that.
base/BUILD.gn lgtm
The CQ bit was checked by alexandermont@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, oysteine@chromium.org, charliea@chromium.org, dalecurtis@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2159323002/#ps300001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by alexandermont@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by alexandermont@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Add PersistentAsyncEvent class. The PersistentAsyncEvent class handles events that start and stop. A user creates a PersistentAsyncEvent object, then calls Start() on it to indicate that the event starts, and Stop() on it to indicate that the event stops. PersistentAsyncEvent also handles the case where Start() was called before tracing started. ========== to ========== Add tracing AutoOpenCloseEvent. The AutoOpenCloseEvent class handles events that start and stop. A user creates a AutoOpenCloseEvent object, then calls Start() on it to indicate that the event starts, and Stop() on it to indicate that the event stops. AutoOpenCloseEvent also handles the case where Start() was called before tracing started. ==========
The CQ bit was checked by alexandermont@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, oysteine@chromium.org, charliea@chromium.org, dalecurtis@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2159323002/#ps320001 (title: "create the AutoOpenCloseEvent when you use it")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2159323002/diff/320001/media/blink/video_fram... File media/blink/video_frame_compositor.cc (right): https://codereview.chromium.org/2159323002/diff/320001/media/blink/video_fram... media/blink/video_frame_compositor.cc:45: DCHECK(compositor_task_runner_->BelongsToCurrentThread()); You're deleting auto_open_close_ on the compositor_task_runner_ thread. That looks right to me. https://codereview.chromium.org/2159323002/diff/320001/media/blink/video_fram... media/blink/video_frame_compositor.cc:112: if (auto_open_close_ == nullptr) { It looks like you're creating this and calling Begin and End on the media thread, so move this code to OnRendererStateUpdate so that it runs on the compositor_task_runner_ thread.
https://codereview.chromium.org/2159323002/diff/340001/media/blink/video_fram... File media/blink/video_frame_compositor.cc (right): https://codereview.chromium.org/2159323002/diff/340001/media/blink/video_fram... media/blink/video_frame_compositor.cc:53: delete auto_open_close_; This will only work if VFC is created on the compositor_task_runner thread.
If the problem is that begin/end are called form other threads, you can make them easily thread safe in your ACE. The only state you care about is the start_time_, the trace macros themselves are threadsafe. Either protect them with a lock or use a release_store and acquire_load pair and drop the threadcheker in those two methods. The thing that you still have to guarantee is the ctor/dtor are called on the same thread, as ben pointed out https://codereview.chromium.org/2159323002/diff/340001/base/trace_event/auto_... File base/trace_event/auto_open_close_event.h (right): https://codereview.chromium.org/2159323002/diff/340001/base/trace_event/auto_... base/trace_event/auto_open_close_event.h:50: DISALLOW_COPY_AND_ASSIGN(AutoOpenCloseEvent); nit: add a \n beween line 49 and 50 https://codereview.chromium.org/2159323002/diff/340001/media/blink/video_fram... File media/blink/video_frame_compositor.cc (right): https://codereview.chromium.org/2159323002/diff/340001/media/blink/video_fram... media/blink/video_frame_compositor.cc:43: auto_open_close_ = new base::trace_event::AutoOpenCloseEvent( raw pointers with manual new/delete are discouraged. Why this isn't a scoped_ptr? https://codereview.chromium.org/2159323002/diff/340001/media/blink/video_fram... media/blink/video_frame_compositor.cc:53: delete auto_open_close_; this should be just a auto_open_close.reset()
https://www.chromium.org/developers/smart-pointer-guidelines https://codereview.chromium.org/2159323002/diff/340001/media/blink/video_fram... File media/blink/video_frame_compositor.cc (right): https://codereview.chromium.org/2159323002/diff/340001/media/blink/video_fram... media/blink/video_frame_compositor.cc:62: if aoce doesn't exist, aoce_ = new AOCE(); if rendering_: Begin() else End() https://codereview.chromium.org/2159323002/diff/340001/media/blink/video_fram... File media/blink/video_frame_compositor.h (right): https://codereview.chromium.org/2159323002/diff/340001/media/blink/video_fram... media/blink/video_frame_compositor.h:170: base::trace_event::AutoOpenCloseEvent* auto_open_close_; std::unique_ptr<AOCE> aoce_;
Made it so that the ctor, begin, end, and dtor function in the AutoOpenCloseEvent are always called from the compositor thread, thus no additional thread safety machanisms are necessary. https://codereview.chromium.org/2159323002/diff/340001/base/trace_event/auto_... File base/trace_event/auto_open_close_event.h (right): https://codereview.chromium.org/2159323002/diff/340001/base/trace_event/auto_... base/trace_event/auto_open_close_event.h:50: DISALLOW_COPY_AND_ASSIGN(AutoOpenCloseEvent); On 2016/10/12 at 16:36:50, Primiano Tucci wrote: > nit: add a \n beween line 49 and 50 Done https://codereview.chromium.org/2159323002/diff/340001/media/blink/video_fram... File media/blink/video_frame_compositor.cc (right): https://codereview.chromium.org/2159323002/diff/340001/media/blink/video_fram... media/blink/video_frame_compositor.cc:43: auto_open_close_ = new base::trace_event::AutoOpenCloseEvent( On 2016/10/12 at 16:36:50, Primiano Tucci wrote: > raw pointers with manual new/delete are discouraged. Why this isn't a scoped_ptr? Changed to unique ptr https://codereview.chromium.org/2159323002/diff/340001/media/blink/video_fram... File media/blink/video_frame_compositor.h (right): https://codereview.chromium.org/2159323002/diff/340001/media/blink/video_fram... media/blink/video_frame_compositor.h:170: base::trace_event::AutoOpenCloseEvent* auto_open_close_; On 2016/10/12 at 20:41:10, benjhayden wrote: > std::unique_ptr<AOCE> aoce_; Done
LGTM with some final minor comments assuming that the threading is right. I don't know that media code but the threadchecker will definitely bark if it's not right. Thankfully we had that in place in the first place. P.S. The commit message should be wrapped at 72 cols https://codereview.chromium.org/2159323002/diff/360001/media/blink/video_fram... File media/blink/video_frame_compositor.cc (right): https://codereview.chromium.org/2159323002/diff/360001/media/blink/video_fram... media/blink/video_frame_compositor.cc:40: auto_open_close_(nullptr) { no need to initialze this. unique_ptr is not a POD and will be initialized by default by the ctor. https://codereview.chromium.org/2159323002/diff/360001/media/blink/video_fram... File media/blink/video_frame_compositor.h (right): https://codereview.chromium.org/2159323002/diff/360001/media/blink/video_fram... media/blink/video_frame_compositor.h:18: #include "base/trace_event/auto_open_close_event.h" you don't need this, you can fwd declare AOCE as long as the dtor of this class it not implicit. Or if you really want to keep this include here, remove from the .cc file.
The trybots caught the thread safety issues with previous patches, right? What do they say now that AOCE is only accessed on the compositor thread? https://codereview.chromium.org/2159323002/diff/360001/media/blink/video_fram... File media/blink/video_frame_compositor.h (right): https://codereview.chromium.org/2159323002/diff/360001/media/blink/video_fram... media/blink/video_frame_compositor.h:15: #include "base/threading/thread_task_runner_handle.h" Is this needed? https://codereview.chromium.org/2159323002/diff/360001/media/blink/video_fram... media/blink/video_frame_compositor.h:18: #include "base/trace_event/auto_open_close_event.h" On 2016/10/13 at 20:11:00, Primiano Tucci wrote: > you don't need this, you can fwd declare AOCE as long as the dtor of this class it not implicit. > Or if you really want to keep this include here, remove from the .cc file. Forward declaration is preferable because it speeds up compiling all of the files that include this header, none of which need to know anything about AOCE or anything that aoce.h includes.
The CQ bit was checked by alexandermont@chromium.org
https://codereview.chromium.org/2159323002/diff/360001/media/blink/video_fram... File media/blink/video_frame_compositor.cc (right): https://codereview.chromium.org/2159323002/diff/360001/media/blink/video_fram... media/blink/video_frame_compositor.cc:40: auto_open_close_(nullptr) { On 2016/10/13 at 20:11:00, Primiano Tucci wrote: > no need to initialze this. unique_ptr is not a POD and will be initialized by default by the ctor. Done https://codereview.chromium.org/2159323002/diff/360001/media/blink/video_fram... File media/blink/video_frame_compositor.h (right): https://codereview.chromium.org/2159323002/diff/360001/media/blink/video_fram... media/blink/video_frame_compositor.h:15: #include "base/threading/thread_task_runner_handle.h" On 2016/10/13 at 21:25:41, benjhayden wrote: > Is this needed? Done https://codereview.chromium.org/2159323002/diff/360001/media/blink/video_fram... media/blink/video_frame_compositor.h:18: #include "base/trace_event/auto_open_close_event.h" On 2016/10/13 at 21:25:41, benjhayden wrote: > On 2016/10/13 at 20:11:00, Primiano Tucci wrote: > > you don't need this, you can fwd declare AOCE as long as the dtor of this class it not implicit. > > Or if you really want to keep this include here, remove from the .cc file. > > Forward declaration is preferable because it speeds up compiling all of the files that include this header, none of which need to know anything about AOCE or anything that aoce.h includes. Done
The patchset sent to the CQ was uploaded after l-g-t-m from oysteine@chromium.org, charliea@chromium.org, dalecurtis@chromium.org, thakis@chromium.org, primiano@chromium.org Link to the patchset: https://codereview.chromium.org/2159323002/#ps380001 (title: "add forward declaration")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #20 (id:380001)
Message was sent while issue was closed.
Description was changed from ========== Add tracing AutoOpenCloseEvent. The AutoOpenCloseEvent class handles events that start and stop. A user creates a AutoOpenCloseEvent object, then calls Start() on it to indicate that the event starts, and Stop() on it to indicate that the event stops. AutoOpenCloseEvent also handles the case where Start() was called before tracing started. ========== to ========== Add tracing AutoOpenCloseEvent. The AutoOpenCloseEvent class handles events that start and stop. A user creates a AutoOpenCloseEvent object, then calls Start() on it to indicate that the event starts, and Stop() on it to indicate that the event stops. AutoOpenCloseEvent also handles the case where Start() was called before tracing started. Committed: https://crrev.com/146c424c7d4fc43699d530675c9839833f0e8418 Cr-Commit-Position: refs/heads/master@{#425439} ==========
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/146c424c7d4fc43699d530675c9839833f0e8418 Cr-Commit-Position: refs/heads/master@{#425439} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
