Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: sinker, Assigned: shelly)

Tracking

(Blocks 2 bugs)

unspecified
mozilla31
All
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(4 attachments, 55 obsolete attachments)

18.25 KB, patch
shelly
: review+
Details | Diff | Splinter Review
17.43 KB, patch
shelly
: review+
Details | Diff | Splinter Review
4.87 KB, patch
shelly
: review+
Details | Diff | Splinter Review
1.41 KB, patch
shelly
: review+
Details | Diff | Splinter Review
Reporter

Description

6 years ago
Task tracer is for tracing runnables/tasks spread over threads and processes.  It records information that is necessary for correlating runnables/tasks.  The basic idea is correlating all tasks created for an platform event; likes input event, timer event, network event, .... etc.  With the capability of correlating runnables/tasks, developers may understand how Gecko spends CPU time for an event in an easy way.
Reporter

Comment 1

6 years ago
Posted patch WIP - traced runnable (obsolete) — Splinter Review
This is the type of decorators of other nsIRunnables to logging activities of them.
Will this be useful to locate tasks that enter endless loop?
Will/can this be linked to the devtools?
Reporter

Comment 4

6 years ago
I think so for endless loop.  For the devtools, I had told with Vladan for this, and we also considering to integrate with Cleopatra, seems they are very relative.  Do you have any other idea?  For example, a better way or tools to integrate with.
Reporter

Updated

6 years ago
Assignee: nobody → cyu
Reporter

Updated

6 years ago
Blocks: 916409
Reporter

Updated

6 years ago
Blocks: 916410
Assignee

Comment 5

6 years ago
Posted patch WIP: A clean-up version (obsolete) — Splinter Review
Added comments and renamed some of the function names, and the goal for this revise is to make it understandable as much as possible.
Why not store the task data in the Gecko profiler's buffer? You would just need to call AddMarker instead of calling LogAction:

http://mxr.mozilla.org/mozilla-central/source/tools/profiler/nsIProfiler.idl#17

This way you get Cleopatra integration for free, you don't have to write the "data retriever" tool, and we can use the task tracing on multiple platforms :)
Reporter

Comment 7

6 years ago
We will finally call AddMarker, but for now Cleopatra still does not correlate markers.  So, it is not good for reading and studying data.  I am trying to find some one to work on Cleopatra to get a better representation for task tracer, maybe you know someone available can help us to do that.  Before that, for short term, a simple tool to retrieve data and write out a plain-text file is more helpful.
Thanks Thinker. Have you seen Benoit Girard's blog post about correlating stacks from different B2G processes? The same functionality is also available for markers

http://benoitgirard.wordpress.com/2013/09/18/efficient-multi-process-profiling-on-b2g/
This sounds great! Can we coordinate with the folks in bug 853864 to maybe share some code?
Posted patch Task tracer WIP. (obsolete) — Splinter Review
Changes:
* Move most logic into /tools/profiler and encapsulate the fact that we are wrapping the tasks run by nsThread and MessageLoop (maybe we should consider placing them in a separate folder).
** We are still littering in IPC channel and nsAppShell. This needs to be improved.
* An skeleton for logging using ProfileEntry (not complete yet).

Todo:
* Complete the implementation of logging.
* Implement log flushing (possibly creating a thread for this).
Attachment #796876 - Attachment is obsolete: true
Attachment #797202 - Attachment is obsolete: true
Attachment #805823 - Attachment is obsolete: true
Reporter

Comment 11

6 years ago
Comment on attachment 819273 [details] [diff] [review]
Task tracer WIP.

Review of attachment 819273 [details] [diff] [review]:
-----------------------------------------------------------------

::: tools/profiler/ProfileEntry.cpp
@@ +118,5 @@
>        LOGF("%c \'%c\'", mTagName, mTagChar); break;
>      case 'r': case 't':
>        LOGF("%c %f", mTagName, mTagFloat); break;
> +    case 'a':
> +      LOGF("%c %d", mTagName, mTagActivity->originTaskId); break;

Benoit, will this break Cleopatra?
Reporter

Updated

6 years ago
Attachment #819273 - Flags: feedback?(bjacob)
You want the other Benoit for profiler things.
Flags: needinfo?(bgirard)
Attachment #819273 - Flags: feedback?(bjacob) → feedback?(bgirard)
(In reply to Thinker Li [:sinker] from comment #11)
> Comment on attachment 819273 [details] [diff] [review]
> Task tracer WIP.
> 
> Review of attachment 819273 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: tools/profiler/ProfileEntry.cpp
> @@ +118,5 @@
> >        LOGF("%c \'%c\'", mTagName, mTagChar); break;
> >      case 'r': case 't':
> >        LOGF("%c %f", mTagName, mTagFloat); break;
> > +    case 'a':
> > +      LOGF("%c %d", mTagName, mTagActivity->originTaskId); break;
> 
> Benoit, will this break Cleopatra?

First, I don't think this to break Cleopatra because this added tag is not in the vocabulary the built-in profiler is using. This is at most some dead code to the profiler. The problem is: are we going to take this route and use the profiler framework for data output? We could be better off writing out the data on our own if reusing the profiler code doesn't bring us much benefit.
Other alternatives to Cleopatra for visualization for consideration:
* Visutal event tracer:
  http://www.janbambas.cz/firefox-detailed-event-tracer-about-timeline/
* perf timechart
  http://bloggingthemonkey.blogspot.com/2013/09/freedreno-update-moar-fps.html
Some thoughts about the above 2:

Generally, we don't want to (or at least try not to) spread instrumentation macro/function calls in the code base but instead work on the low-level code to make it more generic. So we don't expect to see things like MOZ_EVENT_TRACE_... in many necko files. But the about:timeline UI is worth taking a look at.

The SVG perf outputs lacks the interactivity we would like to see in this tool so I would not consider it for now.
Reporter

Comment 16

6 years ago
The event tracer seems really close to our requirement, but it does not(?) provide a way to serialize and persist data.  The data can not be redistributed.  Especially, for B2G, we don't run event tracer on the target devices.  So, the profiler approach is still a better solution as I know.  We could provide a extractor for desktop to generate data that can be handled by other tools; event tracer for example.  But, even event tracer would show the output of task tracer in a very un-organized way.  It don't show relation-ship of tasks, the most valuable part of task tracer.  For the short term, a plain text file format is better than any other file format since there are a lot of text processors there.

If the performance of profiler approach match our requirement, I should leverage it, and provide an extractor running on desktop to generate a text file for short term.
Sorry for the review delay. I was at a work week and now on vacation until next week. If this can't wait ask for Vladan otherwise I'll get to this next week. Sorry
Flags: needinfo?(bgirard)

Updated

6 years ago
See Also: → 836151
Comment on attachment 819273 [details] [diff] [review]
Task tracer WIP.

Review of attachment 819273 [details] [diff] [review]:
-----------------------------------------------------------------

Here's some feedback. Sorry for taking so long. You should be reusing the marker infrastructure. I've already done something similar to what you're doing with the paint markers. Here's an example:
http://people.mozilla.org/~bgirard/cleopatra/#report=dcaaf63dd8d844d576634c8e59d6677fa9d854e3

We need to reuse that instead of adding another piece of the profiler that will be difficult to maintain. They should support all the features you need. Tracing the activity should live entirely outside the profiler and should simply insert markers for the traced events. Then you can extend cleopatra to visualize your markers however you want by writing your custom code, or write an alternate UI for viewing the traced event.

::: dom/wifi/WifiProxyService.cpp
@@ +180,5 @@
>    if (NS_FAILED(rv)) {
>      NS_WARNING("Can't create wifi event thread");
>      return NS_ERROR_FAILURE;
>    }
> +  NS_SetThreadName(mEventThread, "wifi event");

All the thread naming bits in this patch should be broken out and landed separately (ideally in a different bug).

::: ipc/chromium/src/base/thread.cc
@@ +8,5 @@
>  #include "base/string_util.h"
>  #include "base/thread_local.h"
>  #include "base/waitable_event.h"
>  #include "GeckoProfiler.h"
> +#include "prthread.h"

Is this needed?

::: ipc/chromium/src/chrome/common/ipc_channel_posix.cc
@@ +708,5 @@
>        msg->set_fd_cookie(++last_pending_fd_id_);
>  #endif
>      }
> +#ifdef MOZ_TASK_TRACER
> +    if (*GetCurrentThreadTaskIdPtr()) {

avoid calling this twice by using: if (T a = *GetCurrentThreadTaskIdPtr())

::: tools/profiler/GeckoProfilerImpl.h
@@ +284,5 @@
>    // we only copy the strings at save time, so to take multiple parameters we'd need to copy them then.
>    SamplerStackFrameRAII(const char *aInfo, uint32_t line) {
>      mHandle = mozilla_sampler_call_enter(aInfo, this, false, line);
> +    mSamplerInfo = aInfo;
> +    mozilla::tasktracer::LogSamplerEnter(mSamplerInfo);

Sorry but I strongly disagree that we increase the overhead to these. We need to keep the overhead down so that we can continue to use these without worrying about the overhead.

::: tools/profiler/ProfileEntry.h
@@ +36,5 @@
>    ProfileEntry(char aTagName, uintptr_t aTagOffset);
>    ProfileEntry(char aTagName, Address aTagAddress);
>    ProfileEntry(char aTagName, int aTagLine);
>    ProfileEntry(char aTagName, char aTagChar);
> +  ProfileEntry(char aTagName, TracedActivity *aTagActivity);

We don't need a new ProfileEntry. The data you need to store is a marker. You should represented your tracked activity as a marker.

::: tools/profiler/platform.cpp
@@ +720,5 @@
>  bool mozilla_sampler_register_thread(const char* aName, void* stackTop)
>  {
> +#ifdef MOZ_TASK_TRACER
> +  if (aName) {
> +    PR_SetCurrentThreadName(aName);

The thread is responsible for registering it's own thread name and it may not be a PRThread. We can't do this.
Attachment #819273 - Flags: feedback?(bgirard) → feedback-
Assignee

Comment 19

6 years ago
Posted patch Part1: Add GeckoTaskTracer (obsolete) — Splinter Review
Assignee

Comment 20

6 years ago
Posted patch Part2: Add TracedCommon (obsolete) — Splinter Review
Assignee

Comment 21

6 years ago
Assignee

Comment 22

6 years ago
Assignee

Comment 23

6 years ago
Posted patch Part 5: Apply to IPC (obsolete) — Splinter Review
Assignee

Comment 26

6 years ago
Comment on attachment 8348595 [details] [diff] [review]
Part1: Add GeckoTaskTracer

Hi Kyle, we found you the best fit to review this one!

Please find the brief introduction of TaskTracer in comment 0, part 1 is good for reviews, but feel free to review the rest! Thanks :).
Attachment #8348595 - Flags: review?(khuey)
Assignee

Comment 27

6 years ago
Posted patch Part1: Add GeckoTaskTracer (obsolete) — Splinter Review
Attachment #8348595 - Attachment is obsolete: true
Attachment #8348596 - Attachment is obsolete: true
Attachment #8348595 - Flags: review?(khuey)
Attachment #8351131 - Flags: review?(khuey)
Assignee

Updated

6 years ago
Attachment #8351131 - Flags: feedback?(cyu)
Assignee

Comment 28

6 years ago
Posted patch Part2: Add TracedCommon (obsolete) — Splinter Review
Attachment #8351132 - Flags: review?(khuey)
Attachment #8351132 - Flags: feedback?(cyu)
Assignee

Comment 29

6 years ago
Posted patch Part1: Add GeckoTaskTracer (obsolete) — Splinter Review
Attachment #8351131 - Attachment is obsolete: true
Attachment #8351132 - Attachment is obsolete: true
Attachment #8351131 - Flags: review?(khuey)
Attachment #8351131 - Flags: feedback?(cyu)
Attachment #8351132 - Flags: review?(khuey)
Attachment #8351132 - Flags: feedback?(cyu)
Attachment #8355144 - Flags: review?(khuey)
Assignee

Comment 30

6 years ago
Posted patch Part2: Add TracedCommon (obsolete) — Splinter Review
Fixed the crash due to a change in nsThread.
Attachment #8355145 - Flags: review?(khuey)
(In reply to Shelly Lin [:shelly] from comment #30)
> Created attachment 8355145 [details] [diff] [review]
> Part2: Add TracedCommon
> 
> Fixed the crash due to a change in nsThread.

Member fields like TaskId, SourceEventId and SourceEventType and related methods are redundant in TracedRunnable and TracedTask. They should be refactored into a mixin class.
Assignee

Comment 32

6 years ago
Posted patch Part1: Add GeckoTaskTracer (obsolete) — Splinter Review
Attachment #8355144 - Attachment is obsolete: true
Attachment #8355145 - Attachment is obsolete: true
Attachment #8355144 - Flags: review?(khuey)
Attachment #8355145 - Flags: review?(khuey)
Attachment #8355415 - Flags: review?(khuey)
Attachment #8355415 - Flags: feedback?(cyu)
Assignee

Comment 33

6 years ago
Comment on attachment 8355415 [details] [diff] [review]
Part1: Add GeckoTaskTracer

Review of attachment 8355415 [details] [diff] [review]:
-----------------------------------------------------------------

::: tools/profiler/GeckoTaskTracerImpl.h
@@ +19,5 @@
> +};
> +
> +// Each thread owns a TracedInfo on its tread local storage, keeps track of
> +// currently-traced task and other information.
> +struct TracedInfo

Hey Kyle, we are debating whether we should use "TracedInfo" or "TraceInfo" here, what's your call?
Assignee

Comment 34

6 years ago
Posted patch Part2: Add TracedTaskCommon (obsolete) — Splinter Review
Attachment #8355416 - Flags: review?(khuey)
Attachment #8355416 - Flags: feedback?(cyu)
Having spent approximately 30 seconds reading the patch, I think TraceInfo makes more sense.
FWIW I hope to review this next week once I'm back from vacation.
Assignee

Comment 37

6 years ago
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #35)
> Having spent approximately 30 seconds reading the patch, I think TraceInfo
> makes more sense.
Ha, "TraceInfo" is the winner of our lunch hour face-to-face campaign too, enjoy your vacation and Happy Holidays :).
(In reply to Benoit Girard (:BenWa) from comment #18)
> Comment on attachment 819273 [details] [diff] [review]
> Task tracer WIP.
> 
> Review of attachment 819273 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Here's some feedback. Sorry for taking so long. You should be reusing the
> marker infrastructure. I've already done something similar to what you're
> doing with the paint markers. Here's an example:
> http://people.mozilla.org/~bgirard/cleopatra/
> #report=dcaaf63dd8d844d576634c8e59d6677fa9d854e3
> 
> We need to reuse that instead of adding another piece of the profiler that
> will be difficult to maintain. They should support all the features you
> need. Tracing the activity should live entirely outside the profiler and
> should simply insert markers for the traced events. Then you can extend
> cleopatra to visualize your markers however you want by writing your custom
> code, or write an alternate UI for viewing the traced event.
>
We are taking another route that doesn't reuse the profiler buffer and data output functions. We plan to output the TaskTracer data in custom format and use external tools to convert the data into JSON format because of the way the profiler buffers and outputs data. We can't drop data in the task tracer buffer and may need to flush the buffer before we run out of the buffer space, which means we'd better not use JS_Stringify() to output data.

> ::: tools/profiler/platform.cpp
> @@ +720,5 @@
> >  bool mozilla_sampler_register_thread(const char* aName, void* stackTop)
> >  {
> > +#ifdef MOZ_TASK_TRACER
> > +  if (aName) {
> > +    PR_SetCurrentThreadName(aName);
> 
> The thread is responsible for registering it's own thread name and it may
> not be a PRThread. We can't do this.
It's only a quick workaround to anonymous threads. This will be fixed.
Assignee

Comment 39

6 years ago
Posted patch Part1: Add GeckoTaskTracer (obsolete) — Splinter Review
Attachment #8348597 - Attachment is obsolete: true
Attachment #8348599 - Attachment is obsolete: true
Attachment #8348601 - Attachment is obsolete: true
Attachment #8348603 - Attachment is obsolete: true
Attachment #8355415 - Attachment is obsolete: true
Attachment #8355416 - Attachment is obsolete: true
Attachment #8355415 - Flags: review?(khuey)
Attachment #8355415 - Flags: feedback?(cyu)
Attachment #8355416 - Flags: review?(khuey)
Attachment #8355416 - Flags: feedback?(cyu)
Attachment #8355458 - Flags: review?(khuey)
Attachment #8355458 - Flags: feedback?(cyu)
Assignee

Comment 40

6 years ago
Posted patch Part2: Add TracedTaskCommon (obsolete) — Splinter Review
Correct naming.
Attachment #8355459 - Flags: review?(khuey)
Attachment #8355459 - Flags: feedback?(cyu)
Assignee

Comment 41

6 years ago
Assignee

Comment 42

6 years ago
Assignee

Comment 43

6 years ago
Posted patch Part 5: Apply to IPC (obsolete) — Splinter Review
Assignee

Comment 46

6 years ago
Posted patch Part1: Add GeckoTaskTracer (obsolete) — Splinter Review
Attachment #8348606 - Attachment is obsolete: true
Attachment #8355458 - Attachment is obsolete: true
Attachment #8355459 - Attachment is obsolete: true
Attachment #8355460 - Attachment is obsolete: true
Attachment #8355462 - Attachment is obsolete: true
Attachment #8355463 - Attachment is obsolete: true
Attachment #8355464 - Attachment is obsolete: true
Attachment #8355504 - Attachment is obsolete: true
Attachment #8355458 - Flags: review?(khuey)
Attachment #8355458 - Flags: feedback?(cyu)
Attachment #8355459 - Flags: review?(khuey)
Attachment #8355459 - Flags: feedback?(cyu)
Attachment #8356050 - Flags: review?(khuey)
Attachment #8356050 - Flags: feedback?(cyu)
Assignee

Comment 47

6 years ago
Posted patch Part2: Add TracedTaskCommon (obsolete) — Splinter Review
Attachment #8356051 - Flags: review?(khuey)
Attachment #8356051 - Flags: feedback?(cyu)
Assignee

Comment 48

6 years ago
It is a pain to maintain so many parts since the rest are still under development, but Part 1 and Part 2 are ready to review.
Assignee

Comment 49

6 years ago
Posted file profile.sh (obsolete) —
Assignee

Comment 51

6 years ago
- Add mouse event, power key, and home key to the SourceEvent.
- Add GetUserMedia to the SourceEvent.
(We might just want to track the "input" hardware event, such event is theoretically triggered by touch event.)
Comment on attachment 8356050 [details] [diff] [review]
Part1: Add GeckoTaskTracer

Generally this looks good. We have some platform-dependent stuff in the code, like __android_log_print(), code using gettid(), etc. It's OK for now since we only target at GONK for now. But we should strive for making things platform neutral.
Attachment #8356050 - Flags: feedback?(cyu)
Comment on attachment 8356051 [details] [diff] [review]
Part2: Add TracedTaskCommon

This looks good to me. Just one nit: AttachTraceInfo() and ClearTraceInfo() don't sound like a pair of opposite operations.
Attachment #8356051 - Flags: feedback?(cyu) → feedback+
(In reply to Cervantes Yu from comment #38)
> > We need to reuse that instead of adding another piece of the profiler that
> > will be difficult to maintain. They should support all the features you
> > need. Tracing the activity should live entirely outside the profiler and
> > should simply insert markers for the traced events. Then you can extend
> > cleopatra to visualize your markers however you want by writing your custom
> > code, or write an alternate UI for viewing the traced event.
> >
> We are taking another route that doesn't reuse the profiler buffer and data
> output functions. We plan to output the TaskTracer data in custom format and
> use external tools to convert the data into JSON format because of the way
> the profiler buffers and outputs data. We can't drop data in the task tracer
> buffer and may need to flush the buffer before we run out of the buffer
> space, which means we'd better not use JS_Stringify() to output data.

So after this patch, we would have profiler markers, the visual event tracer stuff:

http://mxr.mozilla.org/mozilla-central/source/xpcom/base/VisualEventTracer.h#5

and these task tracing bits.  Seems like we ought to be unifying things, not creating slightly-different-but-similar things.
Assignee

Comment 55

6 years ago
Hey Kyle, can you give them a quick review when your hands are free? Thanks!
Flags: needinfo?(khuey)
This is towards the top of my review queue.  It's a lot more involved than a quick review though :P
Flags: needinfo?(khuey)
Assignee

Comment 57

6 years ago
This is a mock of the front-end of TaskTracer - Isis.

Some of the functions are under development, and we are still tweaking the UI towards the best representation of tasks. Never then less, hopefully this mock can provide a good overview of how it should look like and work.
Attachment #8356405 - Attachment is obsolete: true
Assignee

Comment 58

6 years ago
(In reply to Cervantes Yu from comment #53)
> Comment on attachment 8356051 [details] [diff] [review]
> Part2: Add TracedTaskCommon
> 
> This looks good to me. Just one nit: AttachTraceInfo() and ClearTraceInfo()
> don't sound like a pair of opposite operations.

Thanks, I think so too, shall probably rename to SetTraceInfo to pair up with ClearTraceInfo.
Assignee

Comment 59

5 years ago
Attachment #8356050 - Attachment is obsolete: true
Attachment #8356051 - Attachment is obsolete: true
Attachment #8356056 - Attachment is obsolete: true
Attachment #8359149 - Attachment is obsolete: true
Attachment #8356050 - Flags: review?(khuey)
Attachment #8356051 - Flags: review?(khuey)
Attachment #8373930 - Flags: review?(khuey)
Assignee

Comment 61

5 years ago
Assignee

Comment 63

5 years ago
Comment on attachment 8373943 [details]
https://github.com/shellylin/gecko-dev/tree/tt-v3

https://github.com/shellylin/gecko-dev/tree/tt-v3
Attachment #8373943 - Attachment description: https://github.com/shellylin/gecko-dev/tree/tt-v4 → https://github.com/shellylin/gecko-dev/tree/tt-v3
Assignee

Comment 64

5 years ago
Attachment #8373943 - Attachment is obsolete: true
Component: General → Gecko Profiler
Product: Firefox OS → Core
Comment on attachment 8373930 [details] [diff] [review]
Part 1: The core of TaskTracer.

Review of attachment 8373930 [details] [diff] [review]:
-----------------------------------------------------------------

Nits mostly, looks pretty good.

One thing that does concern me is nested event loops.  It doesn't look like those are handled here.

::: tools/profiler/GeckoTaskTracer.cpp
@@ +40,5 @@
> +  return (pid_t) syscall(SYS_gettid);
> +}
> +#endif
> +
> +#define MAX_THREAD_NUM 64

This seems like something that will be exceeded eventually.

@@ +48,5 @@
> +namespace tasktracer {
> +
> +static TraceInfo sAllTraceInfo[MAX_THREAD_NUM];
> +static mozilla::ThreadLocal<TraceInfo*> sTraceInfo;
> +static pthread_mutex_t sTraceInfoLock = PTHREAD_MUTEX_INITIALIZER;

Is there a reason not to use one of our Mutex classes?  I don't like reviewing pthreads :P

@@ +104,5 @@
> +  if (!sTraceInfo.get()) {
> +    sTraceInfo.set(AllocTraceInfo(gettid()));
> +  }
> +
> +  return sTraceInfo.get();

How about

TraceInfo* info = sTraceInfo.get();
if (!info) {
  info = AllocTraceInfo();
  sTraceInfo.set(info);
}

return info;

to avoid doing two TLS gets on every lookup.

@@ +117,5 @@
> +  pid_t tid = gettid();
> +  uint64_t taskid = ((uint64_t)tid << 32) | ++info->mLastUniqueTaskId;
> +  return taskid;
> +
> +}

nit: extra \n before }

@@ +143,5 @@
> +}
> +
> +void
> +SetCurTraceInfo(uint64_t aSourceEventId, uint64_t aParentTaskId,
> +                uint32_t aSourceEventType)

Why not just take a SourceEventType argument?

@@ +155,5 @@
> +}
> +
> +void
> +GetCurTraceInfo(uint64_t* aOutSourceEventId, uint64_t* aOutParentTaskId,
> +                uint32_t* aOutSourceEventType)

Same here.

::: tools/profiler/GeckoTaskTracer.h
@@ +9,5 @@
> +
> +/**
> + * TaskTracer provides a way to trace the correlation between tasks, as opposed
> + * to sample base profilers, TaskTracer is used for finding out where a task is
> + * dispatched from, which source event of this task is.

How about

TaskTracer provides a way to trace the correlation between different tasks, across threads and processes.  Unlike sampling based profilers, TaskTracer can tell you where a task is dispatched from, what it's original source was, how long it spent in the event queue, whatever else you want to brag about ...

@@ +12,5 @@
> + * to sample base profilers, TaskTracer is used for finding out where a task is
> + * dispatched from, which source event of this task is.
> + *
> + * Source Events are usually some kinds of I/O events we're interested in, such
> + * as touch event, timer event, network event... etc. When a source event is

events, events, events

@@ +16,5 @@
> + * as touch event, timer event, network event... etc. When a source event is
> + * created, it also generates a chain of Tasks/nsRunnables, dispatched to
> + * different threads and processes. TaskTracer records information of each tasks
> + * (latency, execution time... etc.), and which source event is this task
> + * originally dispatched from.

When a source event is created, TaskTracer records the entire chain of Tasks and nsRunnables as they are dispatched to different threads and processes.  It records latency, execution time, etc. for each Task and nsRunnable that chains back to the original source event.

@@ +44,5 @@
> + */
> +
> +// Create a source event of aType. Make sure to pair up with DestroySourceEvent.
> +void CreateSourceEvent(SourceEventType aType);
> +void DestroySourceEvent();

This should be some sort of RAII helper so that you can't forget to DestroySourceEvent.

@@ +58,5 @@
> +// Wrap aTask at where message loops post tasks.
> +Task *CreateTracedTask(Task *aTask);
> +
> +// Wrap aRunnable at where threads dispatch events.
> +nsIRunnable *CreateTracedRunnable(nsIRunnable *aRunnable);

Drop the comments.  It's obvious what these do.

@@ +60,5 @@
> +
> +// Wrap aRunnable at where threads dispatch events.
> +nsIRunnable *CreateTracedRunnable(nsIRunnable *aRunnable);
> +
> +// Free the TraceInfo allocated on its tread local storage.

*thread* local storage.  This is a common misspelling in these patches.

::: tools/profiler/GeckoTaskTracerImpl.h
@@ +19,5 @@
> +  uint64_t mCurTaskId; // Task Id of currently running task.
> +  uint64_t mSavedCurTraceSourceId;
> +  uint64_t mSavedCurTaskId;
> +
> +  SourceEventType mCurTraceSourceType; // Source event type of currently running task.

The comments on these variables are just duplicating the variable names.

@@ +31,5 @@
> +
> +// Initialize the TaskTracer.
> +void InitTaskTracer();
> +
> +// Return true if TaskTracer has successfully initialized and setup.

s/has/was/

@@ +35,5 @@
> +// Return true if TaskTracer has successfully initialized and setup.
> +bool IsInitialized();
> +
> +// Return the TraceInfo of current thread, allocate a new one if not exit.
> +TraceInfo* GetTraceInfo();

In Gecko we generally name functions like this GetOrCreateFoo.

@@ +39,5 @@
> +TraceInfo* GetTraceInfo();
> +
> +uint64_t GenNewUniqueTaskId();
> +
> +// Make sure to pair up with RestorePrevTraceInfo() if called.

This also might want an RAII helper.

::: tools/profiler/TracedTaskCommon.cpp
@@ +27,5 @@
> +  // enough source events setup, this should go away eventually, or hopefully.
> +//  if (!info->mCurTraceTaskId) {
> +//    info->mCurTraceTaskId = mTaskId;
> +//    info->mCurTraceTaskType = mSourceEventType;
> +//  }

Remove this?

@@ +67,5 @@
> +TracedRunnable::TracedRunnable(nsIRunnable *aFactualObj)
> +  : TracedTaskCommon()
> +  , mFactualObj(aFactualObj)
> +{
> +  LogVirtualTablePtr(mTaskId, mSourceEventId, *(int**)(aFactualObj));

Not scary at all :P

@@ +118,5 @@
> + * Public functions of GeckoTaskTracer.
> + *
> + * CreateTracedRunnable() returns a new nsIRunnable pointer wrapped by
> + * TracedRunnable, aRunnable is the original runnable object where now stored
> + * by this TracedRunnable.

CreateTracedRunnable() returns a TracedRunnable wrapping the original nsIRunnable object, aRunnable.

@@ +121,5 @@
> + * TracedRunnable, aRunnable is the original runnable object where now stored
> + * by this TracedRunnable.
> + *
> + * CreateTracedTask() returns a new Task pointer wrapped by TracedTask, aTask
> + * is the original task object where now stored by this TracedTask.

Move this down to CreateTracedTask, and rewrite it similarly.

@@ +128,5 @@
> +CreateTracedRunnable(nsIRunnable *aRunnable)
> +{
> +  if (IsInitialized()) {
> +    TracedRunnable* runnable = new TracedRunnable(aRunnable);
> +    return runnable;

This needs to be AddRefed.  In fact, this function should probably return already_AddRefed<nsIRunnable>.

::: tools/profiler/TracedTaskCommon.h
@@ +22,5 @@
> +  virtual ~TracedTaskCommon() {}
> +
> +protected:
> +  // Allocates a TraceInfo for the current thread on its thread local storage
> +  // if not exist; Sets the source event Id and source event type of this

if it does not already exist.  If it does exist, sets the ... from the TraceInfo of the current thread.

@@ +27,5 @@
> +  // runnable to the currently-traced task Id and task type from the TraceInfo
> +  // of current thread.
> +  void Setup();
> +
> +  // Before calling the Run() of its factual object, sets the currently-traced

I think you should use the term 'original' instead of 'factual' throughout these patches.

@@ +30,5 @@
> +
> +  // Before calling the Run() of its factual object, sets the currently-traced
> +  // task Id from the TraceInfo of current thread, to its source event Id.
> +  // Setup other information if needed. Should call ClearTraceInfo() to reset
> +  // them when done.

Sets up the metadata on the current thread's TraceInfo for this task.  After Run(), ClearTraceInfo is called to reset the metadata.

@@ +48,5 @@
> +  SourceEventType mSourceEventType;
> +};
> +
> +class TracedRunnable : public TracedTaskCommon
> +                     , public nsIRunnable

inherit from nsRunnable

@@ +51,5 @@
> +class TracedRunnable : public TracedTaskCommon
> +                     , public nsIRunnable
> +{
> +public:
> +  NS_DECL_THREADSAFE_ISUPPORTS

Then you don't need to redeclare nsISupports.

@@ +54,5 @@
> +public:
> +  NS_DECL_THREADSAFE_ISUPPORTS
> +  NS_DECL_NSIRUNNABLE
> +
> +  TracedRunnable(nsIRunnable *aFactualObj);

nit: stars to the left, here and everywhere

@@ +56,5 @@
> +  NS_DECL_NSIRUNNABLE
> +
> +  TracedRunnable(nsIRunnable *aFactualObj);
> +
> +  virtual ~TracedRunnable() {}

Refcounted objects should have protected or private dtors.
Attachment #8373930 - Flags: review?(khuey)
Comment on attachment 8373931 [details] [diff] [review]
Part 2: Wrap the Task and nsRunnable with TracedTask and TracedRunnable.

Review of attachment 8373931 [details] [diff] [review]:
-----------------------------------------------------------------

Obviously the SetCurrentProcessPrivileges bit is bogus ;-)

::: ipc/chromium/src/base/message_loop.cc
@@ +296,5 @@
>      bool nestable) {
> +
> +#ifdef MOZ_TASK_TRACER
> +  task = mozilla::tasktracer::CreateTracedTask(task);
> +#endif

Does TaskThrottler::PostTask need this too?

::: ipc/chromium/src/base/process_util_linux.cc
@@ +271,2 @@
>      return;
>    }

uh ...

::: ipc/chromium/src/chrome/common/ipc_message.h
@@ +340,5 @@
> +#ifdef MOZ_TASK_TRACER
> +    uint64_t source_event_id;
> +    uint64_t parent_task_id;
> +    uint32_t source_event_type;
> +#endif

I really hope nothing makes assumptions about the length of Messages.

::: xpcom/threads/nsThreadPool.cpp
@@ +94,5 @@
>                                      getter_AddRefs(thread));
>    if (NS_WARN_IF(!thread))
>      return NS_ERROR_UNEXPECTED;
>  
> +  NS_SetThreadName(thread, mName);

Why is this here?
Attachment #8373931 - Flags: review?(khuey) → review-
Assignee

Comment 67

5 years ago
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #65)
> Comment on attachment 8373930 [details] [diff] [review]
> Part 1: The core of TaskTracer.
> 
> Review of attachment 8373930 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nits mostly, looks pretty good.
> 
> One thing that does concern me is nested event loops.  It doesn't look like
> those are handled here.
> 

Many thanks to your reviews!

As for the nested event loops, our plan is to let the data converter (Bug 956620) do some special mark ups or filter at them, and let the front-end "Isis" present something like the mock below:
http://people.mozilla.org/~slin/tasktracer/nestedloop.svg
Assignee

Comment 68

5 years ago
Turns out that the mock above might not be suitable enough to present nested event loops. Event loops can have event loops, making other tasks outside of a event loop tangled with descendant tasks of a event loop at present. Still trying to figure out a better way to show.
Assignee

Comment 69

5 years ago
Comment on attachment 8373930 [details] [diff] [review]
Part 1: The core of TaskTracer.

Review of attachment 8373930 [details] [diff] [review]:
-----------------------------------------------------------------

::: tools/profiler/TracedTaskCommon.cpp
@@ +128,5 @@
> +CreateTracedRunnable(nsIRunnable *aRunnable)
> +{
> +  if (IsInitialized()) {
> +    TracedRunnable* runnable = new TracedRunnable(aRunnable);
> +    return runnable;

I had tried making it ref counted, and did the following at where we wrapped the event in nsThread
event = CreateTracedRunnable(event).get();

But then it fail at PutEvent(event, ...), where it tries to AddRef to the event.
Assignee

Comment 70

5 years ago
Is that because the nsIRunnable* event in |nsThread::DispatchInternal| is not ref counted?
Assignee

Comment 71

5 years ago
Attachment #8373930 - Attachment is obsolete: true
Attachment #8382131 - Flags: review?(khuey)
Assignee

Comment 72

5 years ago
Comment on attachment 8382131 [details] [diff] [review]
Part 1: The core of TaskTracer.

Review of attachment 8382131 [details] [diff] [review]:
-----------------------------------------------------------------

::: tools/profiler/TracedTaskCommon.cpp
@@ +112,5 @@
> +nsIRunnable*
> +CreateTracedRunnable(nsIRunnable* aRunnable)
> +{
> +  TracedRunnable* runnable = new TracedRunnable(aRunnable);
> +  return runnable;

I had tried

already_AddRefed<nsIRunnable>
CreateTracedRunnable(nsIRunnable* aRunnable)
{
  nsCOMPtr<TracedRunnable> runnable = new TracedRunnable(aRunnable);
  return runnable.forget();
}

But couldn't figure out how should I adjust inside 

nsThread::DispatchInternal(nsIRunnable *event, uint32_t flags,
                           nsNestedEventTarget *target)

Where I intend to wrap |event| with TracedRunnable Orz...
(In reply to Shelly Lin [:shelly] from comment #72)
> 
> already_AddRefed<nsIRunnable>
> CreateTracedRunnable(nsIRunnable* aRunnable)
> {
>   nsCOMPtr<TracedRunnable> runnable = new TracedRunnable(aRunnable);
>   return runnable.forget();
> }
> 
We hit a pitfall here: nsCOMPtr<TracedRunnable>::forget() to return already_AddRefed<nsIRunnable> crashes if TracedRunnable has nsRunnable as the 2nd parent class. The offset is added twice and already_AddRefed<nsIRunnable> contains a wrong mRawPtr.
Assignee

Comment 74

5 years ago
Attachment #8373931 - Attachment is obsolete: true
Attachment #8373932 - Attachment is obsolete: true
Attachment #8382131 - Attachment is obsolete: true
Attachment #8382131 - Flags: review?(khuey)
Assignee

Comment 76

5 years ago
Assignee

Updated

5 years ago
Attachment #8384438 - Flags: review?(khuey)
Assignee

Updated

5 years ago
Attachment #8384439 - Flags: review?(khuey)
Assignee

Comment 77

5 years ago
Comment on attachment 8384439 [details] [diff] [review]
Part 2: Wrap Task and nsRunnable with TracedTask and TracedRunnable.

Review of attachment 8384439 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/TaskThrottler.cpp
@@ +25,5 @@
>                          CancelableTask* aTask, const TimeStamp& aTimeStamp)
>  {
> +#ifdef MOZ_TASK_TRACER
> +  aTask = static_cast<CancelableTask*>(
> +            mozilla::tasktracer::CreateTracedTask(aTask));

Okay, turns out that we don't care about such task, since this aTask is not posted to the message loop.
Assignee

Comment 78

5 years ago
Attachment #8384438 - Attachment is obsolete: true
Attachment #8384439 - Attachment is obsolete: true
Attachment #8384438 - Flags: review?(khuey)
Attachment #8384439 - Flags: review?(khuey)
Attachment #8386723 - Flags: review?(khuey)
Assignee

Comment 79

5 years ago
Remove the wrap in TaskThrottler.
Attachment #8386725 - Flags: review?(khuey)
Kyle, thanks for CC me on this!  Wish it were sooner tho, this is so close to what I want to do my self (and actually started already, just didn't have a good demo to show).  I'll look more in detail to this soon, just a note that my tracker wanted to go even further.
Looking closer at the screenshot, this is almost the same what my (existing) Visual Even Tracer does...  Aren't you reinventing a wheel?
The screenshot is not really an accurate representation of where this is today.  They have links between events and IPC messages working, even across processes.
Assignee

Comment 83

5 years ago
The similar part between Visual Event Tracer and TaskTracer is them both record the "time of a task sits in the queue" and "time of execution". Visual Event Tracer is a sampling based profiler, however, TaskTracer gives you the correlation between tasks, across threads and processes. Furthermore, TaskTracer doesn't need to add "labels" in tasks/runnables everywhere in codes for recording latency and execution time.

So, to be clear, if you look at a task/runnable from Isis, TaskTracer tells you:

1. Its original "source".
2. Where it is dispatched from.

If Visual Event Tracer is doing the two above as well, please let us know :)

Cheers,
Shelly
Visual Event Tracer doesn't connect stuff together, no.  I'm about to start a private thread on this.  We both tend to the same thing and we should synchronize.
Assignee

Comment 85

5 years ago
This is for the purpose of connecting timer events and their dispatchers.
Attachment #8397743 - Flags: review?(khuey)
Assignee

Comment 86

5 years ago
Posted patch Part 4: Add source events (obsolete) — Splinter Review
So far the implementation of adding source events like touch, mouse, key, unixsocket, and wifi events are pretty solid now.
Attachment #8384440 - Attachment is obsolete: true
Attachment #8397745 - Flags: review?(khuey)
Comment on attachment 8386723 [details] [diff] [review]
Part 1: The core of TaskTracer.

Review of attachment 8386723 [details] [diff] [review]:
-----------------------------------------------------------------

I had enough comments here that I'd like to see it again.

::: tools/profiler/GeckoTaskTracer.cpp
@@ +51,5 @@
> +namespace tasktracer {
> +
> +static mozilla::ThreadLocal<TraceInfo*> sTraceInfoTLS;
> +static StaticMutex sMutex;
> +static nsTArray<nsAutoPtr<TraceInfo> > sTraceInfos;

>> is supported on all of our compilers now.

static XPCOM classes (such as nsTArray) are not allowed.  Make this a pointer and allocate it at startup.

@@ +66,5 @@
> +  aInfo->mThreadName.SetIsVoid(true);
> +
> +  nsAutoPtr<TraceInfo>* info = sTraceInfos.AppendElement(aInfo);
> +
> +  if (!info->get()) {

nsTArray is infallible by default.  This cannot fail.

@@ +85,5 @@
> +    i--;
> +    if (sTraceInfos[i]->mThreadId == aTid) {
> +      sTraceInfos.RemoveElementAt(i);
> +      break;
> +    }

Maybe sTraceInfos should be a hashtable for O(1) insertion/removal, but creating/destroying threads should be rare.

@@ +88,5 @@
> +      break;
> +    }
> +  }
> +
> +}

Extra \n above the last }

@@ +150,5 @@
> +
> +  // Restore the previously saved source event info.
> +  RestoreCurTraceInfo();
> +}
> +} // namespace anonymous

Put a \n above the } that ends the namespace.

@@ +245,5 @@
> +  // Log format:
> +  // [0 taskId dispatchTime sourceEventId sourceEventType parentTaskId]
> +  TTLOG("%d %lld %lld %lld %d %lld",
> +        ACTION_DISPATCH, aTaskId, PR_Now(), aSourceEventId, aSourceEventType,
> +        aParentTaskId);

I think you should convert TTLOG to use the profiler markers.  Benwa can explain more here, but I'm fine with doing this in a followup.

@@ +259,5 @@
> +    info->mThreadName = EmptyCString();
> +    if (getpid() == gettid()) {
> +      if (XRE_GetProcessType() == GeckoProcessType_Default) {
> +        info->mThreadName.AppendASCII("b2g");
> +      }

I'd prefer that you did not hardcode in b2g here ... we'll want to be able to use this tool on desktop.

Also, what happens here if you're on the main thread of a child process?

::: tools/profiler/GeckoTaskTracer.h
@@ +40,5 @@
> +  UNIXSOCKET,
> +  WIFI
> +};
> +
> +class CreateSourceEventRAII

AutoCreateSourceEvent (or maybe even AutoSourceEvent)

@@ +60,5 @@
> +
> +already_AddRefed<nsIRunnable> CreateTracedRunnable(nsIRunnable* aRunnable);
> +
> +// Free the TraceInfo allocated on its TLS.
> +void FreeTraceInfo();

What actually calls this?

::: tools/profiler/GeckoTaskTracerImpl.h
@@ +18,5 @@
> +namespace mozilla {
> +namespace tasktracer {
> +
> +struct TraceInfo
> +{

This needs a constructor/destructor with MOZ_COUNT_CTOR/DTOR macros.

@@ +46,5 @@
> +{
> +public:
> +  SaveCurTraceInfoRAII();
> +  ~SaveCurTraceInfoRAII();
> +};

AutoSaveCurrentTraceInfo

@@ +57,5 @@
> +
> +enum {
> +  TYPE_THREAD,
> +  TYPE_PROCESS
> +};

Give enum this a name.

@@ +59,5 @@
> +  TYPE_THREAD,
> +  TYPE_PROCESS
> +};
> +
> +void SetThreadName(const char* aName, int aType = TYPE_THREAD);

And then aType a real type.  Also this should be something other than SetThreadName if you can use it on something other than threads.
Attachment #8386723 - Flags: review?(khuey)
Comment on attachment 8386725 [details] [diff] [review]
Part 2: Wrap Task and nsRunnable with TracedTask and TracedRunnable.

Review of attachment 8386725 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/ContentChild.cpp
@@ +479,5 @@
>  
> +#ifdef MOZ_TASK_TRACER
> +  mozilla::tasktracer::SetThreadName(NS_LossyConvertUTF16toASCII(aName).get(),
> +                                     mozilla::tasktracer::TYPE_PROCESS);
> +#endif

Aha, this is what handles the main thread case in the child.

Indent this properly please.

::: xpcom/threads/nsThread.cpp
@@ +295,5 @@
>    self->SetObserver(nullptr);
>  
> +#ifdef MOZ_TASK_TRACER
> +  FreeTraceInfo();
> +#endif

And this is where that call comes from, great.

Do we have any non-nsThread threads that we need to worry about?  I hope not.

@@ +401,5 @@
>  
> +#ifdef MOZ_TASK_TRACER
> +  nsRefPtr<nsIRunnable> event_ = CreateTracedRunnable(event);
> +  event = event_;
> +#endif

Put this in a nested scope (extra braces { } ) so that event_ can't be accidentally used later.
Attachment #8386725 - Flags: review?(khuey) → review+
Comment on attachment 8397743 [details] [diff] [review]
Part 3: Create a new wrapper TracedDummy for TimerEvents

Review of attachment 8397743 [details] [diff] [review]:
-----------------------------------------------------------------

::: tools/profiler/TracedTaskCommon.h
@@ +69,5 @@
>  private:
>    Task* mOriginalObj;
>  };
>  
> +class TracedDummy : public TracedTaskCommon

I would call this FakeTracedTask.

@@ +77,5 @@
> +  void BeginTracedDummy();
> +  void EndTracedDummy();
> +};
> +
> +class RunTracedDummyRAII

Same bit about AutoBlah instead of BlahRAII
Attachment #8397743 - Flags: review?(khuey) → review+
Blocks: 992454
Comment on attachment 8397745 [details] [diff] [review]
Part 4: Add source events

Review of attachment 8397745 [details] [diff] [review]:
-----------------------------------------------------------------

I think mwu should review the appshell bits.
Attachment #8397745 - Flags: review?(mwu)
Attachment #8397745 - Flags: review?(khuey)
Attachment #8397745 - Flags: review+
Comment on attachment 8397745 [details] [diff] [review]
Part 4: Add source events

Can we put this tracing higher up in the stack so we don't need to implement this for every widget backend? Maybe http://dxr.mozilla.org/mozilla-central/source/view/src/nsViewManager.cpp#729 ? I have no idea if that's an appropriate place, but putting it in widget code doesn't seem ideal.
Attachment #8397745 - Flags: review?(mwu)
Assignee

Comment 92

5 years ago
Comment on attachment 8386723 [details] [diff] [review]
Part 1: The core of TaskTracer.

Review of attachment 8386723 [details] [diff] [review]:
-----------------------------------------------------------------

::: tools/profiler/GeckoTaskTracer.cpp
@@ +259,5 @@
> +    info->mThreadName = EmptyCString();
> +    if (getpid() == gettid()) {
> +      if (XRE_GetProcessType() == GeckoProcessType_Default) {
> +        info->mThreadName.AppendASCII("b2g");
> +      }

It's hardcoded as "main" in my github version.

Anyhow, I actually dislike the whole name-setting implementation, I don't like to keep this log function too busy, nor log the process names and thread names in each task entry. We did this because there's no way to tell when TaskTracer has started, but with BenWa's help and the integration with Profiler, maybe we can approach the needs.
Assignee

Comment 93

5 years ago
Comment on attachment 8386725 [details] [diff] [review]
Part 2: Wrap Task and nsRunnable with TracedTask and TracedRunnable.

Review of attachment 8386725 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/threads/nsThread.cpp
@@ +295,5 @@
>    self->SetObserver(nullptr);
>  
> +#ifdef MOZ_TASK_TRACER
> +  FreeTraceInfo();
> +#endif

Hmm...Since we are creating TracedTask at the PostTask of message_loop, and IO threads are using "ipc/chromium/src/base/thread.cc", I think we should add the FreeTraceInfo here as well:

http://dxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/thread.cc#142
Assignee

Updated

5 years ago
No longer blocks: 916409, 916410, 956620, 992454
Assignee

Updated

5 years ago
Blocks: tasktracer
Assignee

Comment 94

5 years ago
Posted patch Part1: Add GeckoTaskTracer (obsolete) — Splinter Review
Since spamming adb logcat is not a good idea, I have separated the logging implementation (bug 992454) and the rest here.

A more clean up version, please find my comments inline.
Attachment #8362337 - Attachment is obsolete: true
Attachment #8373947 - Attachment is obsolete: true
Attachment #8386723 - Attachment is obsolete: true
Attachment #8386725 - Attachment is obsolete: true
Attachment #8397743 - Attachment is obsolete: true
Attachment #8397745 - Attachment is obsolete: true
Attachment #8408902 - Flags: review?(khuey)
Assignee

Updated

5 years ago
Assignee: cyu → slin
Assignee

Comment 95

5 years ago
I've merged the part of timer events into this patch.
Attachment #8408905 - Flags: review?(khuey)
Assignee

Comment 96

5 years ago
Address mwu's comment.
Attachment #8408907 - Flags: review?(khuey)
Assignee

Comment 97

5 years ago
Comment on attachment 8408907 [details] [diff] [review]
Part 3: Add source events for TaskTracer

Hi mwu, it does look cleaner of moving the tracking of input events to nsViewManager::DispatchEvent(), thanks for the feedback last time:)
Attachment #8408907 - Flags: review?(mwu)
Assignee

Comment 98

5 years ago
Missed the #ifdef MOZ_TASK_TRACER in nsTimerImpl.h.
Attachment #8408905 - Attachment is obsolete: true
Attachment #8408905 - Flags: review?(khuey)
Attachment #8408924 - Flags: review?(khuey)
Assignee

Comment 99

5 years ago
Comment on attachment 8408924 [details] [diff] [review]
Part 2: Wrap runnables, tasks and timer events with TaskTracer events

Review of attachment 8408924 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/threads/nsThread.cpp
@@ +455,5 @@
>    }
>  
> +#ifdef MOZ_TASK_TRACER
> +  nsRefPtr<nsIRunnable> tracedRunnable = CreateTracedRunnable(event);
> +  event = tracedRunnable;

event = tracedRunnable is assign_assuming_AddRef, so putting {} around these two lines will destruct the value of event which assigned from tracedRunnable.
Assignee

Comment 100

5 years ago
Build flag for turning on TaskTracer.

To enable, add "export MOZ_TASK_TRACER=1" in .userconfig
Attachment #8408938 - Flags: review?(khuey)
Comment on attachment 8408907 [details] [diff] [review]
Part 3: Add source events for TaskTracer

Not sure who the right reviewer for view/src/nsViewManager.cpp is, but it's not me. Maybe smaug?
Attachment #8408907 - Flags: review?(mwu) → review?(bugs)
Comment on attachment 8408907 [details] [diff] [review]
Part 3: Add source events for TaskTracer

I would put that code to PresShell.
(I wish we could at some point get rid of ViewManager, or at least the 
event handling bits there.)
Attachment #8408907 - Flags: review?(bugs) → review-
Comment on attachment 8408924 [details] [diff] [review]
Part 2: Wrap runnables, tasks and timer events with TaskTracer events

Review of attachment 8408924 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/threads/nsThread.cpp
@@ +455,5 @@
>    }
>  
> +#ifdef MOZ_TASK_TRACER
> +  nsRefPtr<nsIRunnable> tracedRunnable = CreateTracedRunnable(event);
> +  event = tracedRunnable;

Ah, yes, that's true.  tracedRunnable is the only thing holding the runnable alive :/  Guess we can't clean up the scope here then.
Attachment #8408924 - Flags: review?(khuey) → review+
Comment on attachment 8408907 [details] [diff] [review]
Part 3: Add source events for TaskTracer

Review of attachment 8408907 [details] [diff] [review]:
-----------------------------------------------------------------

I already gave you an r+ for the parts of this I feel comfortable reviewing :P
Attachment #8408907 - Flags: review?(khuey) → review+
Comment on attachment 8408902 [details] [diff] [review]
Part1: Add GeckoTaskTracer

Review of attachment 8408902 [details] [diff] [review]:
-----------------------------------------------------------------

::: tools/profiler/GeckoTaskTracer.cpp
@@ +119,5 @@
> +
> +void
> +InitTaskTracer()
> +{
> +  sTraceInfos = new nsClassHashtable<nsUint32HashKey, TraceInfo>();

MOZ_ASSERT(!sTraceInfos);

::: tools/profiler/GeckoTaskTracer.h
@@ +9,5 @@
> +
> +#include "nsCOMPtr.h"
> +
> +/**
> + * TaskTracer provides a way to trace the correlation between different tasks,

no comma.

@@ +11,5 @@
> +
> +/**
> + * TaskTracer provides a way to trace the correlation between different tasks,
> + * across threads and processes. Unlike sampling based profilers, TaskTracer can
> + * tell you where a task is dispatched from, what it's original source was, how

its, not it's.

@@ +12,5 @@
> +/**
> + * TaskTracer provides a way to trace the correlation between different tasks,
> + * across threads and processes. Unlike sampling based profilers, TaskTracer can
> + * tell you where a task is dispatched from, what it's original source was, how
> + * long it spent in the event queue, and how long it spent on execution.

it waited in the event queue ... it took to execute.
Attachment #8408902 - Flags: review?(khuey) → review+
Assignee

Comment 106

5 years ago
Nit fixed. Carry r+ from :khuey.
Attachment #8408902 - Attachment is obsolete: true
Attachment #8408907 - Attachment is obsolete: true
Attachment #8408924 - Attachment is obsolete: true
Attachment #8408938 - Attachment is obsolete: true
Attachment #8409554 - Flags: review+
Assignee

Comment 107

5 years ago
Update patch comment, carry r+ from :khuey.
Attachment #8409554 - Attachment is obsolete: true
Attachment #8409556 - Flags: review+
Assignee

Comment 108

5 years ago
Update patch comment, carry r+ from :khuey.
Attachment #8409557 - Flags: review+
Assignee

Comment 109

5 years ago
PresShell::HandleEvent is such a big function, so I thought it is better to group my implementation together than scatter #define all over places.

Olli, could you take a look at the part of nsPresShell.cpp? Thanks so much :)
Attachment #8409562 - Flags: review?(bugs)
Assignee

Comment 110

5 years ago
Update patch comment, carry r+ from :khuey.
Attachment #8409563 - Flags: review+
Assignee

Comment 111

5 years ago
Nit fix per try server errors.
Attachment #8409556 - Attachment is obsolete: true
Attachment #8410018 - Flags: review+
Assignee

Comment 112

5 years ago
Nit fix per try server errors.
Attachment #8409563 - Attachment is obsolete: true
Attachment #8410021 - Flags: review+
Have you profiled how much AutoSourceEvent takes cpu time?
I think it should be close to no-op by default and controlled using a pref
(pref value should be cached)
Comment on attachment 8409562 [details] [diff] [review]
Part 3: Add source events for TaskTracer

r+ for the presshell part, assuming AutoSourceEvent isn't too slow even
on b2g.

I do wonder why SourceEvent can't handle all the keyCodes.
It would be easier and compatible with future devices if one could just
pass keyCode to AutoSourceEvent.
Attachment #8409562 - Flags: review?(bugs) → review+
Assignee

Comment 116

5 years ago
Nit fixed.
Attachment #8409557 - Attachment is obsolete: true
Attachment #8409562 - Attachment is obsolete: true
Attachment #8410018 - Attachment is obsolete: true
Attachment #8410021 - Attachment is obsolete: true
Attachment #8410814 - Flags: review+
Assignee

Comment 118

5 years ago
Comment addressed, nit fixed.
Attachment #8410816 - Flags: review+
Assignee

Comment 120

5 years ago
(In reply to Olli Pettay [:smaug] from comment #115)
> Comment on attachment 8409562 [details] [diff] [review]
> Part 3: Add source events for TaskTracer
> 
> r+ for the presshell part, assuming AutoSourceEvent isn't too slow even
> on b2g.
> 
> I do wonder why SourceEvent can't handle all the keyCodes.
> It would be easier and compatible with future devices if one could just
> pass keyCode to AutoSourceEvent.

You're right, I've already make it take all kinds of keyCode.

As for the pref-controlling, I think I'll make it turned off by default in the configure file, will file another bug about the pref-controlling later.

Thanks :)
Assignee

Updated

5 years ago
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.