1451902 - Consider shipping release builds with frame pointers enabled on macOS

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Markus Stange [:mstange]]

On macOS, our Nightlies are built with frame pointers enabled, ever since bug 764216. However, our Beta and Release builds are not.

The only way that the Gecko Profiler is able to unwind native stacks on macOS is using frame pointers. This means that we only have working stackwalking support on Nightly, and not on release builds. If our users submit a profile that was taken on a Mac release build, that profile does not contain any native stacks; it only contains JS stacks and pseudo stacks but is otherwise rather flat and unhelpful.

On other platforms, the situation is as follows:
 - On Windows 32 bit builds, we have chosen to take the hit and have enabled frame pointers on release. This was done in bug 1322735.
 - On Windows 64 bit builds, there is no way to enable frame pointers. Stackwalking in those builds makes use of unwinding tables.
 - On Linux, 32 bit and 64 bit, the situation is the same as on Mac: We have frame pointers on Nightly but not on Beta or Release. On Beta and Release, we use lul for unwinding, which consults unwinding tables in the binary.

Lul does not currently support unwinding with unwinding tables on macOS. It could be extended to support it but it would be some work.

The easiest way to get useful profiles from release users would be to enable frame pointers on Beta and Release builds on macOS. This might come with a small perf impact.

Comment 1

[Gregory Szorc [:gps]]

Why did we disable frame pointers in the first place? To have an extra register available to the compiler? Bug 492688 seems to indicate possibly so.

Assuming this is about performance, the decision to omit frame pointers was made when we used 32-bit builds pretty much everywhere. My understanding is the extra register doesn't buy you much on 64-bit processors because 64-bit processors have more registers available. I think having stack unwinding and a consistent build between Nightly and release is more important than <some> raw performance. Quantifying <some> is not something I'm comfortable answering.

So if this is about performance, let's throw some builds through our perf tests so we can make a data-informed decision?

ted: you live at the intersection of builds, crash handling, and are a general repository of knowledge. Anything you'd like to add?

Comment 2

[Eric Rahm [:erahm]]

(In reply to Markus Stange [:mstange] from comment #0)
> The easiest way to get useful profiles from release users would be to enable
> frame pointers on Beta and Release builds on macOS. This might come with a
> small perf impact.

On the Win32 side there were only a couple small regressions on PGO builds (bug 1322735, comment 12) and a 0.19% increase in the size of libxul. That might be a good baseline for evaluating what we deem as okay.

Comment 3

[(not currently active) Ted Mielczarek]

(In reply to Gregory Szorc [:gps] from comment #1)
> Why did we disable frame pointers in the first place? To have an extra
> register available to the compiler? Bug 492688 seems to indicate possibly so.

That sounds right, but that decision was obviously made when we were shipping x86 binaries on Mac, not x86-64. I doubt we've ever revisited that decision in light of shipping on x86-64. The odd thing is that on x86-64 the ABI spec says that frame pointers should be omitted by default despite having plenty of registers because the ABI also mandates having unwind info (like .eh_frame). Despite that, it looks like clang on Mac doesn't actually default to omitting frame pointers.

Because of the amount of available registers on x86-64 compared to x86 the perf hit should be much smaller. It would be entirely due to having the extra instructions for pushing and popping the frame pointer register in each function, and the slight codesize increase due to this.

(In reply to Markus Stange [:mstange] from comment #0)
>  - On Linux, 32 bit and 64 bit, the situation is the same as on Mac: We have
> frame pointers on Nightly but not on Beta or Release. On Beta and Release,
> we use lul for unwinding, which consults unwinding tables in the binary.
> 
> Lul does not currently support unwinding with unwinding tables on macOS. It
> could be extended to support it but it would be some work.

Is there any particular reason here other than nobody did the work? Mach-O binaries use the same eh_frame data as ELF binaries, so it doesn't seem like it'd be that hard to wire this up. Presumably it's just a matter of dealing with the platform-specific bits?

I would generally defer this decision to someone with better knowledge of the CPU architecture like jrmuizel. It should be easy enough to flip the setting, push to try, and get talos data for comparison.

Comment 4

[Julian Seward [:jseward]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #3)
> > Lul does not currently support unwinding with unwinding tables on macOS. It
> > could be extended to support it but it would be some work.
> 
> Is there any particular reason here other than nobody did the work? Mach-O
> binaries use the same eh_frame data as ELF binaries, so it doesn't seem like
> it'd be that hard to wire this up. Presumably it's just a matter of dealing
> with the platform-specific bits?

I think this didn't happen because we've never seriously considered using
LUL to unwind on OSX (until now).  It wouldn't be a big deal to write a
Mach-O parser for LUL if we wanted.  That would, as Ted implies, find the
relevant Dwarf unwind info sections, which we'd just hand off to LUL as at
present.

However, isn't there some complication in which Mach-O debuginfo is not
linked into .dsym files, but held separately?  I seem to remember some
complication with this, and the use of the dsymutil program, as a workaround
from Valgrind-on-OSX days.  But I'm not sure of the details and also not
sure if this is actually relevant here.

Comment 5

[(not currently active) Ted Mielczarek]

You're correct, but thankfully Apple's toolchain does put .eh_frame directly in the binary itself:

$ objdump --section-headers /Applications/Firefox.app/Contents/MacOS/firefox

/Applications/Firefox.app/Contents/MacOS/firefox:     file format mach-o-x86-64

Sections:
Idx Name          Size      VMA               LMA               File off  Algn
  0 .text         00000ced  0000000100000cd0  0000000100000cd0  00000cd0  2**4
                  CONTENTS, ALLOC, LOAD, CODE
  1 __TEXT.__stubs 000000d8  00000001000019be  00000001000019be  000019be  2**1
                  CONTENTS, ALLOC, LOAD, READONLY, CODE
  2 __TEXT.__stub_helper 00000164  0000000100001a98  0000000100001a98  00001a98  2**2
                  CONTENTS, ALLOC, LOAD, READONLY, CODE
  3 .cstring      000001c5  0000000100001bfc  0000000100001bfc  00001bfc  2**0
                  CONTENTS, ALLOC, LOAD, READONLY, DATA
  4 __TEXT.__unwind_info 00000088  0000000100001dc4  0000000100001dc4  00001dc4  2**2
                  CONTENTS, ALLOC, LOAD, READONLY, CODE
  5 .eh_frame     000001b0  0000000100001e50  0000000100001e50  00001e50  2**3
                  CONTENTS, ALLOC, LOAD, READONLY, DATA
<...>

There's also that __unwind_info section which is unwind info in Apple's "compact unwind encoding":
https://github.com/llvm-mirror/libunwind/blob/master/include/mach-o/compact_unwind_encoding.h

I've never dug into it but it looks extremely similar to the ARM unwind tables or the Win64 unwind info.

Comment 6

[Julian Seward [:jseward]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #5)
> There's also that __unwind_info section which is unwind info in Apple's
> "compact unwind encoding":

Without looking at it in detail .. I'd guess those could be converted into
LUL's internal unwind records at the point LUL reads the info.  So then
it could seamlessly unwind with any mixture of "compact unwind" and
traditional Dwarf CFI.  Some time in the past there was a similar scheme
to read ARM32 EXIDX into LUL (it worked, but has since been removed).

Comment 8

[Markus Stange [:mstange]]

The conversation about LUL aside, I think we should just find out how much framepointers affect our performance. Stackwalking using framepointers is simpler, faster, and often more reliable than using more involved methods of unwinding. And LUL adds serious startup overhead.

(I've copied the comments about LUL into bug 1635812, and collapsed them in this bug.)

Comment 10

[Markus Stange [:mstange]]

Let's do this for the arm64 builds now, and for x86_64 builds later.

Comment 11

[Markus Stange [:mstange]]

I've done a tryserver comparison for x86_64 macOS, for Talos and browsertime.

Try push: No frame pointers (like release)
Try push: With frame pointers (like nightly)

Talos comparison
Browsertime comparison

The vast majority of performance tests and benchmarks are completely unaffected by frame pointers. There are two Talos tests which show a difference with "high" confidence:

• "a11yr dhtml.html opt e10s fission stylo webrender-sw" reports a regression of 2.5%.
• "perf_reftest_singletons opt e10s fission stylo webrender" reports an improvement; specifically the "bloom-basic.html" and "bloom-basic-2.html" subtests report an improvement of 20-25% with high confidence.

I am rather confused by the improvements, but with 10 test runs on each push and such a strong signal I'll have to concede that they're probably real.
The "a11yr dhtml.html" regression is probably real, too. I'm not very familiar with this tests but if this is the only regression, 2.5% on it seems like a small price to pay. (I didn't get profiles of this test due to bug 1753483.)

Comment 12

[Markus Stange [:mstange]]

Attachment: Bug 1451902 - Enable framepointers for all macOS builds on all channels. r=glandium

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/65c90081484d
Enable framepointers for all macOS builds on all channels. r=glandium

Comment 14

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/65c90081484d