Closed
Bug 908995
Opened 11 years ago
Closed 10 years ago
[B2G] Task tracer
Categories
(Core :: Gecko Profiler, defect)
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: sinker, Assigned: shelly)
References
()
Details
Attachments
(4 files, 55 obsolete files)
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 |
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•11 years ago
|
||
This is the type of decorators of other nsIRunnables to logging activities of them.
Comment 2•11 years ago
|
||
Comment 3•11 years ago
|
||
Will this be useful to locate tasks that enter endless loop? Will/can this be linked to the devtools?
Reporter | ||
Comment 4•11 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•11 years ago
|
Assignee: nobody → cyu
Assignee | ||
Comment 5•11 years ago
|
||
Added comments and renamed some of the function names, and the goal for this revise is to make it understandable as much as possible.
Comment 6•11 years ago
|
||
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•11 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.
Comment 8•11 years ago
|
||
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?
Comment 10•11 years ago
|
||
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•11 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•11 years ago
|
Attachment #819273 -
Flags: feedback?(bjacob)
Updated•11 years ago
|
Attachment #819273 -
Flags: feedback?(bjacob) → feedback?(bgirard)
Comment 13•11 years ago
|
||
(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.
Comment 14•11 years ago
|
||
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
Comment 15•11 years ago
|
||
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•11 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.
Comment 17•11 years ago
|
||
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)
Comment 18•11 years ago
|
||
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•10 years ago
|
||
Assignee | ||
Comment 20•10 years ago
|
||
Assignee | ||
Comment 21•10 years ago
|
||
Assignee | ||
Comment 22•10 years ago
|
||
Assignee | ||
Comment 23•10 years ago
|
||
Assignee | ||
Comment 24•10 years ago
|
||
Assignee | ||
Comment 25•10 years ago
|
||
Assignee | ||
Comment 26•10 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•10 years ago
|
||
Attachment #8348595 -
Attachment is obsolete: true
Attachment #8348596 -
Attachment is obsolete: true
Attachment #8348595 -
Flags: review?(khuey)
Attachment #8351131 -
Flags: review?(khuey)
Assignee | ||
Updated•10 years ago
|
Attachment #8351131 -
Flags: feedback?(cyu)
Assignee | ||
Comment 28•10 years ago
|
||
Attachment #8351132 -
Flags: review?(khuey)
Attachment #8351132 -
Flags: feedback?(cyu)
Assignee | ||
Comment 29•10 years ago
|
||
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•10 years ago
|
||
Fixed the crash due to a change in nsThread.
Attachment #8355145 -
Flags: review?(khuey)
Comment 31•10 years ago
|
||
(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•10 years ago
|
||
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•10 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•10 years ago
|
||
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•10 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 :).
Comment 38•10 years ago
|
||
(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•10 years ago
|
||
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•10 years ago
|
||
Correct naming.
Attachment #8355459 -
Flags: review?(khuey)
Attachment #8355459 -
Flags: feedback?(cyu)
Assignee | ||
Comment 41•10 years ago
|
||
Assignee | ||
Comment 42•10 years ago
|
||
Assignee | ||
Comment 43•10 years ago
|
||
Assignee | ||
Comment 44•10 years ago
|
||
Assignee | ||
Comment 45•10 years ago
|
||
Assignee | ||
Comment 46•10 years ago
|
||
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•10 years ago
|
||
Attachment #8356051 -
Flags: review?(khuey)
Attachment #8356051 -
Flags: feedback?(cyu)
Assignee | ||
Comment 48•10 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•10 years ago
|
||
Assignee | ||
Comment 50•10 years ago
|
||
Assignee | ||
Comment 51•10 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 52•10 years ago
|
||
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 53•10 years ago
|
||
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+
Comment 54•10 years ago
|
||
(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•10 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•10 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•10 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.
Attachment #819273 -
Attachment is obsolete: true
Assignee | ||
Comment 59•10 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 60•10 years ago
|
||
Attachment #8373931 -
Flags: review?(khuey)
Assignee | ||
Comment 61•10 years ago
|
||
Assignee | ||
Comment 62•10 years ago
|
||
Assignee | ||
Comment 63•10 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•10 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•10 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•10 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•10 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•10 years ago
|
||
Is that because the nsIRunnable* event in |nsThread::DispatchInternal| is not ref counted?
Assignee | ||
Comment 71•10 years ago
|
||
Attachment #8373930 -
Attachment is obsolete: true
Attachment #8382131 -
Flags: review?(khuey)
Assignee | ||
Comment 72•10 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...
Comment 73•10 years ago
|
||
(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•10 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 75•10 years ago
|
||
Assignee | ||
Comment 76•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8384438 -
Flags: review?(khuey)
Assignee | ||
Updated•10 years ago
|
Attachment #8384439 -
Flags: review?(khuey)
Assignee | ||
Comment 77•10 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•10 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•10 years ago
|
||
Remove the wrap in TaskThrottler.
Attachment #8386725 -
Flags: review?(khuey)
Comment 80•10 years ago
|
||
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.
Comment 81•10 years ago
|
||
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•10 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
Comment 84•10 years ago
|
||
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•10 years ago
|
||
This is for the purpose of connecting timer events and their dispatchers.
Attachment #8397743 -
Flags: review?(khuey)
Assignee | ||
Comment 86•10 years ago
|
||
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+
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 91•10 years ago
|
||
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•10 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•10 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•10 years ago
|
Assignee | ||
Comment 94•10 years ago
|
||
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•10 years ago
|
Assignee: cyu → slin
Assignee | ||
Comment 95•10 years ago
|
||
I've merged the part of timer events into this patch.
Attachment #8408905 -
Flags: review?(khuey)
Assignee | ||
Comment 96•10 years ago
|
||
Address mwu's comment.
Attachment #8408907 -
Flags: review?(khuey)
Assignee | ||
Comment 97•10 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•10 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•10 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•10 years ago
|
||
Build flag for turning on TaskTracer. To enable, add "export MOZ_TASK_TRACER=1" in .userconfig
Attachment #8408938 -
Flags: review?(khuey)
Comment 101•10 years ago
|
||
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 102•10 years ago
|
||
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+
Attachment #8408938 -
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•10 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•10 years ago
|
||
Update patch comment, carry r+ from :khuey.
Attachment #8409554 -
Attachment is obsolete: true
Attachment #8409556 -
Flags: review+
Assignee | ||
Comment 108•10 years ago
|
||
Update patch comment, carry r+ from :khuey.
Attachment #8409557 -
Flags: review+
Assignee | ||
Comment 109•10 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•10 years ago
|
||
Update patch comment, carry r+ from :khuey.
Attachment #8409563 -
Flags: review+
Assignee | ||
Comment 111•10 years ago
|
||
Nit fix per try server errors.
Attachment #8409556 -
Attachment is obsolete: true
Attachment #8410018 -
Flags: review+
Assignee | ||
Comment 112•10 years ago
|
||
Nit fix per try server errors.
Attachment #8409563 -
Attachment is obsolete: true
Attachment #8410021 -
Flags: review+
Assignee | ||
Comment 113•10 years ago
|
||
Try server results: https://tbpl.mozilla.org/?tree=Try&rev=328153ca858a https://tbpl.mozilla.org/?tree=Try&rev=e6c44dff012e https://tbpl.mozilla.org/?tree=Try&rev=6a8ff2d773d8
Comment 114•10 years ago
|
||
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 115•10 years ago
|
||
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•10 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•10 years ago
|
||
Comment addressed, nit fixed.
Attachment #8410816 -
Flags: review+
Assignee | ||
Comment 119•10 years ago
|
||
Attachment #8410817 -
Flags: review+
Assignee | ||
Comment 120•10 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 | ||
Comment 121•10 years ago
|
||
Try result: https://tbpl.mozilla.org/?tree=Try&rev=8391d011fd7b
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 122•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/587ca014fdf0 https://hg.mozilla.org/integration/mozilla-inbound/rev/2e0304d75739 https://hg.mozilla.org/integration/mozilla-inbound/rev/3064ab64f06a https://hg.mozilla.org/integration/mozilla-inbound/rev/978c15264432
Keywords: checkin-needed
Comment 123•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/587ca014fdf0 https://hg.mozilla.org/mozilla-central/rev/2e0304d75739 https://hg.mozilla.org/mozilla-central/rev/3064ab64f06a https://hg.mozilla.org/mozilla-central/rev/978c15264432
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
\o/
You need to log in
before you can comment on or make changes to this bug.
Description
•