Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: sinker, Assigned: sinker)

Tracking

(Depends on 1 bug)

Trunk
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(9 attachments, 12 obsolete attachments)

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
Assignee

Description

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

Comment 1

3 years ago
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+
Assignee

Comment 3

3 years ago
Attachment #8772758 - Flags: review?(cyu)
Attachment #8772758 - Flags: review?(cyu) → review+
Assignee

Comment 4

3 years ago
Posted 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)
Assignee

Comment 6

3 years ago
Ok! I will land it in this week.
Flags: needinfo?(tlee)
Assignee

Comment 7

3 years ago
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)
Assignee

Comment 9

3 years ago
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+
Assignee

Updated

3 years ago
Flags: needinfo?(tlee)
Assignee

Comment 11

3 years ago
r=khuey,bgirard
Attachment #8771922 - Attachment is obsolete: true
Attachment #8809428 - Flags: review+
Assignee

Comment 12

3 years ago
r=cyu
Attachment #8772758 - Attachment is obsolete: true
Attachment #8809430 - Flags: review+
Assignee

Comment 13

3 years ago
r=cyu
Attachment #8808533 - Attachment is obsolete: true
Attachment #8809431 - Flags: review+

Comment 15

3 years ago
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

Comment 17

3 years ago
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
Assignee

Comment 18

3 years ago
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).
Assignee

Comment 21

3 years ago
Fix compile time error.
Attachment #8809428 - Attachment is obsolete: true
Attachment #8810018 - Flags: review+
Assignee

Comment 23

3 years ago
Changes following part 1~3.
Attachment #8781046 - Attachment is obsolete: true

Comment 24

3 years ago
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
Assignee

Comment 26

3 years ago
Fix some synchronization issues.
Attachment #8811596 - Flags: review?(cyu)
Assignee

Comment 27

3 years ago
Posted 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)
Assignee

Comment 28

3 years ago
Attachment #8811598 - Flags: review?(cyu)
Assignee

Comment 29

3 years ago
Use a linked list in place of nsTArray.
Attachment #8811601 - Flags: review?(cyu)
Assignee

Comment 30

3 years ago
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+
Assignee

Comment 32

3 years ago
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+
Assignee

Comment 34

3 years ago
Fix unify build problems.
Attachment #8812076 - Attachment is obsolete: true
Attachment #8812415 - Flags: review+
Assignee

Comment 35

3 years ago
(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+
Assignee

Comment 39

3 years ago
Attachment #8811596 - Attachment is obsolete: true
Attachment #8813060 - Flags: review+
Assignee

Comment 40

3 years ago
Attachment #8811597 - Attachment is obsolete: true
Attachment #8813061 - Flags: review+
Assignee

Comment 41

3 years ago
Attachment #8813061 - Attachment is obsolete: true
Attachment #8813062 - Flags: review+
Assignee

Comment 42

3 years ago
Attachment #8811598 - Attachment is obsolete: true
Assignee

Comment 43

3 years ago
Attachment #8811601 - Attachment is obsolete: true
Attachment #8813065 - Flags: review+
Assignee

Updated

3 years ago
Attachment #8813063 - Flags: review+
Assignee

Comment 45

3 years ago
please checkin part 4 ~ 8.  Thank you!
Assignee

Updated

3 years ago
Blocks: 1319669

Comment 46

3 years ago
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

Updated

2 years ago
Depends on: 1358214
You need to log in before you can comment on or make changes to this bug.