No native stacks on Windows AArch64

RESOLVED FIXED in Firefox 66

Status

()

RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: overholt, Assigned: glandium)

Tracking

unspecified
mozilla66
ARM64
Windows
Points:
---

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

3 months ago
https://perfht.ml/2B9Xr72 doesn't have any native stacks
Bug 1485716 was trying to fix this; it shipped with Firefox 63 and this profile was taken on Firefox 65. Maybe what landed in that bug never fixed it completely.
Blocks: 1485716
Flags: needinfo?(nfroyd)
OS: Unspecified → Windows
Hardware: Unspecified → ARM64
We're not uploading symbols for the aarch64 builds yet; does that make a difference, or should perf-html show the numeric addresses even if we don't have symbols?
Flags: needinfo?(nfroyd)
It should show the numeric addresses. There should be lots of those between the js::RunScript label frames, for example.
(In reply to Markus Stange [:mstange] from comment #3)
> It should show the numeric addresses. There should be lots of those between
> the js::RunScript label frames, for example.

Hm.  And I suppose we can rule out some of the bits we have in place for x86-64 Windows, e.g.

https://searchfox.org/mozilla-central/source/tools/profiler/core/platform-win32.cpp#135-141
https://searchfox.org/mozilla-central/source/tools/profiler/core/platform-win32.cpp#257-307

because Andrew was able to take a profile without crashing and/or hanging?

It's entirely possible we shouldn't be using frame pointers to unwind, or something.  I see that Microsoft's standards say that frame pointers are enabled within Windows code, and third-party code is encouraged to use frame pointers as well:

https://docs.microsoft.com/en-us/cpp/build/arm64-windows-abi-conventions?view=vs-2017#stack-walking

But we enabled frame pointers unconditionally on aarch64:

https://searchfox.org/mozilla-central/source/build/autoconf/frameptr.m4#33

so frame pointers *should* be the correct thing...
I have a feeling that we're going to have to use the RtlVirtualUnwind (i.e. not frame pointers) path eventually anyway, to play nice with our JIT region checking and whatnot. I think that generally we should try to make ARM64 more like AMD64 than X86.
Duplicate of this bug: 1514988
(Assignee)

Updated

3 months ago
Assignee: nobody → mh+mozilla
(Assignee)

Updated

3 months ago
Component: Gecko Profiler → mozglue
(Assignee)

Comment 7

3 months ago
Copy/paste from bug 1514988 comment 0:

As far as my stepping through WalkStackMain64 goes, it seems StackWalk64 doesn't work. I tried adding frame64.AddrReturn.Offset = Context->Lr, but that made no difference.

I tried switching to the same code as for x86-64, using RtlVirtualUnwind, and that appears to work... except we seem to be skipping too many frames. I'm now waiting for a new build with no frame skipping at all to see what the top frames are.
(Assignee)

Comment 8

3 months ago
As far as my stepping through WalkStackMain64 goes, it seems StackWalk64
doesn't work, even with more information added to the frame data it's
given.

Switching to the same code as for x86-64, however, works, albeit
skipping too many frames, but all platforms are actually skipping too
many frames, so let's ignore that for now and leave it to bug 1515229.
(Assignee)

Comment 10

3 months ago
Here's a profile I captured with a build with both patches applied, loading treeherder: https://perfht.ml/2BsOUML
(Assignee)

Comment 11

3 months ago
And here's a profile without the second patch: https://perfht.ml/2QI6y9R
A lot of C++ doesn't have proper traces in that case.

Comment 12

3 months ago
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/7eb93bcd0ee3
Switch aarch64-windows MozStackWalk code to RtlVirtualUnwind. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/c1c5fd1fed41
Use MozStackWalk in the profiler for aarch64 Windows. r=froydnj

Comment 13

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7eb93bcd0ee3
https://hg.mozilla.org/mozilla-central/rev/c1c5fd1fed41
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox66: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.