Closed
Bug 1098681
Opened 10 years ago
Closed 10 years ago
TaskTracer should call GetCurTraceInfo() on message earlier
Categories
(Core :: Gecko Profiler, defect)
Core
Gecko Profiler
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: sinker, Assigned: shelly)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
sinker
:
review+
|
Details | Diff | Splinter Review |
We call GetCurTraceInfo() in ProcessOutgoingMessages(), but it is potentially to have more than one messages that belong to different tasks on the queue. It means some of messages are given wrong trace info. We should call this function earlier; for example, at OutputQueuePush().
http://dxr.mozilla.org/mozilla-central/source/ipc/chromium/src/chrome/common/ipc_channel_posix.cc#711
Assignee | ||
Comment 1•10 years ago
|
||
I haven't test it on the real device though...
Attachment #8523804 -
Flags: review?(tlee)
Reporter | ||
Comment 2•10 years ago
|
||
Comment on attachment 8523804 [details] [diff] [review]
Call GetCurTraceInfo at where this message is enqueued
Review of attachment 8523804 [details] [diff] [review]:
-----------------------------------------------------------------
::: ipc/chromium/src/chrome/common/ipc_channel_posix.cc
@@ -607,5 @@
> " with type " << m.type();
> #endif
>
> #ifdef MOZ_TASK_TRACER
> - AutoSaveCurTraceInfo saveCurTraceInfo;
We had better to keep this line as a good discipline of always restoring state.
@@ +885,5 @@
> +#ifdef MOZ_TASK_TRACER
> + GetCurTraceInfo(&msg->header()->source_event_id,
> + &msg->header()->parent_task_id,
> + &msg->header()->source_event_type);
> +#endif
We should move this block to the line before |output_queue_.push(msg)|. Although we restrict the access of output_queue_ at the same thread, this order involves more knowledge about threading to understand it. If do all changes before pushing it to the queue, readers need not to afraid about the side-effect anymore.
Attachment #8523804 -
Flags: review?(tlee) → review-
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8523804 -
Attachment is obsolete: true
Attachment #8524269 -
Flags: review?(tlee)
Reporter | ||
Comment 4•10 years ago
|
||
Comment on attachment 8524269 [details] [diff] [review]
Call GetCurTraceInfo at where this message is enqueued (v2)
Review of attachment 8524269 [details] [diff] [review]:
-----------------------------------------------------------------
Good!
Attachment #8524269 -
Flags: review?(tlee) → review+
Assignee | ||
Comment 5•10 years ago
|
||
Thanks for the tip! I'll keep that in mind :)
Try result:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ccfd28b915ce
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 6•10 years ago
|
||
Assignee: nobody → slin
Keywords: checkin-needed
Comment 7•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•