Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1275693002:
Change PostTask to use Flow v2. (Closed)
DescriptionChange PostTask in base::debug::TaskAnnotator::RunTask() and base::debug::TaskAnnotator::DidQueueTask() to use Flow v2.
BUG=https://github.com/catapult-project/catapult/issues/1079
Committed: https://crrev.com/bbe9cfdf0feedae4392ccbc16f2784ab527bd466
Cr-Commit-Position: refs/heads/master@{#342403}
Patch Set 1 #Patch Set 2 : Name tweak. #
Total comments: 5
Patch Set 3 : Use queue_function as the slice name. #
Total comments: 1
Patch Set 4 : Remove braces. #Messages
Total messages: 32 (7 generated)
yuhaoz@google.com changed reviewers: + beaudoin@chromium.org, dsinclair@chromium.org, nduca@chromium.org, skyostil@chromium.org, vmpstr@chromium.org
Change PostTask to use Flow v2.
https://codereview.chromium.org/1275693002/diff/20001/base/debug/task_annotat... File base/debug/task_annotator.cc (right): https://codereview.chromium.org/1275693002/diff/20001/base/debug/task_annotat... base/debug/task_annotator.cc:23: TRACE_EVENT_WITH_FLOW0("toplevel", The category should still be TRACE_DISABLED_BY_DEFAULT("toplevel.flow") https://codereview.chromium.org/1275693002/diff/20001/base/debug/task_annotat... base/debug/task_annotator.cc:24: "QueueTask", Why did you change the name from queue_function? https://codereview.chromium.org/1275693002/diff/20001/base/debug/task_annotat... base/debug/task_annotator.cc:37: TRACE_EVENT_WITH_FLOW1("toplevel", TRACE_DISABLED_BY_DEFAULT("toplevel.flow") https://codereview.chromium.org/1275693002/diff/20001/base/debug/task_annotat... base/debug/task_annotator.cc:38: "RunTask", Why did this change from queue_function to RunTask?
LGTM given Dan's comments.
https://codereview.chromium.org/1275693002/diff/20001/base/debug/task_annotat... File base/debug/task_annotator.cc (right): https://codereview.chromium.org/1275693002/diff/20001/base/debug/task_annotat... base/debug/task_annotator.cc:24: "QueueTask", On 2015/08/05 at 19:38:04, dsinclair wrote: > Why did you change the name from queue_function? Because queue_function is the name of the original flow events. But with TRACE_EVENT_WITH_FLOW, we are creating a slice, and the name here is going to be the same of the slice. If we directly use queue_function, we will get the following: [ toplevel ] [ toplevel ] [MessageLoop::PostTask] ------------------------------------------> [MessageLoop:PostTask] This is because both the producer and consumer pass "MessageLoop:PostTask" as queue_function. The producer code: https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop/... The consumer code: https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop/...
On 2015/08/05 at 21:22:07, yuhaoz wrote: > https://codereview.chromium.org/1275693002/diff/20001/base/debug/task_annotat... > File base/debug/task_annotator.cc (right): > > https://codereview.chromium.org/1275693002/diff/20001/base/debug/task_annotat... > base/debug/task_annotator.cc:24: "QueueTask", > On 2015/08/05 at 19:38:04, dsinclair wrote: > > Why did you change the name from queue_function? > > Because queue_function is the name of the original flow events. But with TRACE_EVENT_WITH_FLOW, we are creating a slice, and the name here is going to be the same of the slice. If we directly use queue_function, we will get the following: > > > [ toplevel ] [ toplevel ] > [MessageLoop::PostTask] ------------------------------------------> [MessageLoop:PostTask] > > > This is because both the producer and consumer pass "MessageLoop:PostTask" as queue_function. > > The producer code: > https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop/... > > The consumer code: > https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop/... How about using the function name as |name| here. [ toplevel ] [ toplevel ] [TaskAnnotator::DidQueueTask] -------------> [TaskAnnotator::RunTask]
On 2015/08/05 21:25:22, yuhaoz wrote: > On 2015/08/05 at 21:22:07, yuhaoz wrote: > > > https://codereview.chromium.org/1275693002/diff/20001/base/debug/task_annotat... > > File base/debug/task_annotator.cc (right): > > > > > https://codereview.chromium.org/1275693002/diff/20001/base/debug/task_annotat... > > base/debug/task_annotator.cc:24: "QueueTask", > > On 2015/08/05 at 19:38:04, dsinclair wrote: > > > Why did you change the name from queue_function? > > > > Because queue_function is the name of the original flow events. But with > TRACE_EVENT_WITH_FLOW, we are creating a slice, and the name here is going to be > the same of the slice. If we directly use queue_function, we will get the > following: > > > > > > [ toplevel ] [ > toplevel ] > > [MessageLoop::PostTask] ------------------------------------------> > [MessageLoop:PostTask] > > > > > > This is because both the producer and consumer pass "MessageLoop:PostTask" as > queue_function. > > > > The producer code: > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop/... > > > > The consumer code: > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop/... > > How about using the function name as |name| here. > > [ toplevel ] [ toplevel > ] > [TaskAnnotator::DidQueueTask] -------------> [TaskAnnotator::RunTask] The queue_function is only used for this purpose, so you can either remove it, or change the callers to use a better name.
PTAL. Now use |queue_function| as the slice name.
https://codereview.chromium.org/1275693002/diff/40001/base/debug/task_annotat... File base/debug/task_annotator.cc (right): https://codereview.chromium.org/1275693002/diff/40001/base/debug/task_annotat... base/debug/task_annotator.cc:36: { This doesn't need braces around it.
PTAL.
lgtm. Can you please add the BUG= for this work?
On 2015/08/07 at 14:02:17, dsinclair wrote: > lgtm. Can you please add the BUG= for this work? Is there a BUG associated with this or do you want to create a new one?
On 2015/08/07 14:59:46, yuhaoz wrote: > On 2015/08/07 at 14:02:17, dsinclair wrote: > > lgtm. Can you please add the BUG= for this work? > > Is there a BUG associated with this or do you want to create a new one? I believe you had a bug for the flow v2 work yes?
On 2015/08/07 at 15:02:34, dsinclair wrote: > I believe you had a bug for the flow v2 work yes? We have a trace-viewer bug for it, not a chromium bug. Did you mean to associated with trace-viewer (catapult) bug?
On 2015/08/07 15:06:25, yuhaoz wrote: > On 2015/08/07 at 15:02:34, dsinclair wrote: > > I believe you had a bug for the flow v2 work yes? > > We have a trace-viewer bug for it, not a chromium bug. Did you mean to > associated with trace-viewer (catapult) bug? Sure, just use BUG=catapult-project:#### which I believe works. You'll also need an OWNER lgtm.
yuhaoz@google.com changed reviewers: + danakj@chromium.org, mark@chromium.org, rvargas@chromium.org, thakis@chromium.org, thestig@chromium.org
Changing PostTask to use Flow V2. Of critical importance to correctly trace event relationship and debugging.
On 2015/08/07 16:26:19, yuhaoz wrote: > Changing PostTask to use Flow V2. Of critical importance to correctly trace > event relationship and debugging. yuhaoz: It looks like you added a bunch of base/ OWNERS to ask for a review. Can you say so in your message? dsinclair: what's catapult-project? I can't find it anywhere.
On 2015/08/07 17:20:57, Lei Zhang wrote: > On 2015/08/07 16:26:19, yuhaoz wrote: > > Changing PostTask to use Flow V2. Of critical importance to correctly trace > > event relationship and debugging. > > yuhaoz: It looks like you added a bunch of base/ OWNERS to ask for a review. Can > you say so in your message? > > dsinclair: what's catapult-project? I can't find it anywhere. https://github.com/catapult-project/catapult It's the new home for trace-viewer, telemetry, perf dashboard and other performance related things.
On 2015/08/07 17:23:07, dsinclair wrote: > On 2015/08/07 17:20:57, Lei Zhang wrote: > > On 2015/08/07 16:26:19, yuhaoz wrote: > > > Changing PostTask to use Flow V2. Of critical importance to correctly trace > > > event relationship and debugging. > > > > yuhaoz: It looks like you added a bunch of base/ OWNERS to ask for a review. > Can > > you say so in your message? > > > > dsinclair: what's catapult-project? I can't find it anywhere. > > https://github.com/catapult-project/catapult > > It's the new home for trace-viewer, telemetry, perf dashboard and other > performance related things. Maybe BUG=https://github.com/catapult-project/catapult/issues/1079 then?
yuhaoz@google.com changed reviewers: - danakj@chromium.org, mark@chromium.org, rvargas@chromium.org, thakis@chromium.org
On 2015/08/07 at 17:30:07, thestig wrote: > > > yuhaoz: It looks like you added a bunch of base/ OWNERS to ask for a review. > > Can > > > you say so in your message? This CL changes the PostTask from using the old Flow API to Flow API v2 (https://codereview.chromium.org/1239593002). It allows us to bind the flow to definitively bind to a trace event slice, and therefore allows better debugging capability and performance related analysis.
lgtm
The CQ bit was checked by yuhaoz@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from beaudoin@chromium.org Link to the patchset: https://codereview.chromium.org/1275693002/#ps60001 (title: "Remove braces.")
Oh, in the commit message, by "PostTask", do you really mean base::debug::TaskAnnotator::RunTask() and base::debug::TaskAnnotator::DidQueueTask()?
The CQ bit was unchecked by yuhaoz@google.com
On 2015/08/07 at 17:43:18, thestig wrote: > Oh, in the commit message, by "PostTask", do you really mean base::debug::TaskAnnotator::RunTask() and base::debug::TaskAnnotator::DidQueueTask()? Yes. I will make that clarification.
The CQ bit was checked by yuhaoz@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1275693002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1275693002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/bbe9cfdf0feedae4392ccbc16f2784ab527bd466 Cr-Commit-Position: refs/heads/master@{#342403} |
