1323076 - Build TaskTracer code by default and put all overhead behind a runtime check

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Markus Stange [:mstange]]

It would be great to have task tracer instrumentation enabled by default, so that one does not need a custom build.

We should find out which parts can be put behind a runtime check, and for the parts that cannot, what their overhead is.

Comment 2

[Thinker Li [:sinker]]

Attachment: TaskTracer with only flag checking overhead

Comment 3

[Thinker Li [:sinker]]

Attachment: Part 1: TaskTracer with only flag checking overhead

Comment 4

[Thinker Li [:sinker]]

Attachment: Part 2: EventDispatcher with flag checking for TaskTr acer

Comment 5

[Thinker Li [:sinker]]

Attachment: Part 3: IPC with flag checking for TaskTracer

Comment 6

[Thinker Li [:sinker]]

Attachment: Part 4: ConsoleService with flag checking for TaskTracer

Comment 7

[Thinker Li [:sinker]]

Attachment: Part 5: Necko with flag checking for TaskTracer

Comment 8

[Thinker Li [:sinker]]

Comment on attachment 8846556 [details] [diff] [review]
Part 2: EventDispatcher with flag checking for TaskTr acer

Add a line to check if TaskTracer has been enabled.  It reduces overhead if TaskTracer is built but disabled.

Comment 9

[Thinker Li [:sinker]]

Comment on attachment 8846558 [details] [diff] [review]
Part 4: ConsoleService with flag checking for TaskTracer

Hi Bobby, add a line to check if TaskTracer is enabled before doing any real matter.

Comment 10

[Thinker Li [:sinker]]

Comment on attachment 8846560 [details] [diff] [review]
Part 5: Necko with flag checking for TaskTracer

Hi Honza, this patch add lines to check if TaskTracer has been enabled before doing real matters.

Comment 11

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

Does this affect to microbenchmarks like http://mozilla.pettay.fi/moztests/events/event_speed_2.1_nolistener.html ? I assume no, but please verify.

Comment 12

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

Comment on attachment 8846556 [details] [diff] [review]
Part 2: EventDispatcher with flag checking for TaskTr acer

The TaskTracer related code in EventDispatcher is wrong enough, that I don't think we should enable this before fixing it.

It is totally fine to not have DOMEvent when dispatching an event - that happens with most of the user initiated events which are WidgetEvents from widget/ or dom/ipc code. DOMEvent is created then on demand if there are listeners for the event.
So, if there isn't aDOMEvent, you want to get the type from aEvent.
You may want to refactor Event::GetType to a static helper method and use it here too.

Comment 13

[Markus Stange [:mstange]]

Comment on attachment 8846557 [details] [diff] [review]
Part 3: IPC with flag checking for TaskTracer

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

At some point this code needs to be reviewed by an IPC peer (and I'm not one). What's your plan for that? Are you going to create patches that remove the #ifdefs and have those be reviewed by the relevant peers?

Comment 14

[Thinker Li [:sinker]]

Attachment: Part 1: TaskTracer with only flag checking overhead, v2

Make |sStarted| non-atomic, and rename it to |gStarted|.  Since there is only one place would change it with the protection of a mutex, and it is a |bool|, a two states variable, it is safe to read it elsewhere without atomic.

Comment 15

[Thinker Li [:sinker]]

Attachment: Part 2: EventDispatcher with flag checking for TaskTracer, v2

smaug, This patch gets a name from |WidgetEvent::mClass| if there is no given DOM event.

Comment 16

[Thinker Li [:sinker]]

(In reply to Olli Pettay [:smaug] from comment #11)
> Does this affect to microbenchmarks like
> http://mozilla.pettay.fi/moztests/events/event_speed_2.1_nolistener.html ? I
> assume no, but please verify.

I have tried it.  It has minor degrade with the patch.  I tried several ways to optimize it.  In one of them, I refactor the code to a function and using |MOZ_UNLIKELY| for the branch calling the function, it is success to optimize the code and get almost the same number as without the patch.  It make me believe that it is somewhat compiler can do for us with PGO.  I will try PGO build later to confirm it.

Comment 17

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

Comment on attachment 8847517 [details] [diff] [review]
Part 2: EventDispatcher with flag checking for TaskTracer, v2

No, you don't want mClass. You end up reporting either class names (mClass) or event types (GetType), so totally different things

aEvent->mMessage is the interesting part. Please just try to do what Event::GetType does.

Comment 18

[Thinker Li [:sinker]]

Attachment: Part 2: EventDispatcher with flag checking for TaskTracer, v3

Smaug, Just like what you have mentioned, I refactored |Event::GetType()| to get event type from |WidgetEvent::mMessage|.

Comment 19

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

Comment on attachment 8849023 [details] [diff] [review]
Part 2: EventDispatcher with flag checking for TaskTracer, v3


>+void
>+Event::GetWidgetEventType(WidgetEvent* aEvent, nsAString& aType) {
Nit, { goes to its own line with method definitions.


> #ifdef MOZ_TASK_TRACER
>-  {
>+  if (MOZ_UNLIKELY(mozilla::tasktracer::IsStartLogging())) {
So tasktracer is multithreaded, right?

>+    nsAutoCString eventType;
>+    nsAutoString eventTypeU16;
>     if (aDOMEvent) {
>-      nsAutoString eventType;
>-      aDOMEvent->GetType(eventType);
>+      aDOMEvent->GetType(eventTypeU16);
>+    } else {
>+      Event::GetWidgetEventType(aEvent, eventTypeU16);
>+    }
Technically you could just use GetWidgetEventType and Event::GetType wouldn't even need to do thread check, but we can keep it this way for now.

>+++ b/widget/BasicEvents.h
>@@ -390,16 +390,18 @@ public:
>     MOZ_ASSERT(mClass == eBasicEventClass,
>                "Duplicate() must be overridden by sub class");
>     WidgetEvent* result = new WidgetEvent(false, mMessage);
>     result->AssignEventData(*this, true);
>     result->mFlags = mFlags;
>     return result;
>   }
> 
>+  static void GetType(nsAString& aType);
>+
What is this? Leftover? Please remove

Comment 20

[Cervantes Yu [:cyu] [:cervantes]]

Comment on attachment 8847515 [details] [diff] [review]
Part 1: TaskTracer with only flag checking overhead, v2

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

If you make gStarted non-atomic, we must be pretty sure this is safe for readers of it.
Generally it's OK if it's changed from false to true and then some other thread haven't seen the change because the lack of a memory barrier. But the case where we change its value from true to false is far more tricky: how are you sure it's OK when we change its value from true to false when its readers doesn't use a memory barrier? Are you sure that we won't free some memory and then some other thread thinks that gStarted is true and then accesses the freed memory?

Comment 21

[Thinker Li [:sinker]]

(In reply to Olli Pettay [:smaug] from comment #19)
> Comment on attachment 8849023 [details] [diff] [review]
> Part 2: EventDispatcher with flag checking for TaskTracer, v3
> 
> 
> >+void
> >+Event::GetWidgetEventType(WidgetEvent* aEvent, nsAString& aType) {
> Nit, { goes to its own line with method definitions.
> 
> 
> > #ifdef MOZ_TASK_TRACER
> >-  {
> >+  if (MOZ_UNLIKELY(mozilla::tasktracer::IsStartLogging())) {
> So tasktracer is multithreaded, right?
Yes, it is multi-threaded.  But, it is a boolean value (gStarted), and
only one function protected by a mutex would change it, so it is in
theory changed in atomic.  There is a latency of propagation of the value
among threads, but it is fined for the case here even threads do
not start logging at exact the same moment.

Comment 22

[Thinker Li [:sinker]]

(In reply to Cervantes Yu [:cyu] [:cervantes] from comment #20)
> Comment on attachment 8847515 [details] [diff] [review]
> Part 1: TaskTracer with only flag checking overhead, v2
> 
> Review of attachment 8847515 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If you make gStarted non-atomic, we must be pretty sure this is safe for
> readers of it.
> Generally it's OK if it's changed from false to true and then some other
> thread haven't seen the change because the lack of a memory barrier. But the
> case where we change its value from true to false is far more tricky: how
> are you sure it's OK when we change its value from true to false when its
> readers doesn't use a memory barrier? Are you sure that we won't free some
> memory and then some other thread thinks that gStarted is true and then
> accesses the freed memory?

|IsStartLogging()| will be checked again in logging functions (|AddLabel()| for example)
with the protection of |mLogsMutex|.

Comment 23

[Cervantes Yu [:cyu] [:cervantes]]

Comment on attachment 8847515 [details] [diff] [review]
Part 1: TaskTracer with only flag checking overhead, v2

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

::: tools/profiler/tasktracer/TracedTaskCommon.h
@@ +103,5 @@
>     * instances are never dispatched and ran by event loops.  This
>     * class used to define running time as the life-span of it's
>     * instance.
>     */
>    class AutoRunTask : public AutoSaveCurTraceInfo {

This class is only defined but not used anywhere. Is it planned to be used or just dead code?

Comment 24

[Thinker Li [:sinker]]

Attachment: Part 6: HandleEvent with flag checking for TaskTracer

Handle source events from PresShell for TaskTracer.

Comment 25

[Thinker Li [:sinker]]

Attachment: Part 6: HandleEvent with flag checking for TaskTracer, v2

r=smaug

Comment 26

[Thinker Li [:sinker]]

Attachment: Part 1: TaskTracer with only flag checking overhead, v3

r=cervantes

Comment 27

[Thinker Li [:sinker]]

Attachment: Part 2: EventDispatcher with flag checking for TaskTr acer, v4

Comment 28

[Thinker Li [:sinker]]

Attachment: Part 3: IPC with flag checking for TaskTracer, v2

r=mstange

Comment 29

[Thinker Li [:sinker]]

Attachment: Part 4: ConsoleService with flag checking for TaskTracer, v2

r=erahm

Comment 30

[Thinker Li [:sinker]]

Attachment: Part 5: Necko with flag checking for TaskTracer, v2

r=mayhemer

Comment 31

[Thinker Li [:sinker]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2f3b5604d2c3eb61557753652a4c424b21bbaa1

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f1a84907446
Part 1: TaskTracer with only flag checking overhead. r=cervantes
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fcd4afbd7f7
Part 2: EventDispatcher with flag checking for TaskTracer. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c1332ca8031
Part 3: IPC with flag checking for TaskTracer. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/425695abf308
Part 4: ConsoleService with flag checking for TaskTracer. r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/26ba1913fa35
Part 5: Necko with flag checking for TaskTracer. r=mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/75e1cfcb595b
Part 6: HandleEvent with flag checking for TaskTracer. r=smaug

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7f1a84907446
https://hg.mozilla.org/mozilla-central/rev/9fcd4afbd7f7
https://hg.mozilla.org/mozilla-central/rev/0c1332ca8031
https://hg.mozilla.org/mozilla-central/rev/425695abf308
https://hg.mozilla.org/mozilla-central/rev/26ba1913fa35
https://hg.mozilla.org/mozilla-central/rev/75e1cfcb595b

Comment 34

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:gregtatum, maybe it's time to close this bug?