Closed Bug 1340721 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Gecko Profiler, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox54 --- wontfix
firefox55 --- wontfix
firefox56 --- fixed

People

(Reporter: mstange, Assigned: n.nethercote)

References

Details

Attachments

(1 file)

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.
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: 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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/0989b2e5e0b9f091c8c68a321061508dbf49d094
Bug 1340721 - Enable profiler stack walking on Windows even for --disable-profiling builds. r=mstange.
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.
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-
https://hg.mozilla.org/mozilla-central/rev/0989b2e5e0b9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.