1221846 - Get Task Tracer working on desktop

Status: RESOLVED FIXED

(Toolkit :: Performance Monitoring, defect, P2)

Description

[Brad Lassey [:blassey] (use needinfo?)]

B2G has an interesting took called Task Tracer that I think could be useful for tracking down performance issues in e10s, we should look into getting it working on desktop.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

According to cervantes, the current front-end for Task Tracer isn't too helpful and is not being maintained. During a meeting in Mozlando with BenWa, cervantes, and mayhemer, we decided that mayhemer's Backtrack tool[1] would use the same back-end that Task Tracer provides, but that mayhemer would write a front-end for Cleopatra to display that information. I believe that work is targeted for Q1 of 2016.

I currently have some small patches that allow Task Tracer to build for desktop, which I'll post here this afternoon once I've cleaned them up. There are a few smaller problems with them still that I think I need to resolve with cervantes, but I think getting Task Tracer support for desktop is pretty straight forward. But we'll still be blocked on a way of visualizing the information until mayhemer's work has landed in Cleopatra.

So what I propose is that I post the patches for getting Task Tracer to work on desktop, and get them reviewed and landed. Another bug (if one doesn't already exist) should be filed for the Cleopatra work, and then we should probably get that tracked by the e10s team.

[1]: http://www.janbambas.cz/new-gecko-performance-tool-backtrack/

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1221846 - Get Task Tracer building on desktop r=cyu.

Review commit: https://reviewboard.mozilla.org/r/28073/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28073/

Comment 3

[Mike Conley (:mconley) (:⚙️)]

So unfortunately, I can't ask for feedback on this patch using MozReview - only review. But assume this is still kinda work-in-progress.

This patch gets Task Tracer to build for me on OS X. I have not tried building it on Linux, and I somehow doubt it will work properly on Windows.

Note that in a debug build, this will cause an assertion failure and crash, since profiler_init is entered into twice (and InitTaskTracer MOZ_ASSERTS that it is only entered once).

We enter twice with these stacks:

First stack:
  * frame #0: 0x0000000107070ace XUL`profiler_init(stackTop=0x00007fff5fbfeea7) + 14 at GeckoProfilerImpl.h:66
    frame #1: 0x0000000107072409 XUL`GeckoProfilerInitRAII::GeckoProfilerInitRAII(this=0x00007fff5fbfeea0, stackTop=0x00007fff5fbfeea7) + 25 at GeckoProfiler.h:257
    frame #2: 0x000000010707147d XUL`GeckoProfilerInitRAII::GeckoProfilerInitRAII(this=0x00007fff5fbfeea0, stackTop=0x00007fff5fbfeea7) + 29 at GeckoProfiler.h:256
    frame #3: 0x000000010706e1be XUL`XREMain::XRE_main(this=0x00007fff5fbfef18, argc=5, argv=0x00007fff5fbff818, aAppData=0x00007fff5fbff198) + 62 at nsAppRunner.cpp:4334
    frame #4: 0x000000010706e9d7 XUL`::XRE_main(argc=5, argv=0x00007fff5fbff818, aAppData=0x00007fff5fbff198, aFlags=0) + 103 at nsAppRunner.cpp:4485
    frame #5: 0x00000001000028cf firefox`do_main(argc=5, argv=0x00007fff5fbff818, xreDirectory=0x000000010040e400) + 1791 at nsBrowserApp.cpp:212
    frame #6: 0x0000000100001cf6 firefox`main(argc=5, argv=0x00007fff5fbff818) + 294 at nsBrowserApp.cpp:352
    frame #7: 0x0000000100001774 firefox`start + 52

Second stack:
  * frame #0: 0x0000000101f878be XUL`profiler_init(stackTop=0x00007fff5fbfed5f) + 14 at GeckoProfilerImpl.h:66
    frame #1: 0x0000000101f85abc XUL`::NS_InitXPCOM2(aResult=0x00000001004044c0, aBinDirectory=0x000000010040e880, aAppFileLocationProvider=0x00007fff5fbfef50) + 92 at XPCOMInit.cpp:507
    frame #2: 0x0000000107064547 XUL`ScopedXPCOMStartup::Initialize(this=0x00000001004044c0) + 135 at nsAppRunner.cpp:1551
    frame #3: 0x000000010706e46b XUL`XREMain::XRE_main(this=0x00007fff5fbfef18, argc=5, argv=0x00007fff5fbff818, aAppData=0x00007fff5fbff198) + 747 at nsAppRunner.cpp:4379
    frame #4: 0x000000010706e9d7 XUL`::XRE_main(argc=5, argv=0x00007fff5fbff818, aAppData=0x00007fff5fbff198, aFlags=0) + 103 at nsAppRunner.cpp:4485
    frame #5: 0x00000001000028cf firefox`do_main(argc=5, argv=0x00007fff5fbff818, xreDirectory=0x000000010040e400) + 1791 at nsBrowserApp.cpp:212
    frame #6: 0x0000000100001cf6 firefox`main(argc=5, argv=0x00007fff5fbff818) + 294 at nsBrowserApp.cpp:352
    frame #7: 0x0000000100001774 firefox`start + 52

BenWa - are these two calls to profiler_init expected? What should we be doing here?

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8698685 [details]
MozReview Request: Bug 1221846 - Get Task Tracer building on desktop r=cyu.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28073/diff/1-2/

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1221846 - Properly close the tasktracer property in the GeckoSampler JSON blob. r=BenWa

Review commit: https://reviewboard.mozilla.org/r/28103/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28103/

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1221846 - Avoid assertions from Task Tracer due to reentry on profiler init and shutdown. r?cyu

Review commit: https://reviewboard.mozilla.org/r/28105/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28105/

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Here's a profile that I shared that has tasktracer data in it: http://people.mozilla.org/~bgirard/cleopatra/#report=87365170f57d4f697fa5410f1f83a97b81df44b3

And here's the pull request for the Gecko Profiler add-on that adds the Task Tracer feature toggle: https://github.com/bgirard/Gecko-Profiler-Addon/pull/106

Comment 8

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

Comment on attachment 8698744 [details]
MozReview Request: Bug 1221846 - Avoid assertions from Task Tracer due to reentry on profiler init and shutdown. r?cyu

https://reviewboard.mozilla.org/r/28105/#review25159

r=me with the following addressed.

::: tools/profiler/tasktracer/GeckoTaskTracer.cpp:230
(Diff revision 1)
>    MOZ_ASSERT(!sTraceInfos);

We don't need this assertion since sTraceInfos is already checked right above it.

Comment 9

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

Comment on attachment 8698685 [details]
MozReview Request: Bug 1221846 - Get Task Tracer building on desktop r=cyu.

https://reviewboard.mozilla.org/r/28073/#review25163

::: tools/profiler/tasktracer/GeckoTaskTracer.cpp:31
(Diff revision 2)
> +#elif defined(LINUX) || defined(XP_MACOSX)

You can't use gettid() on MacOSX since it has totally different semantics. What needs to be done on MacOSX is like this: http://hg.mozilla.org/mozilla-central/file/94714c206f18/tools/profiler/core/platform-macos.cc#l362

::: tools/profiler/tasktracer/TracedTaskCommon.cpp:98
(Diff revision 2)
> -  LogVirtualTablePtr(mTaskId, mSourceEventId, *(int**)(aOriginalObj));
> +  nsCOMPtr<nsIRunnable> eventRef(aOriginalObj);

aOriginalObj is already moved in asssigning to mOriginalObj. eventRef will always be null. Just use mOriginalObj instead.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8698685 [details]
MozReview Request: Bug 1221846 - Get Task Tracer building on desktop r=cyu.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28073/diff/2-3/

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8698743 [details]
MozReview Request: Bug 1221846 - Properly close the tasktracer property in the GeckoSampler JSON blob. r=BenWa

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28103/diff/1-2/

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8698744 [details]
MozReview Request: Bug 1221846 - Avoid assertions from Task Tracer due to reentry on profiler init and shutdown. r?cyu

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28105/diff/1-2/

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Sounds like cervantes thinks re-entry into the Task Tracer init / shutdown methods should just be no-ops, so clearing needinfo.

Regarding gettid... I've added the OS X case, but I really have no idea if I'm doing the right thing in there. You might have to spell it out for me exactly.

Comment 14

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

Comment on attachment 8698744 [details]
MozReview Request: Bug 1221846 - Avoid assertions from Task Tracer due to reentry on profiler init and shutdown. r?cyu

https://reviewboard.mozilla.org/r/28105/#review25293

Comment 15

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

Comment on attachment 8698685 [details]
MozReview Request: Bug 1221846 - Get Task Tracer building on desktop r=cyu.

https://reviewboard.mozilla.org/r/28073/#review25301

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Hey cyu, so there's the second "symbolication" step that the tasktracer-converter.py step does in order to turn the samples into something useful.

That script, as far as I can tell, is extremely b2g-specific. Are we planning on putting that step into the Gecko Profiler Add-on? We seem to do similar symbolication work in add-on for normal profiles.

Comment 17

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

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #16) 
> That script, as far as I can tell, is extremely b2g-specific. Are we
> planning on putting that step into the Gecko Profiler Add-on? We seem to do
> similar symbolication work in add-on for normal profiles.
That's something nice to have, but symbolication in Task Tracer doesn't fit pretty well in the addon and cannot reuse the symbolication work. Task Tracer it uses nm (and it needs to because the address to symbolicate is vtable), while the addon uses addr2line, which doesn't symbolicate a vtable. That part of calculation needs to be ported from tasktracer-converter.py.

Comment 18

[Benoit Girard (:BenWa)]

Comment on attachment 8698743 [details]
MozReview Request: Bug 1221846 - Properly close the tasktracer property in the GeckoSampler JSON blob. r=BenWa

https://reviewboard.mozilla.org/r/28103/#review25403

Comment 19

[Benoit Girard (:BenWa)]

Nephthys pull request to receive Cleopatra profiles when embedded into a tab:
https://github.com/alivedise/nephthys/pull/36

Comment 20

[Benoit Girard (:BenWa)]

(In reply to Cervantes Yu [:cyu] [:cervantes] from comment #17)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #16) 
> > That script, as far as I can tell, is extremely b2g-specific. Are we
> > planning on putting that step into the Gecko Profiler Add-on? We seem to do
> > similar symbolication work in add-on for normal profiles.
> That's something nice to have, but symbolication in Task Tracer doesn't fit
> pretty well in the addon and cannot reuse the symbolication work. Task
> Tracer it uses nm (and it needs to because the address to symbolicate is
> vtable), while the addon uses addr2line, which doesn't symbolicate a vtable.
> That part of calculation needs to be ported from tasktracer-converter.py.

I'm not sure on all the difference between nm/addr2line but fundamentally the aim of both is to resolve addresses into useful strings (symbolicate). To support desktop we need to symbolicate from the add-on in order to support windows which supports neither nm or addr2line but rather needs to use the symbol server.

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8698685 [details]
MozReview Request: Bug 1221846 - Get Task Tracer building on desktop r=cyu.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28073/diff/3-4/

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8698743 [details]
MozReview Request: Bug 1221846 - Properly close the tasktracer property in the GeckoSampler JSON blob. r=BenWa

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28103/diff/2-3/

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/61cefe69917e
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e4bdac31c32

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/61cefe69917e
https://hg.mozilla.org/mozilla-central/rev/0e4bdac31c32

Comment 25

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

(In reply to Benoit Girard (:BenWa) from comment #20)
> I'm not sure on all the difference between nm/addr2line but fundamentally
> the aim of both is to resolve addresses into useful strings (symbolicate).
> To support desktop we need to symbolicate from the add-on in order to
> support windows which supports neither nm or addr2line but rather needs to
> use the symbol server.

Recording vtable allows us to know the concrete class of the runnables and lets us know what the runnables do to some extent. But the target now is to align TaskTracer and Cleopatra timelines, and Cleopatra provides whole stacks in its timeline. We may consider dropping vtable information on desktop.

Comment 26

[Markus Stange [:mstange]]

Comment on attachment 8698685 [details]
MozReview Request: Bug 1221846 - Get Task Tracer building on desktop r=cyu.

https://reviewboard.mozilla.org/r/28073/#review86814

::: tools/profiler/tasktracer/TracedTaskCommon.cpp:98
(Diff revision 4)
>  TracedRunnable::TracedRunnable(already_AddRefed<nsIRunnable>&& aOriginalObj)
>    : TracedTaskCommon()
>    , mOriginalObj(Move(aOriginalObj))
>  {
>    Init();
> -  LogVirtualTablePtr(mTaskId, mSourceEventId, *(int**)(aOriginalObj));
> +  LogVirtualTablePtr(mTaskId, mSourceEventId, reinterpret_cast<uintptr_t*>(mOriginalObj.get()));

This change broke vtable pointer logging - there was a dereference operation here before, which got lost in the change.

I'll file a bug about that and fix it.

Comment 27

[Markus Stange [:mstange]]

It looks like this was already found out about in bug 1287392, but the patches there haven't landed.