1287392 - TaskTracer is broken

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Thinker Li [:sinker]]

Adding |ac_add_options --enable-tasktracer| in mozconfig would cause compilation error.  TaskTracer is broken for the changes of IPC.

Comment 1

[Thinker Li [:sinker]]

Attachment: Fix TaskTracer bugs caused by the changes of IPC

This patch removes some unused code, and fix code in the IPC module.  And, fix some init & deinit problems of |GeckoSampler|.

Comment 2

[Benoit Girard (:BenWa)]

Comment on attachment 8771922 [details] [diff] [review]
Fix TaskTracer bugs caused by the changes of IPC

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

This looks fine but I'm not that familiar with the code being changed here.

::: tools/profiler/core/GeckoSampler.cpp
@@ +281,5 @@
>    // requests
>    mGatherer->Cancel();
> +
> +#ifdef MOZ_TASK_TRACER
> +  if (mTaskTracer) {

This (StartLogging/StopLogging) would be probably be better as something like a UniquePtr to a task tracer object.

But since this is existing code it doesn't really need to be done here.

Comment 3

[Thinker Li [:sinker]]

Attachment: Ignore obsolete TaskTracer data and fix vptr

Comment 4

[Thinker Li [:sinker]]

Attachment: More changes - wip

Comment 5

[Markus Stange [:mstange]]

Thinker, can we land the first two patches? I spent some time debugging the bugs that you already have patches for, because I didn't know about this bug.

Comment 6

[Thinker Li [:sinker]]

Ok! I will land it in this week.

Comment 7

[Thinker Li [:sinker]]

Attachment: Pass info of TaskTracer along with IPC messages

This patch fixes the problem of passing trace info to the process in the other end of an IPC channel.

Comment 8

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

Comment on attachment 8808533 [details] [diff] [review]
Pass info of TaskTracer along with IPC messages

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

::: ipc/chromium/src/chrome/common/ipc_channel_posix.cc
@@ -805,5 @@
> -  // Save the current TaskTracer info into the message header.
> -  GetCurTraceInfo(&msg->header()->source_event_id,
> -                  &msg->header()->parent_task_id,
> -                  &msg->header()->source_event_type);
> -#endif

This is removed because it's already done in Message ctor in your part 1 patch?

Comment 9

[Thinker Li [:sinker]]

Comment on attachment 8808533 [details] [diff] [review]
Pass info of TaskTracer along with IPC messages

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

::: ipc/chromium/src/chrome/common/ipc_channel_posix.cc
@@ -805,5 @@
> -  // Save the current TaskTracer info into the message header.
> -  GetCurTraceInfo(&msg->header()->source_event_id,
> -                  &msg->header()->parent_task_id,
> -                  &msg->header()->source_event_type);
> -#endif

Here is the wrong place to do it since it is running on IOThread while these information are available at the main thread or the thread occupied by the sender.

Comment 10

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

Comment on attachment 8808533 [details] [diff] [review]
Pass info of TaskTracer along with IPC messages

LTGM.

Comment 11

[Thinker Li [:sinker]]

Attachment: Part 1: Fix TaskTracer bugs caused by the changes of IPC

r=khuey,bgirard

Comment 12

[Thinker Li [:sinker]]

Attachment: Part 2: Ignore obsolete TaskTracer data and fix vptr

r=cyu

Comment 13

[Thinker Li [:sinker]]

Attachment: Part 3: Pass info of TaskTracer along with IPC messages

r=cyu

Comment 14

[Thinker Li [:sinker]]

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/606c7cb149c9
Part 1: Fix TaskTracer bugs caused by the changes of IPC. r=khuey, r=bgirard
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac7da68e00f5
Part 2: Ignore obsolete TaskTracer data and fix vptr. r=cyu
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d8ed1ceafd7
Part 3: Pass info of TaskTracer along with IPC messages. r=cyu

Comment 16

[Carsten Book [:Tomcat]]

backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=39052798&repo=mozilla-inbound

Comment 17

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d243b2d5c128
Backed out changeset 7d8ed1ceafd7 
https://hg.mozilla.org/integration/mozilla-inbound/rev/915541925ea9
Backed out changeset ac7da68e00f5 
https://hg.mozilla.org/integration/mozilla-inbound/rev/db56c201e562
Backed out changeset 606c7cb149c9 for bustage on a CLOSED TREE

Comment 18

[Thinker Li [:sinker]]

Hi Carsten, do you know how do make it happen on try-server?  My try at comment 14 did not find it out.

Comment 19

[Markus Stange [:mstange]]

It looks like your trypush in comment 14 did not include any of the patches that you wanted to push.

Comment 20

[Markus Stange [:mstange]]

But since your patch is adding gettid to platform.h, you can probably just remove the gettid function from platform-macos.cc (and the #include above it).

Comment 21

[Thinker Li [:sinker]]

Attachment: Part 1: Fix TaskTracer bugs caused by the changes of IPC.

Fix compile time error.

Comment 22

[Thinker Li [:sinker]]

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

Comment 23

[Thinker Li [:sinker]]

Attachment: More changes - wip, v2

Changes following part 1~3.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8e5421730e3
Part 1: Fix TaskTracer bugs caused by the changes of IPC. r=khuey, r=bgirard
https://hg.mozilla.org/integration/mozilla-inbound/rev/18bf88f09ab5
Part 2: Ignore obsolete TaskTracer data and fix vptr. r=cyu
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b4df02e2393
Part 3: Pass info of TaskTracer along with IPC messages. r=cyu

Comment 25

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

https://hg.mozilla.org/mozilla-central/rev/c8e5421730e3
https://hg.mozilla.org/mozilla-central/rev/18bf88f09ab5
https://hg.mozilla.org/mozilla-central/rev/6b4df02e2393

Comment 26

[Thinker Li [:sinker]]

Attachment: Fix bugs of sync for TaskTracer

Fix some synchronization issues.

Comment 27

[Thinker Li [:sinker]]

Attachment: Add VirtualTask RAII class

A helper class for definition of lift-span of a virtual task.

Comment 28

[Thinker Li [:sinker]]

Attachment: Change definition of VPtr of TaskTracer

Comment 29

[Thinker Li [:sinker]]

Attachment: Improve performance of logging of TaskTracer

Use a linked list in place of nsTArray.

Comment 30

[Thinker Li [:sinker]]

Attachment: Expose a file writer API for GeckoProfiler

Hi Markus,
This patch add a helper function to dump logs of GeckoProfiler and TaskTracer into a file.  I usually call this function |mozilla_sampler_get_profile_data_async_file()| in gdb with shell script.

I know the log of GeckoProfiler is different to the one saved by Cleopatra, but it is useful for people who used to use shell script.

Comment 31

[Markus Stange [:mstange]]

Comment on attachment 8811606 [details] [diff] [review]
Expose a file writer API for GeckoProfiler

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

r+ with comments addressed

::: tools/profiler/core/GeckoSampler.h
@@ +111,5 @@
>    void GetGatherer(nsISupports** aRetVal);
>  #endif
>    mozilla::UniquePtr<char[]> ToJSON(double aSinceTime = 0);
>    virtual void ToJSObjectAsync(double aSinceTime = 0, mozilla::dom::Promise* aPromise = 0);
> +  void ToFilePath(const char* aFileName, double aSinceTime = 0);

Let's call this ToFileAsync, or DumpToFileAsync.

::: tools/profiler/core/platform.cpp
@@ +623,5 @@
>  
>    t->ToJSObjectAsync(aSinceTime, aPromise);
>  }
>  
> +void mozilla_sampler_get_profile_data_async_file(double aSinceTime,

we already have mozilla_sampler_save_profile_to_file, so I'd call this mozilla_sampler_save_profile_to_file_async

@@ +626,5 @@
>  
> +void mozilla_sampler_get_profile_data_async_file(double aSinceTime,
> +						 const char* aFileName)
> +{
> +  char *filename = strdup(aFileName);

I don't like using strdup + free. Please store this in an nsCString. It'll mean an extra copy but I think that's ok.

::: tools/profiler/gecko/ProfileGatherer.cpp
@@ +138,5 @@
> +}
> +
> +void
> +ProfileGatherer::Start(double aSinceTime,
> +                       const char* aFileName)

You can remove this overload then.

Comment 32

[Thinker Li [:sinker]]

Attachment: Part 4: Expose a file writer API for GeckoProfiler, v2

Change the code according to the suggestion of Markus.
r=mstange

Comment 33

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

Comment on attachment 8811596 [details] [diff] [review]
Fix bugs of sync for TaskTracer

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

r+ with the following addressed:

::: tools/profiler/tasktracer/GeckoTaskTracer.cpp
@@ +146,5 @@
>  
>  inline static bool
>  IsStartLogging()
>  {
> +  return sStarted && sTraceInfos != nullptr;

I think we need to lock sMutex for checking whether sTraceInfos is nullptr here.

@@ +189,5 @@
>  } // namespace anonymous
>  
>  nsCString*
>  TraceInfo::AppendLog()
>  {

Since we lock the mutex outside this method to protect the operation of AppendPrintf(), please assert that the mutex is locked in this method.

Comment 34

[Thinker Li [:sinker]]

Attachment: Part 4: Expose a file writer API for GeckoProfiler, v3

Fix unify build problems.

Comment 35

[Thinker Li [:sinker]]

(In reply to Thinker Li [:sinker] from comment #34)
> Created attachment 8812415 [details] [diff] [review]
> Part 4: Expose a file writer API for GeckoProfiler, v3
> 
> Fix unify build problems.

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

Comment 36

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

Comment on attachment 8811597 [details] [diff] [review]
Add VirtualTask RAII class

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

r+ with the following addressed.

::: tools/profiler/moz.build
@@ +127,5 @@
>  if CONFIG['MOZ_TASK_TRACER']:
>      EXPORTS += [
>          'tasktracer/GeckoTaskTracer.h',
>          'tasktracer/GeckoTaskTracerImpl.h',
> +        'tasktracer/SourceEventTypeMap.h',

nit: This header file is not one that can be included independently. It's exported just because it's #include'd in GeckoTaskTracer.h. I think we need to have something like
https://dxr.mozilla.org/mozilla-central/rev/b7f895c1dc2e91530240efbf50ac063a0f8a9cb5/xpcom/glue/nsTArray-inl.h#7
in it.

Comment 37

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

Comment on attachment 8811598 [details] [diff] [review]
Change definition of VPtr of TaskTracer

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

LGTM.

Comment 38

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

Comment on attachment 8811601 [details] [diff] [review]
Improve performance of logging of TaskTracer

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

LGTM.

Comment 39

[Thinker Li [:sinker]]

Attachment: Part 5: Fix bugs of sync for TaskTracer

Comment 40

[Thinker Li [:sinker]]

Attachment: Part 6: Add VirtualTask RAII class

Comment 41

[Thinker Li [:sinker]]

Attachment: Part 6: Add VirtualTask RAII class. r=cyu

Comment 42

[Thinker Li [:sinker]]

Attachment: Part 7: Change definition of VPtr of TaskTracer

Comment 43

[Thinker Li [:sinker]]

Attachment: Part 8: Improve performance of logging of TaskTracer

Comment 44

[Thinker Li [:sinker]]

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

Comment 45

[Thinker Li [:sinker]]

please checkin part 4 ~ 8.  Thank you!

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/09620b9e6d06
Part 4: Expose a file writer API for GeckoProfiler. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c3e92ce1990
Part 5: Fix bugs of sync for TaskTracer. r=cyu
https://hg.mozilla.org/integration/mozilla-inbound/rev/01a2259e75cc
Part 6: Add VirtualTask RAII class. r=cyu
https://hg.mozilla.org/integration/mozilla-inbound/rev/48d6e51b95ea
Part 7: Change definition of VPtr of TaskTracer. r=cyu
https://hg.mozilla.org/integration/mozilla-inbound/rev/df33b1b46ef3
Part 8: Improve performance of logging of TaskTracer. r=cyu

Comment 47

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/09620b9e6d06
https://hg.mozilla.org/mozilla-central/rev/4c3e92ce1990
https://hg.mozilla.org/mozilla-central/rev/01a2259e75cc
https://hg.mozilla.org/mozilla-central/rev/48d6e51b95ea
https://hg.mozilla.org/mozilla-central/rev/df33b1b46ef3