1957535 - Consider switching from Lul to framepointer unwinding an aarch64 Android

Status: REOPENED

(Core :: Gecko Profiler, task, P2)

Description

[Markus Stange [:mstange]]

I think it's worth trying to switch to framepointer unwinding on Android aarch64 for the moment and seeing how far it gets us.

The background is the following:

• We are currently focusing on optimizing startup time, and lul's startup overhead (bug 1635810) interferes with accurate startup profiling.
• On some devices, even with lul we don't get accurate stacks (bug 1957534).
• We could consider adding a way to pick between unwinders (bug 1947589), but it's not obvious how:
  • a profiler feature impacts many consumers, requires UI + user-facing strings, and you still need to pick a default (and rename the feature when you decide to change the default)
  • a pref wouldn't work because unwinding is needed during startup profiling before the pref system is initialized

So for simplicity, let's just switch the ifdefs around. If we start seeing lots of profiles with bad stacks that were correct before, we can back the patch out.

Comment 1

[Markus Stange [:mstange]]

Attachment: Bug 1957535 - Switch to using frame pointer stack walking on Android aarch64 and Android x86_64. r=#profiler-reviewers

Firefox is shipping with frame pointers everywhere these days, and
many aarch64 Android system libraries appear to be shipping with frame
pointers these days as well.

Using frame pointers for stack walking instead of DWARF + Lul avoids
Lul's initialization cost.

This patch is also changing the Android x86_64 path, mostly for simplicity;
I believe that configuration mostly just affects the Android emulator,
where I wouldn't expect the Gecko profiler to be used very much.

Comment 2

[Markus Stange [:mstange]]

Try push: https://treeherder.mozilla.org/jobs?repo=try&revision=9ef552ddb1334e6037534cec98364f64c867c5e6

Profile with patch: https://share.firefox.dev/41TadEG
No more startup delays! 🎉

However, it looks like the Mali driver doesn't have frame pointers: https://share.firefox.dev/42bguui :(

Overall it might still be an acceptable trade-off, but it does suggest that it would be nice to have a way to force lul to be used.

Comment 3

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

It looks good to me even without a mali driver, but I'm not so sure how crucial that is to graphics people.

Overall it might still be an acceptable trade-off, but it does suggest that it would be nice to have a way to force lul to be used.

Yeah, it would be nice to have it still. Do you think it's enough to have an environment variable to force lul or should we have a UI for it?

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/5a2555896415
https://hg.mozilla.org/integration/autoland/rev/7e0bbd6bdb07
Switch to using frame pointer stack walking on Android aarch64 and Android x86_64. r=profiler-reviewers,canaltinova

Comment 5

[Markus Stange [:mstange]]

(In reply to Nazım Can Altınova [:canova][:canaltinova on phabricator] from comment #3)

Overall it might still be an acceptable trade-off, but it does suggest that it would be nice to have a way to force lul to be used.

Yeah, it would be nice to have it still. Do you think it's enough to have an environment variable to force lul or should we have a UI for it?

I've gone ahead and landed it without an option to opt-in. We can add it if we notice trouble.

Comment 6

[[e-mail]]

https://hg.mozilla.org/mozilla-central/rev/7e0bbd6bdb07

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/2854fd88dc10
https://hg.mozilla.org/integration/autoland/rev/c6fdd7e3808e
Revert "Bug 1957535 - Switch to using frame pointer stack walking on Android aarch64 and Android x86_64. r=profiler-reviewers,canaltinova" for causing Bug 1982813

Comment 8

[Markus Stange [:mstange]]

Turns out we don't have framepointers on Android on Beta+Release: frame_pointer_default defaults to off for Android, but framepointers end up getting enabled by enable_profiling on Nightly.

I think we should just enable them on all channels for Android aarch64, but it requires a try push + perfcompare.

Comment 9

[Markus Stange [:mstange]]

I've filed bug 1982862 about enabling framepointers on aarch64 Beta + Release.

Comment 10

[Cosmin Sabou [:CosminS]]

Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/c6fdd7e3808e