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

RESOLVED FIXED in Firefox 56

Status

()

Core
Gecko Profiler
RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: mstange, Assigned: njn)

Tracking

Trunk
mozilla56
Points:
---

Firefox Tracking Flags

(firefox54 wontfix, firefox55 wontfix, firefox56 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

a year ago
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.
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)))
(Reporter)

Comment 2

a year ago
Possibly. Somebody would need to try it out.
(Reporter)

Comment 3

a year ago
(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.
(Assignee)

Comment 4

10 months ago
Created attachment 8889110 [details] [diff] [review]
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.
Attachment #8889110 - Flags: review?(mstange)
(Assignee)

Updated

10 months ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
(Reporter)

Comment 5

10 months ago
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.
Attachment #8889110 - Flags: review?(mstange) → review+
(Assignee)

Comment 6

10 months ago
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

10 months ago
Pushed by nnethercote@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0989b2e5e0b9
Enable profiler stack walking on Windows even for --disable-profiling builds. r=mstange.
(Assignee)

Comment 8

10 months ago
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.
Attachment #8889110 - Flags: approval-mozilla-beta?
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
Attachment #8889110 - Flags: approval-mozilla-beta? → approval-mozilla-beta-

Comment 10

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0989b2e5e0b9
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

Updated

10 months ago
status-firefox54: affected → wontfix
status-firefox55: --- → wontfix
You need to log in before you can comment on or make changes to this bug.