Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10825022:
Support profiling of tasks run as sequenced_tasks (Closed)
|
Created:
8 years, 5 months ago by jar (doing other things) Modified:
8 years, 4 months ago CC:
chromium-reviews, erikwright (departed), brettw-cc_chromium.org, Ilya Sherman, ramant (doing other things), eroman, natduca Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionSupport profiling of tasks run as sequenced_tasks
Mimic code seen in message_loop.cc (since these tasks are run on named
threads) to support tracking of sequenced worker pool runs of tasks.
We surround the *.Run() method with a call to get the time before we
start, and then call to tally the time it took to run after task.Run()
returns.
r=brettw
BUG=139035
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148986
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
Total comments: 14
Patch Set 5 : #
Messages
Total messages: 12 (0 generated)
I think Fred has been doing most of the enhancements to this class recently. He might be a more appropriate reviewer. https://chromiumcodereview.appspot.com/10825022/diff/11001/base/tracking_info.h File base/tracking_info.h (right): https://chromiumcodereview.appspot.com/10825022/diff/11001/base/tracking_info... base/tracking_info.h:27: TrackingInfo(); Normally I'd expect this to be first.
Change made per brettw suggestion. Akalin: PTAL https://chromiumcodereview.appspot.com/10825022/diff/11001/base/tracking_info.h File base/tracking_info.h (right): https://chromiumcodereview.appspot.com/10825022/diff/11001/base/tracking_info... base/tracking_info.h:27: TrackingInfo(); On 2012/07/26 18:06:30, brettw wrote: > Normally I'd expect this to be first. Done.
jbates: akalin appears busy, and this is a pretty simple CL. Could you take a look and provide a review? (also... you'll probably have to hack in tracing RSN in this area... so you'd have to read the code a bit anyway). thanks!
On 2012/07/27 17:38:37, jar wrote: > jbates: akalin appears busy, and this is a pretty simple CL. Could you take a > look and provide a review? (also... you'll probably have to hack in tracing RSN > in this area... so you'd have to read the code a bit anyway). > > thanks! looking now (sorry for the delay)
few questions http://codereview.chromium.org/10825022/diff/6004/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool.cc (right): http://codereview.chromium.org/10825022/diff/6004/base/threading/sequenced_wo... base/threading/sequenced_worker_pool.cc:38: struct SequencedTask : public TrackingInfo { any particular reason for using inheritance? Why not just have a TrackingInfo member? http://codereview.chromium.org/10825022/diff/6004/base/threading/sequenced_wo... base/threading/sequenced_worker_pool.cc:52: tracked_objects::Location posted_from; what's the difference between this and time_posted (from TrackingInfo)? I don't see this used anywhere... http://codereview.chromium.org/10825022/diff/6004/base/tracking_info.cc File base/tracking_info.cc (right): http://codereview.chromium.org/10825022/diff/6004/base/tracking_info.cc#newco... base/tracking_info.cc:12: : birth_tally(static_cast<tracked_objects::Births*>(NULL)) { any particular reason for the explicit cast? just for documentation purposes?
Responding to questions by akalin. Several of your questions suggest larger changes. I do think it is better to have similar calling patterns in all places... but perchance such changes should be a different CL. Right now, we're not getting any records in the profiler of a whole group of tasks... and I'd like to change that status asap. http://codereview.chromium.org/10825022/diff/6004/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool.cc (right): http://codereview.chromium.org/10825022/diff/6004/base/threading/sequenced_wo... base/threading/sequenced_worker_pool.cc:38: struct SequencedTask : public TrackingInfo { First note that TrackingInfo is also a struct. Truth be known... I used inherritance to continue matching the code structure used in message_loop.cc as precisely as possible. I only recently learned that we "prefer" encapsulation via a member, to inheritance. In this case, the encapsulation would just imply that references to the struct member of the encapsulated member would be a bit more verbose (and we'd call into the profiler passing only the tracking info, rather than the "task"). I can change it here (and even in the corresponding code in message_loop as well) if you significantly prefer it. I could also do such a change (to both call sites) as a separable CL. On 2012/07/27 17:45:39, akalin wrote: > any particular reason for using inheritance? Why not just have a TrackingInfo > member? http://codereview.chromium.org/10825022/diff/6004/base/threading/sequenced_wo... base/threading/sequenced_worker_pool.cc:52: tracked_objects::Location posted_from; This change was a name change only (in this file), to a more descriptive name, that also matched the usage in message_loop and worker_pool. The (historic) value of this field tended to appear when running a debugger, and allows a developer to see where the task was "posted_from." I think it is the case that *now* that the profiler is indeed always on (it currently includes code to turn off profiling, either statically, or dynamically), that we can deduce this location by traversing into the profiler data structures. I probably should create a bug to rip out the conditional code in profiler, and then revisit this placement of the field into the tasks. That change seems pretty orthogonal to this... which just improved the name (but it also made me notice the variable wasn't getting much use, and might not be needed anymore, even for developers). On 2012/07/27 17:45:39, akalin wrote: > what's the difference between this and time_posted (from TrackingInfo)? I don't > see this used anywhere... http://codereview.chromium.org/10825022/diff/6004/base/tracking_info.cc File base/tracking_info.cc (right): http://codereview.chromium.org/10825022/diff/6004/base/tracking_info.cc#newco... base/tracking_info.cc:12: : birth_tally(static_cast<tracked_objects::Births*>(NULL)) { I had vague recollections of some compiler complaints... but maybe it was only in the context of things like CHECK_EQ and such. It certainly provides a bit of documentation as well. If you'd like, I can yank it and see how it does. On 2012/07/27 17:45:39, akalin wrote: > any particular reason for the explicit cast? just for documentation purposes?
LGTM (one nit) I was also curious about the inheritance vs member choice of TrackingInfo, but it does seem best to keep the design in sync with the other instances and then change them all one way or the other with a separate CL. http://codereview.chromium.org/10825022/diff/6004/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool.cc (right): http://codereview.chromium.org/10825022/diff/6004/base/threading/sequenced_wo... base/threading/sequenced_worker_pool.cc:52: tracked_objects::Location posted_from; On 2012/07/27 18:15:46, jar wrote: > This change was a name change only (in this file), to a more descriptive name, > that also matched the usage in message_loop and worker_pool. > > The (historic) value of this field tended to appear when running a debugger, and > allows a developer to see where the task was "posted_from." > > I think it is the case that *now* that the profiler is indeed always on (it > currently includes code to turn off profiling, either statically, or > dynamically), that we can deduce this location by traversing into the profiler > data structures. > > I probably should create a bug to rip out the conditional code in profiler, and > then revisit this placement of the field into the tasks. > > That change seems pretty orthogonal to this... which just improved the name (but > it also made me notice the variable wasn't getting much use, and might not be > needed anymore, even for developers). > > On 2012/07/27 17:45:39, akalin wrote: > > what's the difference between this and time_posted (from TrackingInfo)? I > don't > > see this used anywhere... > Btw, I'll use this posted_from for tracing these tasks with the TRACE_EVENT patch. The Location is not actually retained by TrackingInfo, although it could be. http://codereview.chromium.org/10825022/diff/6004/base/threading/sequenced_wo... base/threading/sequenced_worker_pool.cc:623: tracked_objects::ThreadData::NowForStartOfRun(task.birth_tally); nit: one more space on indent
lgtm http://codereview.chromium.org/10825022/diff/6004/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool.cc (right): http://codereview.chromium.org/10825022/diff/6004/base/threading/sequenced_wo... base/threading/sequenced_worker_pool.cc:38: struct SequencedTask : public TrackingInfo { On 2012/07/27 18:15:46, jar wrote: > I only recently learned that we "prefer" encapsulation via a member, to > inheritance. In this case, the encapsulation would just imply that references > to the struct member of the encapsulated member would be a bit more verbose (and > we'd call into the profiler passing only the tracking info, rather than the > "task"). > > I can change it here (and even in the corresponding code in message_loop as > well) if you significantly prefer it. I could also do such a change (to both > call sites) as a separable CL. Doing it as a separate CL sgtm. http://codereview.chromium.org/10825022/diff/6004/base/threading/sequenced_wo... base/threading/sequenced_worker_pool.cc:52: tracked_objects::Location posted_from; On 2012/07/27 19:54:10, jbates wrote: > On 2012/07/27 18:15:46, jar wrote: > > This change was a name change only (in this file), to a more descriptive name, > > that also matched the usage in message_loop and worker_pool. > > > > The (historic) value of this field tended to appear when running a debugger, > and > > allows a developer to see where the task was "posted_from." > > > > I think it is the case that *now* that the profiler is indeed always on (it > > currently includes code to turn off profiling, either statically, or > > dynamically), that we can deduce this location by traversing into the profiler > > data structures. > > > > I probably should create a bug to rip out the conditional code in profiler, > and > > then revisit this placement of the field into the tasks. > > > > That change seems pretty orthogonal to this... which just improved the name > (but > > it also made me notice the variable wasn't getting much use, and might not be > > needed anymore, even for developers). > > > > On 2012/07/27 17:45:39, akalin wrote: > > > what's the difference between this and time_posted (from TrackingInfo)? I > > don't > > > see this used anywhere... I'm okay with ripping it out in a separate CL if needed, if it's orthogonal. http://codereview.chromium.org/10825022/diff/6004/base/tracking_info.cc File base/tracking_info.cc (right): http://codereview.chromium.org/10825022/diff/6004/base/tracking_info.cc#newco... base/tracking_info.cc:12: : birth_tally(static_cast<tracked_objects::Births*>(NULL)) { On 2012/07/27 18:15:46, jar wrote: > I had vague recollections of some compiler complaints... but maybe it was only > in the context of things like CHECK_EQ and such. It certainly provides a bit of > documentation as well. > > If you'd like, I can yank it and see how it does. I think I'd prefer to yank it out (and I expect it to work). Having the cast seems to me like it would make the reader question why the cast is needed. Also, may as well #include <cstddef> (or stddef.h) for NULL.
Changes made per comments by akalin and jbates. http://codereview.chromium.org/10825022/diff/6004/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool.cc (right): http://codereview.chromium.org/10825022/diff/6004/base/threading/sequenced_wo... base/threading/sequenced_worker_pool.cc:52: tracked_objects::Location posted_from; On 2012/07/27 19:54:10, jbates wrote: > Btw, I'll use this posted_from for tracing these tasks with the TRACE_EVENT > patch. The Location is not actually retained by TrackingInfo, although it could > be. Although task doesn't appear to directly save it... it links to the location (most of the time). task.birth_tally is a class Births Today is is NULL if tracking is disabled... but would always be defined if we always did profiling. Then task.birth_tally.Location() would get you the actual posted_from value. Today, due to the possible NULL Births*, it might not always work... but if I ensured it was always set, then you'd be good to go. It is a bit more complex for folks to see the data... but it really doesn't need to be here (I think). Note that a location instance is merely an int, plus a pointer to an atom of a string, so the memory implications of the current approach is not significant. ...also... if I stopped doing this via inheritance, and instead used encapsulation, it would be more like: task.tracking_info.birth_tally.Location() http://codereview.chromium.org/10825022/diff/6004/base/threading/sequenced_wo... base/threading/sequenced_worker_pool.cc:623: tracked_objects::ThreadData::NowForStartOfRun(task.birth_tally); On 2012/07/27 19:54:10, jbates wrote: > nit: one more space on indent Done. http://codereview.chromium.org/10825022/diff/6004/base/tracking_info.cc File base/tracking_info.cc (right): http://codereview.chromium.org/10825022/diff/6004/base/tracking_info.cc#newco... base/tracking_info.cc:12: : birth_tally(static_cast<tracked_objects::Births*>(NULL)) { On 2012/07/27 20:18:18, akalin wrote: > On 2012/07/27 18:15:46, jar wrote: > > I had vague recollections of some compiler complaints... but maybe it was only > > in the context of things like CHECK_EQ and such. It certainly provides a bit > of > > documentation as well. > > > > If you'd like, I can yank it and see how it does. > > I think I'd prefer to yank it out (and I expect it to work). Having the cast > seems to me like it would make the reader question why the cast is needed. > > Also, may as well #include <cstddef> (or stddef.h) for NULL. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jar@chromium.org/10825022/15002
Change committed as 148986 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
