Closed
Bug 1100259
Opened 10 years ago
Closed 10 years ago
TaskTracer: Add labels in EventDispatcher and console.log
Categories
(Core :: Gecko Profiler, defect)
Core
Gecko Profiler
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: shelly, Assigned: shelly)
References
()
Details
Attachments
(1 file, 3 obsolete files)
5.34 KB,
patch
|
shelly
:
review+
|
Details | Diff | Splinter Review |
We tried not to land unnecessary TaskTracer labels into mozilla-central, but we found out labelling the event names is somehow useful in general. Second, also a useful hack in console.log, which might be worth checked-in: Adding prefix "tt:" in the message of console.log will be additionally treated as a TaskTracer label. For example, writing |console.log("tt:Your message here.")| in javascript, will see not only the above message in console, but also a label with message "Your message here." on Nephthys. http://dxr.mozilla.org/mozilla-central/source/xpcom/base/nsConsoleService.cpp#170
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8525216 -
Flags: feedback?(tlee)
Assignee | ||
Comment 2•10 years ago
|
||
I think the patch is ready for review, and I'll post a heads-up about the hack in console.log to dev-b2g before it lands. Thanks!
Attachment #8525216 -
Attachment is obsolete: true
Attachment #8525216 -
Flags: feedback?(tlee)
Attachment #8525260 -
Flags: review?(tlee)
Comment 3•10 years ago
|
||
Comment on attachment 8525260 [details] [diff] [review] Add TaskTracer labels in EventDispatcher and console.log Review of attachment 8525260 [details] [diff] [review]: ----------------------------------------------------------------- ::: tools/profiler/GeckoTaskTracer.cpp @@ +414,5 @@ > +GetJSLabelPrefix() > +{ > + StaticMutexAutoLock lock(sMutex); > + return sJSLabelPrefix->get(); > +} Since this function returns a C string, I had better to keep it simple, just define |sJSLabelPrefix| as a C string. Then, we don't need initialize/clean and lock here.
Attachment #8525260 -
Flags: review?(tlee) → review-
Assignee | ||
Comment 4•10 years ago
|
||
Fixed.
Attachment #8525260 -
Attachment is obsolete: true
Attachment #8525296 -
Flags: review?(tlee)
Comment 5•10 years ago
|
||
Comment on attachment 8525296 [details] [diff] [review] Add TaskTracer labels in EventDispatcher and console.log Review of attachment 8525296 [details] [diff] [review]: ----------------------------------------------------------------- ::: tools/profiler/GeckoTaskTracer.cpp @@ +404,5 @@ > > +const char* > +GetJSLabelPrefix() > +{ > + StaticMutexAutoLock lock(sMutex); I think we could remove this line too since there is no sync risk.
Attachment #8525296 -
Flags: review?(tlee) → review+
Assignee | ||
Comment 6•10 years ago
|
||
Thanks! Update with nit fixed.
Attachment #8525296 -
Attachment is obsolete: true
Attachment #8525763 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 7•10 years ago
|
||
Try result: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0e5dd2cd793d
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → slin
Keywords: checkin-needed
Comment 8•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/794b5fca57b0
Keywords: checkin-needed
Comment 9•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/794b5fca57b0
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment 10•9 years ago
|
||
Comment on attachment 8525763 [details] [diff] [review] Add TaskTracer labels in EventDispatcher and console.log (revised) Don't know why this didn't get a review from a DOM peer. I hope MOZ_TASK_TRACER isn't set by default, since otherwise the change would have observable affect to the performance. Another thing, at this point of DOM event dispatch, we may not yet have a DOM Event, only WidgetEvent, so the change misses probably most of the events.
Assignee | ||
Comment 11•9 years ago
|
||
Hi smaug, thanks for the attention. MOZ_TASK_TRACER isn't set by default, and I've created another bug to track this issue (Bug 1124972 - TaskTracer: (Re-review bug 1100259) Add labels in EventDispatcher), we'll make sure to have it looked by a DOM peer this time.
You need to log in
before you can comment on or make changes to this bug.
Description
•