1100259 - TaskTracer: Add labels in EventDispatcher and console.log

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Shelly Lin [:shelly]]

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

Comment 1

[Shelly Lin [:shelly]]

Attachment: Add TaskTracer labels in EventDispatcher and console.log

Comment 2

[Shelly Lin [:shelly]]

Attachment: Add TaskTracer labels in EventDispatcher and console.log

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!

Comment 3

[Thinker Li [:sinker]]

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.

Comment 4

[Shelly Lin [:shelly]]

Attachment: Add TaskTracer labels in EventDispatcher and console.log

Fixed.

Comment 5

[Thinker Li [:sinker]]

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.

Comment 6

[Shelly Lin [:shelly]]

Attachment: Add TaskTracer labels in EventDispatcher and console.log (revised)

Thanks!
Update with nit fixed.

Comment 7

[Shelly Lin [:shelly]]

Try result:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0e5dd2cd793d

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/794b5fca57b0

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/794b5fca57b0

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 11

[Shelly Lin [:shelly]]

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.