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

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mstange, Assigned: njn)

Tracking

Trunk
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 wontfix, firefox55 wontfix, firefox56 fixed)

Details

Attachments

(1 attachment)

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)))
Possibly. Somebody would need to try it out.
(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

2 years ago
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

2 years ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
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

2 years 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

2 years 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

2 years 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0989b2e5e0b9
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.