Closed Bug 1098681 Opened 5 years ago Closed 5 years ago

TaskTracer should call GetCurTraceInfo() on message earlier

Categories

(Core :: Gecko Profiler, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: sinker, Assigned: shelly)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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
Blocks: 1100259
I haven't test it on the real device though...
Attachment #8523804 - Flags: review?(tlee)
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-
Attachment #8523804 - Attachment is obsolete: true
Attachment #8524269 - Flags: review?(tlee)
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+
Thanks for the tip! I'll keep that in mind :)

Try result:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ccfd28b915ce
No longer blocks: 1100259
Blocks: tasktracer
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/267d132d9737
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.