1340721 - We're not taking advantage of frame pointers in non-Nightly Windows 32bit builds for stack walking in the profiler

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Markus Stange [:mstange]]

Since bug 1322735 we now have frame pointers in 32bit builds on Windows, and we can use FramePointerStackWalk to get C++ stacks.

However, the profiler only attempts to walk the C++ stack if the profiler feature "stackwalk" is enabled, and that's conditional on the MOZ_PROFILING define, which is controlled by the --enable-profiling configure option, which is only enabled for Nightly and Aurora builds, but not on Beta / Release builds.

So the profiler won't attempt to get C++ stacks in Beta / Release builds, even though doing so on Windows in 32bit builds would be no problem.

That's ok for now since most people are profiling on Nightly these days, but it'll get more important once we make it easier for regular users to send profiles to us.

Comment 1

[(not currently active) Ted Mielczarek]

Is this just controlled by this ifdef here?
https://dxr.mozilla.org/mozilla-central/rev/f9362554866b327700c7f9b18050d7b7eb3d2b23/tools/profiler/core/platform.cpp#53

If so it should be easy enough to just switch that around to something like:
#if (defined(MOZ_PROFILING) && \
    (defined(GP_OS_windows) || defined(GP_OS_darwin))) ||
    (defined(GP_OS_windows) && defined(_M_IX86)))

Comment 2

[Markus Stange [:mstange]]

Possibly. Somebody would need to try it out.

Comment 3

[Markus Stange [:mstange]]

(In reply to Markus Stange [:mstange] from comment #0)
> that's conditional on the MOZ_PROFILING
> define, which is controlled by the --enable-profiling configure option,
> which is only enabled for Nightly and Aurora builds, but not on Beta /
> Release builds.

This was slightly wrong - it's only enabled on Nightly builds, but not on Aurora / Beta / Release.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: Enable profiler stack walking on Windows even for --disable-profiling builds

On Win32, stack frames are now always present, so we can always use
FramePointerStackWalk(). On Win64, stack frames are never present, so we always
use MozStackWalk().

In both cases, we can get stack traces no matter the value of MOZ_PROFILING. So
this patch removes MOZ_PROFILING from the relevant conditions. It also
restructures the conditions and adds some helpful comments.

Comment 5

[Markus Stange [:mstange]]

Comment on attachment 8889110 [details] [diff] [review]
Enable profiler stack walking on Windows even for --disable-profiling builds

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

This makes it much clearer what's going on. Thanks.

Comment 6

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0989b2e5e0b9f091c8c68a321061508dbf49d094
Bug 1340721 - Enable profiler stack walking on Windows even for --disable-profiling builds. r=mstange.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0989b2e5e0b9
Enable profiler stack walking on Windows even for --disable-profiling builds. r=mstange.

Comment 8

[Nicholas Nethercote [inactive]]

Comment on attachment 8889110 [details] [diff] [review]
Enable profiler stack walking on Windows even for --disable-profiling builds

Approval Request Comment
[Feature/Bug causing the regression]: N/A

[User impact if declined]: This will allow Firefox 55 users on beta (and eventually release) to get native stacks if using the Gecko Profiler. Otherwise, that won't work until Firefox 56.

[Is this code covered by automated tests?]: Somewhat.

[Has the fix been verified in Nightly?]: No. Nightly defines MOZ_PROFILING, and this patch changes the behaviour in non-MOZ_PROFILING builds (i.e. beta and release).

[Needs manual test from QE? If yes, steps to reproduce]: No.

[List of other uplifts needed for the feature/fix]: None.

[Is the change risky?]: It is low risk.

[Why is the change risky/not risky?]: It's a simple patch that just causes HAVE_NATIVE_UNWIND to be defined on Windows on some configurations that currently lack it.

[String changes made/needed]: None.

Comment 9

[Julien Cristau [:jcristau]]

Comment on attachment 8889110 [details] [diff] [review]
Enable profiler stack walking on Windows even for --disable-profiling builds

I think it's too late for 55 at this point

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0989b2e5e0b9