Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1937323002:
TaskScheduler: Add TRACE_TASK_EXECUTION to TaskTracker::RunTask. (Closed)
|
Created:
4 years, 7 months ago by fdoray Modified:
4 years, 7 months ago CC:
chromium-reviews, gab+watch_chromium.org, robliao+watch_chromium.org, fdoray+watch_chromium.org, Sami, alex clarke (OOO till 29th) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTaskScheduler: Add TRACE_TASK_EXECUTION to TaskTracker::RunTask.
This macro will generate a tracing event in the toplevel category when
the task scheduler runs a task.
TaskAnnotator already generates a tracing event which is connected the
post task "flow" events in the disabled-by-default-toplevel category.
BUG=553459
Committed: https://crrev.com/dbe69aff8ad2d3e336ecf2b4e83e03d92410e139
Cr-Commit-Position: refs/heads/master@{#391489}
Patch Set 1 #
Total comments: 6
Patch Set 2 : rebase, base::RunTask->TaskSchedulerRunTask, no SetSingletonAllowed in scope #
Total comments: 2
Patch Set 3 : CR robliao #7 (add comment) #
Dependent Patchsets: Messages
Total messages: 19 (6 generated)
fdoray@chromium.org changed reviewers: + gab@chromium.org, nduca@chromium.org, robliao@chromium.org
gab@/robliao@: Can you review this CL? nduca@: Can you confirm that we want both TRACE_TASK_EXECUTION and TaskAnnotator? https://codereview.chromium.org/1937323002/diff/1/base/task_scheduler/task_tr... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/1937323002/diff/1/base/task_scheduler/task_tr... base/task_scheduler/task_tracker.cc:19: const char kRunFunctionName[] = "base::RunTask"; I don't want to call this TaskTracker::RunTask because TaskTracker is an implementation detail of the task scheduler.
Thanks for fixing :-) https://codereview.chromium.org/1937323002/diff/1/base/task_scheduler/task_tr... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/1937323002/diff/1/base/task_scheduler/task_tr... base/task_scheduler/task_tracker.cc:19: const char kRunFunctionName[] = "base::RunTask"; On 2016/05/02 19:50:08, fdoray wrote: > I don't want to call this TaskTracker::RunTask because TaskTracker is an > implementation detail of the task scheduler. Okay but base::RunTask isn't a thing... Maybe just "TaskSchedulerRunTask"? At least not making it look like a base:: method. https://codereview.chromium.org/1937323002/diff/1/base/task_scheduler/task_tr... base/task_scheduler/task_tracker.cc:84: TRACE_TASK_EXECUTION(kRunFunctionName, *task); https://codereview.chromium.org/1911023002/ is scoping line 94's RunTask, this should probably go in there (or is it using a singleton and forced to be above the SetSingletonAllowed call?)
https://codereview.chromium.org/1937323002/diff/1/base/task_scheduler/task_tr... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/1937323002/diff/1/base/task_scheduler/task_tr... base/task_scheduler/task_tracker.cc:84: TRACE_TASK_EXECUTION(kRunFunctionName, *task); Should this occur right after we SetSingletonAllowed?
gab@/robliao@: Can you take another look? Thanks! https://codereview.chromium.org/1937323002/diff/1/base/task_scheduler/task_tr... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/1937323002/diff/1/base/task_scheduler/task_tr... base/task_scheduler/task_tracker.cc:19: const char kRunFunctionName[] = "base::RunTask"; On 2016/05/02 20:31:22, gab wrote: > On 2016/05/02 19:50:08, fdoray wrote: > > I don't want to call this TaskTracker::RunTask because TaskTracker is an > > implementation detail of the task scheduler. > > Okay but base::RunTask isn't a thing... Maybe just "TaskSchedulerRunTask"? At > least not making it look like a base:: method. Done. https://codereview.chromium.org/1937323002/diff/1/base/task_scheduler/task_tr... base/task_scheduler/task_tracker.cc:84: TRACE_TASK_EXECUTION(kRunFunctionName, *task); On 2016/05/02 20:32:34, robliao wrote: > Should this occur right after we SetSingletonAllowed? I initially included SetSingletonAllowed into TRACE_TASK_EXECUTION's scope because it usually includes a little bit more than the task execution itself [1][2][3]. Not sure we really want that though so the scope now includes the task execution only. [1] https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop/... [2] https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/wor... [3] https://code.google.com/p/chromium/codesearch#chromium/src/components/schedul...
lgtm
lgtm https://codereview.chromium.org/1937323002/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/1937323002/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_tracker.cc:21: const char kRunFunctionName[] = "TaskSchedulerRunTask"; Comment where this name originated.
fdoray@chromium.org changed reviewers: + primiano@chromium.org
nduca@ or primiano@: Can you confirm that we are using the TRACE_TASK_EXECUTION macro as it should? https://codereview.chromium.org/1937323002/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/1937323002/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_tracker.cc:21: const char kRunFunctionName[] = "TaskSchedulerRunTask"; On 2016/05/03 16:46:51, robliao wrote: > Comment where this name originated. Done.
On 2016/05/03 17:07:14, fdoray wrote: > nduca@ or primiano@: Can you confirm that we are using the TRACE_TASK_EXECUTION > macro as it should? The usage of the macro itself makes sense. I am just trying to follow the all picture of this. More specifically: should this be called by task_annotator, so the other clients would benefit as well? I am just not updated with all the abstraction layers of task scheduling (message loop vs task runner vs task_tracker vs task annotator) and fear we can end up doubling the events in some cases. +skyostil/alexclarke
On 2016/05/03 17:22:40, Primiano Tucci wrote: > On 2016/05/03 17:07:14, fdoray wrote: > > nduca@ or primiano@: Can you confirm that we are using the > TRACE_TASK_EXECUTION > > macro as it should? > > The usage of the macro itself makes sense. I am just trying to follow the all > picture of this. > More specifically: should this be called by task_annotator, so the other clients > would benefit as well? > I am just not updated with all the abstraction layers of task scheduling > (message loop vs task runner vs task_tracker vs task annotator) and fear we can > end up doubling the events in some cases. +skyostil/alexclarke TRACE_TASK_EXECUTION was moved out of TaskAnnotator so that work done by observers can be in its scope https://codereview.chromium.org/1237283006
skyostil@chromium.org changed reviewers: + skyostil@chromium.org
lgtm, this should make the tasks show up in tracing correctly.
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robliao@chromium.org, gab@chromium.org Link to the patchset: https://codereview.chromium.org/1937323002/#ps40001 (title: "CR robliao #7 (add comment)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1937323002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1937323002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== TaskScheduler: Add TRACE_TASK_EXECUTION to TaskTracker::RunTask. This macro will generate a tracing event in the toplevel category when the task scheduler runs a task. TaskAnnotator already generates a tracing event which is connected the post task "flow" events in the disabled-by-default-toplevel category. BUG=553459 ========== to ========== TaskScheduler: Add TRACE_TASK_EXECUTION to TaskTracker::RunTask. This macro will generate a tracing event in the toplevel category when the task scheduler runs a task. TaskAnnotator already generates a tracing event which is connected the post task "flow" events in the disabled-by-default-toplevel category. BUG=553459 Committed: https://crrev.com/dbe69aff8ad2d3e336ecf2b4e83e03d92410e139 Cr-Commit-Position: refs/heads/master@{#391489} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/dbe69aff8ad2d3e336ecf2b4e83e03d92410e139 Cr-Commit-Position: refs/heads/master@{#391489} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
