Closed Bug 1221846 Opened 4 years ago Closed 4 years ago

Get Task Tracer working on desktop

Categories

(Toolkit :: Performance Monitoring, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
e10s + ---
firefox46 --- fixed

People

(Reporter: blassey, Assigned: mconley)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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.
Assignee: nobody → mconley
Priority: -- → P1
Status: NEW → ASSIGNED
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/
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?
Flags: needinfo?(bgirard)
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/
Attachment #8698685 - Attachment description: MozReview Request: Bug 1221846 - Get Task Tracer working on desktop → MozReview Request: Bug 1221846 - Get Task Tracer working on desktop r?cyu.
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
Attachment #8698744 - Flags: review?(cyu)
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 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.
Attachment #8698685 - Flags: review?(cyu)
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/
Attachment #8698685 - Flags: review?(cyu)
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 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/
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.
Flags: needinfo?(bgirard)
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
Attachment #8698744 - Flags: review?(cyu) → review+
Comment on attachment 8698685 [details]
MozReview Request: Bug 1221846 - Get Task Tracer building on desktop r=cyu.

https://reviewboard.mozilla.org/r/28073/#review25301
Attachment #8698685 - Flags: review?(cyu) → review+
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.
Flags: needinfo?(cyu)
(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.
Flags: needinfo?(cyu)
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
Attachment #8698743 - Flags: review?(bgirard) → review+
Depends on: 1233835
Nephthys pull request to receive Cleopatra profiles when embedded into a tab:
https://github.com/alivedise/nephthys/pull/36
Priority: P1 → P2
(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 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/
Attachment #8698685 - Attachment description: MozReview Request: Bug 1221846 - Get Task Tracer working on desktop r?cyu. → MozReview Request: Bug 1221846 - Get Task Tracer building on desktop r=cyu.
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/
Attachment #8698743 - Attachment description: MozReview Request: Bug 1221846 - Properly close the tasktracer property in the GeckoSampler JSON blob. r?BenWa → MozReview Request: Bug 1221846 - Properly close the tasktracer property in the GeckoSampler JSON blob. r=BenWa
https://hg.mozilla.org/mozilla-central/rev/61cefe69917e
https://hg.mozilla.org/mozilla-central/rev/0e4bdac31c32
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
(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.
See Also: → 1167634
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.
It looks like this was already found out about in bug 1287392, but the patches there haven't landed.
Depends on: 1287392
You need to log in before you can comment on or make changes to this bug.