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|.

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.

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.

Ok! I will land it in this week.

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

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 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.

r=khuey,bgirard

r=cyu

r=cyu

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

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

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.

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

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.

Changes following part 1~3.

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

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

Fix some synchronization issues.

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

Use a linked list in place of nsTArray.

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 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.

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

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.

Fix unify build problems.

(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.

Comment on attachment 8811598 [details] [diff] [review] Change definition of VPtr of TaskTracer Review of attachment 8811598 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.

Comment on attachment 8811601 [details] [diff] [review] Improve performance of logging of TaskTracer Review of attachment 8811601 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.

please checkin part 4 ~ 8. Thank you!

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

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