Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1929913002:
[tracing] Add task execution event for sequenced worker pool (Closed)
Description[tracing] Add task execution event for sequenced worker pool
The sequenced worker pool tasks use a flow event while other task
runners add without flow. So, instead of using TRACE_TASK_EXECUTION
macro, add just the SCOPED_TASK_EXECUTION_EVENT part for these tasks.
BUG=600440
Committed: https://crrev.com/90b03660f60165cb29a92eb0bba9b6d61812295b
Cr-Commit-Position: refs/heads/master@{#393976}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Remove UID mangle on name. #Patch Set 3 : Rebase. #Messages
Total messages: 27 (8 generated)
ssid@chromium.org changed reviewers: + thakis@chromium.org
2 line change. ptal, thanks.
friendly ping
primiano@chromium.org changed reviewers: + primiano@chromium.org
https://codereview.chromium.org/1929913002/diff/1/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/1929913002/diff/1/base/threading/sequenced_wo... base/threading/sequenced_worker_pool.cc:813: TRACE_EVENT_API_SCOPED_TASK_EXECUTION_EVENT INTERNAL_TRACE_EVENT_UID( you don't need EVENT INTERNAL_TRACE_EVENT_UID here. 1) as the name suggests it's internal :) 2) That is only used to disambiguate the local variable name when using the TRACE_TASK_EXECUTION macro. Since here you are instantiating explicitly you can just pick a name, e.g. : TRACE_EVENT_API_SCOPED_TASK_EXECUTION_EVENT task_event(task.posted_from.file_name());
Fixed it. Thanks, ptal. https://codereview.chromium.org/1929913002/diff/1/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/1929913002/diff/1/base/threading/sequenced_wo... base/threading/sequenced_worker_pool.cc:813: TRACE_EVENT_API_SCOPED_TASK_EXECUTION_EVENT INTERNAL_TRACE_EVENT_UID( On 2016/05/04 08:21:50, Primiano Tucci wrote: > you don't need EVENT INTERNAL_TRACE_EVENT_UID here. > 1) as the name suggests it's internal :) > 2) That is only used to disambiguate the local variable name when using the > TRACE_TASK_EXECUTION macro. Since here you are instantiating explicitly you can > just pick a name, e.g. > : > > TRACE_EVENT_API_SCOPED_TASK_EXECUTION_EVENT > task_event(task.posted_from.file_name()); I added this because every time i didn't in past you had told me to add. yes it is not required for this case. Sorry.
primiano@chromium.org changed reviewers: + fdoray@chromium.org, skyostil@chromium.org
Looks good from my side. Can I just ask +skyostil, +fdoray to take a look to this one liner? This might be somehow related with the recent https://codereview.chromium.org/1937323002/ and you guys definitely know way more than me about what is the right way of tracing tasks. The question for you here is: is there any harm in adding this? If not this will give more context to the heap profiler (and in general it feels we should have events for top-level task executions). https://codereview.chromium.org/1929913002/diff/1/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/1929913002/diff/1/base/threading/sequenced_wo... base/threading/sequenced_worker_pool.cc:813: TRACE_EVENT_API_SCOPED_TASK_EXECUTION_EVENT INTERNAL_TRACE_EVENT_UID( On 2016/05/06 03:15:26, ssid wrote: > On 2016/05/04 08:21:50, Primiano Tucci wrote: > > you don't need EVENT INTERNAL_TRACE_EVENT_UID here. > > 1) as the name suggests it's internal :) > > 2) That is only used to disambiguate the local variable name when using the > > TRACE_TASK_EXECUTION macro. Since here you are instantiating explicitly you > can > > just pick a name, e.g. > > : > > > > TRACE_EVENT_API_SCOPED_TASK_EXECUTION_EVENT > > task_event(task.posted_from.file_name()); > > I added this because every time i didn't in past you had told me to add. yes it > is not required for this case. Sorry. Oops. I hope I didn't suggest in non-tracing code, that should be internal and used only in macros.
lgtm, although I wish TRACE_EVENT_API_SCOPED_TASK_EXECUTION_EVENT had a name which made it obvious it's only for heap tracing.
On 2016/05/06 10:25:45, Sami wrote: > lgtm, although I wish TRACE_EVENT_API_SCOPED_TASK_EXECUTION_EVENT had a name > which made it obvious it's only for heap tracing. Oh. I think you are correct, the best form of correct. I mistakenly thought this was also introducing a slice, it is not. This is really pushing/popping the state in the pseudo stack tracker. This patch LGTM (sorry for the confusion, shouldn't have cc-ed you in the first place. I thought this was creating a slice). But, yes, in a seprate patch we should rename that to something more clear (and, yes, I'm to blame as I didn't realize that while reviewing the CL that introduced it). TASK_EXECUTION_EVENT suggests that we are adding an event. We are not, we just bookkeep. Let's figure out a better name in a separate CL.
The CL's description says that this "adds task execution event for sequenced
worker pool", but this doesn't add any tracing event.
MessageLoop uses TRACE_TASK_EXECUTION + TaskAnnotator to execute a Task. Could
we do the same thing in SequencedWorkerPool?
1. Move |task| and |posted_from| from base::PendingTask/base::SequencedTask to
their base class base::TrackingInfo.
2. Change the argument type of TaskAnnotator from base::PendingTask to
base::TrackingInfo.
3. Replace lines 807-814 with:
TRACE_TASK_EXECUTION("SequencedWorkerPool::Inner::ThreadLoop", task);
4. Replace lines 832-838 with:
TaskAnnotator task_annotator;
task_annotator.RunTask("SequencedWorkerPool::PostTask", task);
On 2016/05/06 15:52:21, fdoray wrote:
> The CL's description says that this "adds task execution event for sequenced
> worker pool", but this doesn't add any tracing event.
>
Well, the event is for tracing memory allocations, rather than tracing time. So,
it was called TRACE_EVENT.
I agree that the name should have been better, something like
HEAP_PROFILER_EVENT. But with the current name I think the CL description is
matching.
I am planning a clean up of this event with better name in next CL.
> MessageLoop uses TRACE_TASK_EXECUTION + TaskAnnotator to execute a Task. Could
> we do the same thing in SequencedWorkerPool?
>
> 1. Move |task| and |posted_from| from base::PendingTask/base::SequencedTask to
> their base class base::TrackingInfo.
> 2. Change the argument type of TaskAnnotator from base::PendingTask to
> base::TrackingInfo.
> 3. Replace lines 807-814 with:
> TRACE_TASK_EXECUTION("SequencedWorkerPool::Inner::ThreadLoop", task);
Not sure if I understand correctly, are you saying the FLOW_EVENT is not useful
and can be replaced with TRACE_TASK_EXECUTION? If yes, I could do it in this CL.
> 4. Replace lines 832-838 with:
> TaskAnnotator task_annotator;
> task_annotator.RunTask("SequencedWorkerPool::PostTask", task);
Is this a change that you are asking me to work on? Right now I have other
things in my queue and it is not very related to tracing. It changes the
behavior of the existing code. So, I might not be the right person to take up
this work.
If it is a general suggestion, then do you have any concerns related to the way
we trace the tasks here?
lgtm The code works but I think we should use TRACE_TASK_EXECUTION+TaskAnnotator
like MessageLoop does to avoid code duplication. This can be done in another CL.
I can work on it if you want.
On 2016/05/06 17:28:25, ssid wrote:
> On 2016/05/06 15:52:21, fdoray wrote:
> > The CL's description says that this "adds task execution event for sequenced
> > worker pool", but this doesn't add any tracing event.
> >
> Well, the event is for tracing memory allocations, rather than tracing time.
So,
> it was called TRACE_EVENT.
> I agree that the name should have been better, something like
> HEAP_PROFILER_EVENT. But with the current name I think the CL description is
> matching.
> I am planning a clean up of this event with better name in next CL.
sgtm
>
> > MessageLoop uses TRACE_TASK_EXECUTION + TaskAnnotator to execute a Task.
Could
> > we do the same thing in SequencedWorkerPool?
> >
> > 1. Move |task| and |posted_from| from base::PendingTask/base::SequencedTask
to
> > their base class base::TrackingInfo.
> > 2. Change the argument type of TaskAnnotator from base::PendingTask to
> > base::TrackingInfo.
> > 3. Replace lines 807-814 with:
> > TRACE_TASK_EXECUTION("SequencedWorkerPool::Inner::ThreadLoop", task);
>
> Not sure if I understand correctly, are you saying the FLOW_EVENT is not
useful
> and can be replaced with TRACE_TASK_EXECUTION? If yes, I could do it in this
CL.
Rather than using the TRACE_EVENT_WITH_FLOW2 macro directly, MessageLoop and
others use TaskAnnotator
https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop/...
TaskAnnotator also handles the StopWatch+TallyRunOnNamedThreadIfTracking that is
done at lines 830-835 of sequenced_worker_pool.cc.
>
> > 4. Replace lines 832-838 with:
> > TaskAnnotator task_annotator;
> > task_annotator.RunTask("SequencedWorkerPool::PostTask", task);
>
> Is this a change that you are asking me to work on? Right now I have other
> things in my queue and it is not very related to tracing. It changes the
> behavior of the existing code. So, I might not be the right person to take up
> this work.
> If it is a general suggestion, then do you have any concerns related to the
way
> we trace the tasks here?
On 2016/05/06 17:43:55, fdoray wrote: > lgtm The code works but I think we should use TRACE_TASK_EXECUTION+TaskAnnotator > like MessageLoop does to avoid code duplication. This can be done in another CL. > I can work on it if you want. Thanks, sounds good. I have filed a bug crbug.com/611818 for this reason.
@Nico: Please take a look at this CL. This CL just adds a trace event. Note: I have updated the name of the macro in another CL: https://codereview.chromium.org/1950313005/. Filed another bug to remove code duplication.
On 2016/05/13 18:37:05, ssid wrote: > @Nico: Please take a look at this CL. > This CL just adds a trace event. Sorry not trace event. It is a "heap profiler tracker".
lgtm
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1929913002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1929913002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, fdoray@chromium.org, skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/1929913002/#ps40001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1929913002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1929913002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [tracing] Add task execution event for sequenced worker pool The sequenced worker pool tasks use a flow event while other task runners add without flow. So, instead of using TRACE_TASK_EXECUTION macro, add just the SCOPED_TASK_EXECUTION_EVENT part for these tasks. BUG=600440 ========== to ========== [tracing] Add task execution event for sequenced worker pool The sequenced worker pool tasks use a flow event while other task runners add without flow. So, instead of using TRACE_TASK_EXECUTION macro, add just the SCOPED_TASK_EXECUTION_EVENT part for these tasks. BUG=600440 Committed: https://crrev.com/90b03660f60165cb29a92eb0bba9b6d61812295b Cr-Commit-Position: refs/heads/master@{#393976} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/90b03660f60165cb29a92eb0bba9b6d61812295b Cr-Commit-Position: refs/heads/master@{#393976} |
