Closed Bug 1287392 Opened 4 years ago Closed 4 years ago

TaskTracer is broken

Categories

(Core :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: sinker, Assigned: sinker)

References

Details

Attachments

(9 files, 12 obsolete files)

4.64 KB, patch
sinker
: review+
Details | Diff | Splinter Review
1.02 KB, patch
sinker
: review+
Details | Diff | Splinter Review
12.94 KB, patch
sinker
: review+
Details | Diff | Splinter Review
39.96 KB, patch
Details | Diff | Splinter Review
10.64 KB, patch
sinker
: review+
Details | Diff | Splinter Review
5.28 KB, patch
sinker
: review+
Details | Diff | Splinter Review
11.90 KB, patch
sinker
: review+
Details | Diff | Splinter Review
2.13 KB, patch
sinker
: review+
Details | Diff | Splinter Review
10.38 KB, patch
sinker
: review+
Details | Diff | Splinter Review
Adding |ac_add_options --enable-tasktracer| in mozconfig would cause compilation error.  TaskTracer is broken for 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|.
Assignee: nobody → tlee
Attachment #8771922 - Flags: review?(khuey)
Attachment #8771922 - Flags: review?(bgirard)
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.
Attachment #8771922 - Flags: review?(bgirard) → review+
Attachment #8772758 - Flags: review?(cyu)
Attachment #8772758 - Flags: review?(cyu) → review+
Attached patch More changes - wip (obsolete) — Splinter Review
Blocks: 1221846
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.
Flags: needinfo?(tlee)
Ok! I will land it in this week.
Flags: needinfo?(tlee)
This patch fixes the problem of passing trace info to the process in the other end of an IPC channel.
Attachment #8808533 - Flags: review?(cyu)
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?
Flags: needinfo?(tlee)
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 on attachment 8808533 [details] [diff] [review]
Pass info of TaskTracer along with IPC messages

LTGM.
Attachment #8808533 - Flags: review?(cyu) → review+
Flags: needinfo?(tlee)
r=khuey,bgirard
Attachment #8771922 - Attachment is obsolete: true
Attachment #8809428 - Flags: review+
r=cyu
Attachment #8772758 - Attachment is obsolete: true
Attachment #8809430 - Flags: review+
r=cyu
Attachment #8808533 - Attachment is obsolete: true
Attachment #8809431 - Flags: review+
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
Backout by cbook@mozilla.com:
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
Hi Carsten, do you know how do make it happen on try-server?  My try at comment 14 did not find it out.
Flags: needinfo?(tlee) → needinfo?(cbook)
It looks like your trypush in comment 14 did not include any of the patches that you wanted to push.
Flags: needinfo?(cbook)
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).
Fix compile time error.
Attachment #8809428 - Attachment is obsolete: true
Attachment #8810018 - Flags: review+
Changes following part 1~3.
Attachment #8781046 - Attachment is obsolete: true
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
Attached patch Fix bugs of sync for TaskTracer (obsolete) — Splinter Review
Fix some synchronization issues.
Attachment #8811596 - Flags: review?(cyu)
Attached patch Add VirtualTask RAII class (obsolete) — Splinter Review
A helper class for definition of lift-span of a virtual task.
Attachment #8811597 - Flags: review?(cyu)
Attachment #8811598 - Flags: review?(cyu)
Use a linked list in place of nsTArray.
Attachment #8811601 - Flags: review?(cyu)
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.
Attachment #8811606 - Flags: review?(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.
Attachment #8811606 - Flags: review?(mstange) → review+
Change the code according to the suggestion of Markus.
r=mstange
Attachment #8811606 - Attachment is obsolete: true
Attachment #8812076 - Flags: review+
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.
Attachment #8811596 - Flags: review?(cyu) → review+
Fix unify build problems.
Attachment #8812076 - Attachment is obsolete: true
Attachment #8812415 - Flags: review+
(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 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.
Attachment #8811597 - Flags: review?(cyu) → review+
Comment on attachment 8811598 [details] [diff] [review]
Change definition of VPtr of TaskTracer

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

LGTM.
Attachment #8811598 - Flags: review?(cyu) → review+
Comment on attachment 8811601 [details] [diff] [review]
Improve performance of logging of TaskTracer

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

LGTM.
Attachment #8811601 - Flags: review?(cyu) → review+
Attachment #8811596 - Attachment is obsolete: true
Attachment #8813060 - Flags: review+
Attachment #8811597 - Attachment is obsolete: true
Attachment #8813061 - Flags: review+
Attachment #8813061 - Attachment is obsolete: true
Attachment #8813062 - Flags: review+
Attachment #8811598 - Attachment is obsolete: true
Attachment #8811601 - Attachment is obsolete: true
Attachment #8813065 - Flags: review+
Attachment #8813063 - Flags: review+
please checkin part 4 ~ 8.  Thank you!
Blocks: 1319669
Pushed by ihsiao@mozilla.com:
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
Keywords: checkin-needed
Duplicate of this bug: 1307260
Depends on: 1358214
You need to log in before you can comment on or make changes to this bug.