1457481 - Add nsIProfiler.GetSymbolTable and implement it in C++ / rust in order to obtain symbol tables from system libraries on Android

Status: RESOLVED FIXED

(Core :: Gecko Profiler, enhancement, P1)

Description

[Markus Stange [:mstange]]

The primary motivation for this is to have the ability to dump symbol tables from system libraries on Android, on the device itself.

Rather than transmitting the system library files to a Desktop machine and extracting their symbol table there, we can ship a bit of code in the Firefox binary which knows how to extract symbols from ELF object files (for example using the rust "object" crate) without the need for a command line tool like nm. Then, when you profile Firefox for Android using the new devtools performance panel on a desktop machine, we can call nsIProfiler.GetSymbolTable on the device and transmit the symbol table over to the desktop machine.

This also allows for a profiling workflow that doesn't involve a connection to a desktop machine at all. This is something that Ehsan has proposed: We could have an add-on for Firefox for Android which only knows how to record, symbolicate and upload profiles, but not display them. That way you'd be able to record profiles on your phone on the go, without a desktop machine around, and then inspect the collected profiles later. In that scenario, symbolication of system libraries would happen on the phone, and symbolication of libxul would happen in perf.html (using the mozilla symbol server) once you look at the collected profile in your desktop browser.

Having symbolication code in the platform also enables the following use cases:
 - Symbolication of system libraries on macOS on machines that don't have Xcode installed and thus don't have "nm"
 - Getting inline stacks from debug information on macOS and linux, using the addr2line crate (includes file + line number information)
 - Getting symbol information on Windows using msdia*.dll, which would be a lot faster than waiting for dump_syms
 - Getting inline stacks from debug information on Windows, also using msdia*.dll, without having to wait for the rust pdb crate to add support for the relevant information

Comment 1

[Markus Stange [:mstange]]

I am going to implement this for Android only for now. That's the only place where we don't get any support from the OS to obtain symbol tables. There's no nm binary on the phone.

Comment 2

[Markus Stange [:mstange]]

Attachment: Bug 1457481 - Add a MOZ_GECKO_PROFILER_PARSE_ELF define that's only true on Android.

This will be used to conditionally compile the rust code for ELF binary parsing,
which will be used by the profiler to dump symbols from system libraries on
Android.

Ideally I'd like to make this only apply to Nightly + Beta configurations, and
not to Release, but there doesn't seem to be an easy way to differentiate
between Beta and Release and doing so might be frowned upon. So now it's going
to be built on all channels on Android, even Release, even though developers
won't be profiling Release channel builds much, and the extra code size isn't
all that valuable for our users.

We definitely need this code to be included on the Beta channel, though, because
Firefox Focus Nightly uses GeckoView from the Beta channel, and we want to get
good profiling information from Focus.

Comment 3

[Markus Stange [:mstange]]

Attachment: Bug 1457481 - Add nsIProfiler.GetSymbolTable and a profiler/rust-helper crate which implements it for ELF binaries. r?njn,jrmuizel

r?njn for the profiler parts
r?jrmuizel for the ELF parsing parts

Depends on D7020

Comment 4

[Markus Stange [:mstange]]

Attachment: Bug 1457481 - Run mach vendor rust. r?froydnj,erahm

Most importantly, this picks up "object" and "goblin" for ELF binary parsing.
We only use the ELF code from goblin, so the mach-O parsing code gets
eliminated by the linker. Overall, this increases the Android installer size
by 20KB.

Try pushes for reference:
before: https://treeherder.mozilla.org/#/jobs?repo=try&revision=834b56dc5ab3d63a43a32f740ee8212296ac726d&selectedJob=201600899
after: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6983b27e8d3cb715d3b7e6cbd276683f6466e3cc&selectedJob=201600475

installer size: 34524820 -> 34542861 (34.52MB -> 34.54MB)

$ mach vendor rust
    Updating registry `https://github.com/rust-lang/crates.io-index`
      Adding goblin v0.0.17
      Adding memmap v0.6.2
      Adding miniz-sys v0.1.10
      Adding object v0.10.0
      Adding parity-wasm v0.31.3
      Adding plain v0.2.3
      Adding profiler_helper v0.1.0 (file:///Users/mstange/code/mozilla/tools/profiler/rust-helper)
      Adding scroll v0.9.1
      Adding scroll_derive v0.9.5
      Adding syn v0.15.5
      Adding thin-vec v0.1.0
      Adding uuid v0.6.5
 0:30.11 The following files exceed the filesize limit of 102400:

third_party/rust/miniz-sys/miniz.c
third_party/rust/syn-0.14.6/src/expr.rs
third_party/rust/syn-0.14.6/src/gen/fold.rs
third_party/rust/syn-0.14.6/src/gen/visit.rs
third_party/rust/syn-0.14.6/src/gen/visit_mut.rs

The syn dependency is not compiled for goblin, as far as I can tell - it's only
needed for the 'syn' feature of scroll_derive, and scroll does not ask for
scroll_derive/syn.

object -> goblin -> scroll -> scroll_derive -/-> syn

But it looks like other versions of syn were already in the tree.

Depends on D7021

Comment 5

[Markus Stange [:mstange]]

Attachment: Bug 1457481 - Hook up the new devtools performance panel to nsIProfiler.getSymbolTable. r?gregtatum

Depends on D7023

Comment 6

[Markus Stange [:mstange]]

Here's a profile obtained from my Android phone using a macOS build that includes the last patch on a GeckoView-example.apk from this try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6983b27e8d3cb715d3b7e6cbd276683f6466e3cc&selectedJob=201600475

http://bit.ly/2Q9g0OI

It has full symbols for both libxul.so and for system libraries, for example libESXGLESv2_adreno.so.

Comment 7

[Nicholas Nethercote [inactive]]

Comment on attachment 9012410 [details]
Bug 1457481 - Add nsIProfiler.GetSymbolTable and a profiler/rust-helper crate which implements it for ELF binaries. r?njn,jrmuizel

Nicholas Nethercote [:njn] has approved the revision.

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 9012407 [details]
Bug 1457481 - Add a MOZ_GECKO_PROFILER_PARSE_ELF define that's only true on Android.

Ted Mielczarek [:ted] [:ted.mielczarek] has approved the revision.

Comment 9

[Markus Stange [:mstange]]

Here's what I did compared to my initial implementation in order to cut down on code size:
 - I made it Android-only
 - I removed the demangling code, which saved about 200KB in binary size. Instead, demangling is now performed by perf.html (we added a WebAssembly module to it which does C++ and rust demangling).
 - I "forked" the ELF binary ID divination code from https://github.com/getsentry/symbolic/blob/master/debuginfo/src/elf.rs in order to eliminate the dependencies regex, symbolic-common/with_dwarf, and others

The biggest remaining factor seems to be the binary parsing code in goblin, which uses scroll's Pread, which seems to generate more code than necessary because it does bounds checks and copies per element instead of per known-size struct. Jeff filed https://github.com/m4b/scroll_derive/issues/8 about this problem.

Comment 10

[Nathan Froyd [:froydnj]]

Comment on attachment 9012412 [details]
Bug 1457481 - Run mach vendor rust. r?froydnj,erahm

Nathan Froyd [:froydnj] has approved the revision.

Comment 11

[Greg Tatum [:gregtatum]]

Comment on attachment 9012413 [details]
Bug 1457481 - Hook up the new devtools performance panel to nsIProfiler.getSymbolTable. r?gregtatum

Greg Tatum [:gregtatum] has approved the revision.

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 9012410 [details]
Bug 1457481 - Add nsIProfiler.GetSymbolTable and a profiler/rust-helper crate which implements it for ELF binaries. r?njn,jrmuizel

Jeff Muizelaar [:jrmuizel] has approved the revision.

Comment 13

[Eric Rahm [:erahm]]

Comment on attachment 9012412 [details]
Bug 1457481 - Run mach vendor rust. r?froydnj,erahm

Eric Rahm [:erahm] has approved the revision.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1c8460b1d6da
Add a MOZ_GECKO_PROFILER_PARSE_ELF define that's only true on Android. r=ted
https://hg.mozilla.org/integration/autoland/rev/4478820fbcaa
Add nsIProfiler.GetSymbolTable and a profiler/rust-helper crate which implements it for ELF binaries. r=njn,jrmuizel
https://hg.mozilla.org/integration/autoland/rev/ac3deff9340f
Run mach vendor rust. r=froydnj,erahm
https://hg.mozilla.org/integration/autoland/rev/212450f77860
Hook up the new devtools performance panel to nsIProfiler.getSymbolTable. r=gregtatum

Comment 15

[Stefan Hindli [:stefan_hindli]]

Backed out 4 changesets (Bug 1457481) for c1 failures in devtools/client/performance-new/test/chrome/test_perf-settings-entries.html

https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed,busted,exception&classifiedState=unclassified&fromchange=b96cec26d463643b2dc0347930cf2561bfcb0071&selectedJob=202728416

https://treeherder.mozilla.org/logviewer.html#?job_id=202728416&repo=autoland&lineNumber=1926

Comment 16

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/00eb79fc9f8a
Backed out 4 changesets for c1 failures in devtools/client/performance-new/test/chrome/test_perf-settings-entries.html

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7c0eb6f58c1c
Add a MOZ_GECKO_PROFILER_PARSE_ELF define that's only true on Android. r=ted
https://hg.mozilla.org/integration/autoland/rev/7566a6bac33d
Add nsIProfiler.GetSymbolTable and a profiler/rust-helper crate which implements it for ELF binaries. r=njn,jrmuizel
https://hg.mozilla.org/integration/autoland/rev/255e04ebe3bd
Run mach vendor rust. r=froydnj,erahm
https://hg.mozilla.org/integration/autoland/rev/c4a515cf1d8f
Hook up the new devtools performance panel to nsIProfiler.getSymbolTable. r=gregtatum

Comment 18

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/7c0eb6f58c1c
https://hg.mozilla.org/mozilla-central/rev/7566a6bac33d
https://hg.mozilla.org/mozilla-central/rev/255e04ebe3bd
https://hg.mozilla.org/mozilla-central/rev/c4a515cf1d8f