Closed Bug 1323076 Opened 3 years ago Closed Last year

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

Categories

(Core :: Gecko Profiler, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
firefox53 --- affected

People

(Reporter: mstange, Assigned: sinker)

References

(Depends on 1 open bug)

Details

(Whiteboard: [ps-radar])

Attachments

(6 files, 10 obsolete files)

1.95 KB, patch
sinker
: review+
Details | Diff | Splinter Review
12.57 KB, patch
sinker
: review+
Details | Diff | Splinter Review
5.25 KB, patch
sinker
: review+
Details | Diff | Splinter Review
1.95 KB, patch
sinker
: review+
Details | Diff | Splinter Review
914 bytes, patch
sinker
: review+
Details | Diff | Splinter Review
4.04 KB, patch
sinker
: review+
Details | Diff | Splinter Review
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.
Duplicate of this bug: 1076772
Depends on: 1325526
Depends on: 1342839
Depends on: 1331173
Assignee: nobody → tlee
Depends on: 1167634
Depends on: 1342774
Depends on: 1329929
Whiteboard: [ps-radar]
Attachment #8846270 - Attachment is obsolete: true
Attachment #8846555 - Flags: review?(cyu)
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.
Attachment #8846556 - Flags: review?(bugs)
Attachment #8846557 - Flags: review?(mstange)
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.
Attachment #8846558 - Flags: review?(bobbyholley)
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.
Attachment #8846560 - Flags: review?(honzab.moz)
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 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.
Attachment #8846556 - Flags: review?(bugs) → review-
Attachment #8846558 - Flags: review?(bobbyholley) → review?(erahm)
Attachment #8846558 - Flags: review?(erahm) → review+
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?
Attachment #8846557 - Flags: review?(mstange) → review+
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.
Attachment #8846555 - Attachment is obsolete: true
Attachment #8846555 - Flags: review?(cyu)
Attachment #8847515 - Flags: review?(cyu)
smaug, This patch gets a name from |WidgetEvent::mClass| if there is no given DOM event.
Attachment #8846556 - Attachment is obsolete: true
Attachment #8847517 - Flags: review?(bugs)
Attachment #8847515 - Attachment description: TaskTracer with only flag checking overhead, v2 → Part 1: TaskTracer with only flag checking overhead, v2
(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 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.
Attachment #8847517 - Flags: review?(bugs) → review-
Attachment #8846560 - Flags: review?(honzab.moz) → review+
Smaug, Just like what you have mentioned, I refactored |Event::GetType()| to get event type from |WidgetEvent::mMessage|.
Attachment #8847517 - Attachment is obsolete: true
Attachment #8849023 - Flags: review?(bugs)
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
Attachment #8849023 - Flags: review?(bugs) → review+
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?
Flags: needinfo?(tlee)
(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.
Flags: needinfo?(tlee)
(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 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?
Attachment #8847515 - Flags: review?(cyu) → review+
Handle source events from PresShell for TaskTracer.
Attachment #8849878 - Flags: review?(bugs)
Attachment #8849878 - Flags: review?(bugs) → review+
r=smaug
Attachment #8849878 - Attachment is obsolete: true
Attachment #8850301 - Flags: review+
r=cervantes
Attachment #8847515 - Attachment is obsolete: true
Attachment #8850302 - Flags: review+
r=mstange
Attachment #8846557 - Attachment is obsolete: true
Attachment #8850305 - Flags: review+
r=erahm
Attachment #8846558 - Attachment is obsolete: true
Attachment #8850306 - Flags: review+
r=mayhemer
Attachment #8846560 - Attachment is obsolete: true
Attachment #8850308 - Flags: review+
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
The leave-open keyword is there and there is no activity for 6 months.
:gregtatum, maybe it's time to close this bug?
Flags: needinfo?(gtatum)
Status: NEW → RESOLVED
Closed: Last year
Flags: needinfo?(gtatum)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.