Closed Bug 1322735 Opened 8 years ago Closed 8 years ago

Disable frame pointer omission (FPO) on win32

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Platform:
x86
Windows
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox50 wontfix, firefox51 wontfix, firefox52 fixed, firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: erahm, Assigned: erahm)

References

Details

Attachments

(3 files, 3 obsolete files)

Bug 1322735 - Remove MOZ_STACKWALKING define.
58 bytes, text/x-review-board-request
glandium
: review+
jcristau
: approval-mozilla-beta+
Details
Bug 1322735 - Remove OBSERVE_LATE_WRITES define.
58 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1322735 - Enable frame pointers on 32-bit windows builds.
58 bytes, text/x-review-board-request
glandium
: review+
jcristau
: approval-mozilla-beta+
Details
Without frame pointers on Win32 we can't retrieve useful stacks without minidumps. In order to support sending simple crash stacks that are usable via telemetry it is necessary to disable FPO on beta and release. It is currently disabled on nightly/aurora due to having profiling enabled. Background: - Windows itself has disabled FPO in the operating system - In the past (2008?) measurements showed perf improvements by enabling FPO, this needs to be reevaluated. Microsoft released a research paper indicating there aren't significant gains [1] - There are possible binary size concerns In order to evaluate these concerns we can: - Perform talos comparisons on try - Measure xul.dll sizes on FPO and non-FPO builds - Run benchmarks (dromaeo, etc) on low-end to high-end machines Other factors: - We will be transitioning more (most?) users on 64-bit platforms to 64-bit builds in the near future (bug 1274659) which will reduce the size of population impacted by any regressions As a final point I suggest we get this integrated in sync with support for crash stacks in telemetry (bug 1280477, bug 1317968), so either 53 or 52. One part we still need to define is criteria for acceptable regressions.
Attached patch WIP: Just turn on stackwalking (obsolete) — — Splinter Review
It seems that some paths expect stackwalking to be enabled on nightly builds and don't check MOZ_STACKWALKING. This just enables it always. A more elaborate patch would remove MOZ_STACKWALKING completely.
Assignee: nobody → erahm
Status: NEW → ASSIGNED
This disables profiling so that we can test with FPO *enabled* on try. It's a throw-away patch for when we finally land.
This universally disables FPO on win32.
Talos results are in for a try build [1]: It's mostly a wash, but there's ~4% regression in dromaeo-css, and a ~7% improvement in tsvgx. xul.dll increased in size by 0.19%. [1] https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=ce09fb9c5623&newProject=try&newRevision=27ee165f41e4d845678a92edca05b3468e92fe8a&framework=1&showOnlyImportant=0
As follow-up it would be nice to run a comparison across PGO builds, but for now I feel comfortable with these numbers. If necessary we could loop in folks from the WebVM/Layout teams to look at the dromaeo-css regression.
I don't know that Microsoft's performance statements are *quite* applicable, since new Windows releases don't get microbenchmarked to death by the tech press like we do, but I still think this is a good idea. :) It would be good to do a second pair of pushes with PGO enabled, since that's more representative of what we actually ship to users. You can just put MOZ_PGO=1 in the in-tree mozconfigs to get PGO builds.
Try can do pgo builds on its own, now, without touching mozconfigs.
(In reply to Mike Hommey [:glandium] from comment #8) > Try can do pgo builds on its own, now, without touching mozconfigs. Not according to trychooser [1], the wiki [2], or bug 691673. [1] http://trychooser.pub.build.mozilla.org/ [2] https://wiki.mozilla.org/ReleaseEngineering/TryChooser#What_if_I_want_PGO_for_my_build
All of which are outdated, then. I'm not sure whether they can be triggered via try syntax (you may try adding -pgo, it might work), but you can at least trigger them on treeherder itself.
If TaskCluster doesn't accept -pgo via Try syntax, please file a TaskCluster :: Task Configuration bug. Triggering Buildbot PGO via Treeherder is recommended, as that ensures e.g. Perfherder results are reported properly.
Now with PGO [1]: "high" confidence - Dromaeo CSS 3% regression remains - tpaint e10s-only 5% regression (this seems odd, I'd expect a similar regression on non-e10s) - ts_paint 2-2.5% regression So maybe some concern around the t(s_)paint numbers? I still feel good about doing this but it would help if whoever understands those tests chimes in. [1] https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=2d64999e6d06&newProject=try&newRevision=75fb449567cb572de3fd5ef0d711710e1e0aa8d5&framework=1&showOnlyImportant=0
mconley, IIRC you have become something of a ts_paint expert. Any thoughts on those results?
Flags: needinfo?(mconley)
So how I'm interpreting this is that FPO buys us ~20ms of time when painting the first browser window. Disabling FPO buys us easy-to-get-stacks, which I like. To me, that's worth the 20ms for ts_paint. But I'm not certain it's my call, really. You might want to clear it with someone from Product?
Flags: needinfo?(mconley)
The OBSERVE_LATE_WRITES removal is incidental clean-up, right? AFAICT it's not specifically related to FPO.
(In reply to Eric Rahm [:erahm] from comment #1) > Created attachment 8817672 [details] [diff] [review] > WIP: Just turn on stackwalking dmajor, does enabling MOZ_STACKWALKING unconditionally on Win32 risk bringing the Win64 profiler hang bug 1263595 to Win32?
status-firefox50: --- → wontfix
status-firefox51: --- → affected
Flags: needinfo?(dmajor)
See Also: → 1263595
Attachment #8817672 - Attachment is obsolete: true
Attachment #8817673 - Attachment is obsolete: true
Attachment #8817674 - Attachment is obsolete: true
(In reply to Nicholas Nethercote [:njn] from comment #18) > The OBSERVE_LATE_WRITES removal is incidental clean-up, right? AFAICT it's > not specifically related to FPO. Yes, removing MOZ_STACKWALKING made it always defined.
(In reply to Chris Peterson [:cpeterson] from comment #19) > dmajor, does enabling MOZ_STACKWALKING unconditionally on Win32 risk > bringing the Win64 profiler hang bug 1263595 to Win32? It shouldn't. The /Rtl.*Function(Table|Entry)/ APIs are specific to Win64 because of the table-based exception handling on that platform.
Flags: needinfo?(dmajor)
(In reply to David Major [:dmajor] from comment #21) > (In reply to Chris Peterson [:cpeterson] from comment #19) > > dmajor, does enabling MOZ_STACKWALKING unconditionally on Win32 risk > > bringing the Win64 profiler hang bug 1263595 to Win32? > > It shouldn't. The /Rtl.*Function(Table|Entry)/ APIs are specific to Win64 > because of the table-based exception handling on that platform. However... could it potentially bring that nightly-only Win64 hang to all Win64 channels? After this change, what will be the difference between --enable-profiling versus not?
(In reply to David Major [:dmajor] from comment #22) > (In reply to David Major [:dmajor] from comment #21) > > (In reply to Chris Peterson [:cpeterson] from comment #19) > > > dmajor, does enabling MOZ_STACKWALKING unconditionally on Win32 risk > > > bringing the Win64 profiler hang bug 1263595 to Win32? > > > > It shouldn't. The /Rtl.*Function(Table|Entry)/ APIs are specific to Win64 > > because of the table-based exception handling on that platform. > > However... could it potentially bring that nightly-only Win64 hang to all > Win64 channels? > > After this change, what will be the difference between --enable-profiling > versus not? This shouldn't affect profiling. --enable-profiling just toggles MOZ_PROFILING, when MOZ_PROFILING was set we would also disable FPO, but now we just unconditionally do that.
I'm trying to understand how this affects our various platforms and how it affects the built-in profiler. Is this correct?: - This patch does not change anything about Windows 64, Linux or macOS builds. - On Windows 64 we already have frame pointers, even in release builds, but in release builds the profiler doesn't use them to walk stacks because it only enables its "stackwalk" feature in builds that have MOZ_PROFILING. - On release Linux + macOS builds (any architecture) we will still omit frame pointers.
(In reply to Markus Stange [:mstange] from comment #24) > I'm trying to understand how this affects our various platforms and how it > affects the built-in profiler. Is this correct?: > - This patch does not change anything about Windows 64, Linux or macOS > builds. > - On Windows 64 we already have frame pointers, even in release builds, but > in release builds the profiler doesn't use them to walk stacks because it > only enables its "stackwalk" feature in builds that have MOZ_PROFILING. > - On release Linux + macOS builds (any architecture) we will still omit > frame pointers. Correct, we're only changing the build config on win32 to disable FPO. It's possible the profiler was depending on MOZ_STACKWALK instead of MOZ_PROFILING (as MOZ_STACKWALK being defined could imply MOZ_PROFILING), I don't think that's the case but you can audit the changes to the profiler code to be certain [1]. [1] https://reviewboard.mozilla.org/r/99964/diff/1#10
(In reply to Markus Stange [:mstange] from comment #24) > - This patch does not change anything about Windows 64, Linux or macOS > builds. > - On Windows 64 we already have frame pointers, even in release builds, but > in release builds the profiler doesn't use them to walk stacks because it > only enables its "stackwalk" feature in builds that have MOZ_PROFILING. Do we actually have frame pointers on Win64 builds? I can't find anything that indicates that they're on by default. They're not necessary for unwinding though, as the Win64 ABI mandates unwind data in the binary for non-leaf functions. > - On release Linux + macOS builds (any architecture) we will still omit > frame pointers. Yes, but ELF binaries contain .eh_frame sections which has unwind info (and I'm fairly confident the LUL unwinder code can use it for unwinding).
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #26) > Yes, but ELF binaries contain .eh_frame sections which has unwind info (and > I'm fairly confident the LUL unwinder code can use it for unwinding). on x86_64 only.
Comment on attachment 8820470 [details] Bug 1322735 - Enable frame pointers on 32-bit windows builds. https://reviewboard.mozilla.org/r/99968/#review100940 ::: build/autoconf/frameptr.m4:26 (Diff revision 1) > + dnl We always disable FPO on win32 so that we can get usable stacks. > MOZ_ENABLE_FRAME_PTR="-Oy-" > - MOZ_DISABLE_FRAME_PTR="-Oy" > + MOZ_DISABLE_FRAME_PTR="-Oy-" That's the wrong thing to do. If you want to always enable frame pointer, change the MOZ_FRAMEPTR_FLAGS assignment condition further below. Also, this should be the first patch of the series. Or at least it should come before the removal of MOZ_STACKWALKING.
Attachment #8820470 - Flags: review?(mh+mozilla) → review-
Comment on attachment 8820468 [details] Bug 1322735 - Remove MOZ_STACKWALKING define. https://reviewboard.mozilla.org/r/99964/#review100944
Attachment #8820468 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8820469 [details] Bug 1322735 - Remove OBSERVE_LATE_WRITES define. https://reviewboard.mozilla.org/r/99966/#review100946
Attachment #8820469 - Flags: review?(mh+mozilla) → review+
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #26) > Do we actually have frame pointers on Win64 builds? I can't find anything > that indicates that they're on by default. They're not necessary for > unwinding though, as the Win64 ABI mandates unwind data in the binary for > non-leaf functions. I've analyzed a few minidumps a couple of weeks ago taken from the Win64 release builds and they didn't seem to have frame-pointers.
(In reply to Mike Hommey [:glandium] from comment #27) > (In reply to Ted Mielczarek [:ted.mielczarek] from comment #26) > > Yes, but ELF binaries contain .eh_frame sections which has unwind info (and > > I'm fairly confident the LUL unwinder code can use it for unwinding). > > on x86_64 only. I don't believe this is true. GCC defaults to building with `-fasynchronous-unwind-tables`, and so it generates an `.eh_frame` section even for IA-32 binaries. With GCC 4.8.4: luser@eye7:/build$ gcc -m32 -o hello32 hello.c luser@eye7:/build$ readelf -S hello32 | grep eh_frame [16] .eh_frame_hdr PROGBITS 080484dc 0004dc 00002c 00 A 0 0 4 [17] .eh_frame PROGBITS 08048508 000508 0000b0 00 A 0 0 4
That must have changed, because it definitely wasn't doing that in 4.4. Although, eh_frames might be a bigger increase in size than frame pointers.
Comment on attachment 8820470 [details] Bug 1322735 - Enable frame pointers on 32-bit windows builds. https://reviewboard.mozilla.org/r/99968/#review101166 ::: build/autoconf/frameptr.m4:26 (Diff revision 1) > + dnl We always disable FPO on win32 so that we can get usable stacks. > MOZ_ENABLE_FRAME_PTR="-Oy-" > - MOZ_DISABLE_FRAME_PTR="-Oy" > + MOZ_DISABLE_FRAME_PTR="-Oy-" I'll move the test below, reorder the patch.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #26) > (In reply to Markus Stange [:mstange] from comment #24) > > - This patch does not change anything about Windows 64, Linux or macOS > > builds. > > - On Windows 64 we already have frame pointers, even in release builds, but > > in release builds the profiler doesn't use them to walk stacks because it > > only enables its "stackwalk" feature in builds that have MOZ_PROFILING. > > Do we actually have frame pointers on Win64 builds? I can't find anything > that indicates that they're on by default. Bug 1239498 indicates that the reverse of what I said is true. On Windows 64 we never have frame pointers and you can't enable them; FPO is mandatory.
Comment on attachment 8820470 [details] Bug 1322735 - Enable frame pointers on 32-bit windows builds. https://reviewboard.mozilla.org/r/99968/#review104354 ::: build/autoconf/frameptr.m4:22 (Diff revision 2) > MOZ_DISABLE_FRAME_PTR="-fomit-frame-pointer" > if test "$CPU_ARCH" = arm; then > # http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54398 > MOZ_ENABLE_FRAME_PTR="$unwind_tables" > fi > - else > + fi Removing the else here breaks mingw-gcc builds. ::: build/autoconf/frameptr.m4:35 (Diff revision 2) > - # if we are debugging, profiling or using sanitizers, we want a frame pointer. > + # if we are debugging, profiling or using sanitizers, we want a frame pointer. > - if test -z "$MOZ_OPTIMIZE" -o \ > + if test -z "$MOZ_OPTIMIZE" -o \ > - -n "$MOZ_PROFILING" -o \ > + -n "$MOZ_PROFILING" -o \ > - -n "$MOZ_DEBUG" -o \ > + -n "$MOZ_DEBUG" -o \ > - -n "$MOZ_MSAN" -o \ > + -n "$MOZ_MSAN" -o \ > - -n "$MOZ_ASAN"; then > + -n "$MOZ_ASAN"; then It seems to me what you want is to add another condition here to make Windows builds take the MOZ_FRAMEPTR_FLAGS="$MOZ_ENABLE_FRAME_PTR" branch.
Attachment #8820470 - Flags: review?(mh+mozilla) → review-
(In reply to Mike Hommey [:glandium] from comment #33) > That must have changed, because it definitely wasn't doing that in 4.4. > Although, eh_frames might be a bigger increase in size than frame pointers. I'm not really sure, but the bigger worry is always the runtime cost, both in losing a general-purpose register as well as the extra instructions necessary for bookkeeping.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #40) > (In reply to Mike Hommey [:glandium] from comment #33) > > That must have changed, because it definitely wasn't doing that in 4.4. > > Although, eh_frames might be a bigger increase in size than frame pointers. > > I'm not really sure, but the bigger worry is always the runtime cost, both > in losing a general-purpose register as well as the extra instructions > necessary for bookkeeping. Losing a general-purpose register was a big deal on x86 because there are so few. It's not so much of a big deal on architectures with more registers, expecially when you look at how registers are actually used by the compiler (check out http://robert.ocallahan.org/2016/05/data-on-x86-64-register-usage.html)
Comment on attachment 8820470 [details] Bug 1322735 - Enable frame pointers on 32-bit windows builds. https://reviewboard.mozilla.org/r/99968/#review104412
Attachment #8820470 - Flags: review?(mh+mozilla) → review+
Pushed by erahm@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b419aaefae95 Enable frame pointers on 32-bit windows builds. r=glandium https://hg.mozilla.org/integration/autoland/rev/84c729c41230 Remove MOZ_STACKWALKING define. r=glandium https://hg.mozilla.org/integration/autoland/rev/01cfc71ce542 Remove OBSERVE_LATE_WRITES define. r=glandium
Backed out in https://hg.mozilla.org/integration/autoland/rev/90f03368f795 for a leak in every single ASan test suite, https://treeherder.mozilla.org/logviewer.html#?job_id=67846835&repo=autoland, and an xpcshell selftest failure, https://treeherder.mozilla.org/logviewer.html#?job_id=67835494&repo=autoland, which at least so far is restricted to just linux32 debug (both the buildbot build and the taskcluster build).
Definitely reproducible, all signs point to the first patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b3a2725c8e5b5a8a09441d7244f5c040939daa5 I really don't see how this is affecting ASAN/linux32, I'll dig in deeper to see if the compile flags changed for those builds.
Pushed by erahm@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a3e1b6cbc78e Enable frame pointers on 32-bit windows builds. r=glandium https://hg.mozilla.org/integration/autoland/rev/c9afa0e10311 Remove MOZ_STACKWALKING define. r=glandium https://hg.mozilla.org/integration/autoland/rev/20abab774cd1 Remove OBSERVE_LATE_WRITES define. r=glandium
I wonder if it'd be worth sending a notification about this work to dev-platform.
Comment on attachment 8820470 [details] Bug 1322735 - Enable frame pointers on 32-bit windows builds. tl;dr we need these two patches in order to get usable stacks from telemetry features that have already been uplifted to aurora. Approval Request Comment [Feature/Bug causing the regression]: No regression. [User impact if declined]: Crash stacks on win32 in telemetry will be useless. [Is this code covered by automated tests?]: This is a compiler option, so every test exercises it. [Has the fix been verified in Nightly?]: It has landed on nightly, tests are passing. [Needs manual test from QE? If yes, steps to reproduce]: No. [List of other uplifts needed for the feature/fix]: This should go along with bug 1317968 which is already uplifted to Aurora 52. [Is the change risky?]: Low risk. [Why is the change risky/not risky?]: We already disable FPO on nightly/aurora by default. [String changes made/needed]: N/A
Attachment #8820470 - Flags: approval-mozilla-aurora?
Comment on attachment 8820468 [details] Bug 1322735 - Remove MOZ_STACKWALKING define. Approval Request Comment See comment 57.
Attachment #8820468 - Flags: approval-mozilla-aurora?
Note: The 3rd patch is just cleanup, we don't need to uplift it.
Cool! Will this help us with bug 1209131? I can't quite tell, because this bug seems to be about "crash stacks", but also seems to be about ways to capture the stack without crashing.
Flags: needinfo?(erahm)
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #60) > Cool! Will this help us with bug 1209131? I can't quite tell, because this > bug seems to be about "crash stacks", but also seems to be about ways to > capture the stack without crashing. I skimmed that bug and it's dependency's titles but didn't see anything about capturing stacks causing crashes. My understanding is that with FPO enabled on win32 we would get junk stacks so we just didn't bother, but it's possible we would walk off the stack as well which certainly could lead to a crash. Also note that for nightly/aurora FPO was already disabled b/c we have profiling enabled, so I don't think capturing stacks should be blocking bug 1209131 at all.
Flags: needinfo?(erahm)
(In reply to Eric Rahm [:erahm] from comment #61) > (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #60) > > Cool! Will this help us with bug 1209131? I can't quite tell, because this > > bug seems to be about "crash stacks", but also seems to be about ways to > > capture the stack without crashing. > > I skimmed that bug and it's dependency's titles but didn't see anything > about capturing stacks causing crashes. My understanding is that with FPO > enabled on win32 we would get junk stacks so we just didn't bother, but it's > possible we would walk off the stack as well which certainly could lead to a > crash. To be clear, when I said "without crashing" I mean "without intentionally triggering a crash to generate a minidump". > Also note that for nightly/aurora FPO was already disabled b/c we have > profiling enabled, so I don't think capturing stacks should be blocking bug > 1209131 at all. Well, it would assuming we wanted to use that telemetry on beta/release, right?
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #62) > (In reply to Eric Rahm [:erahm] from comment #61) > > (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #60) > > > Cool! Will this help us with bug 1209131? I can't quite tell, because this > > > bug seems to be about "crash stacks", but also seems to be about ways to > > > capture the stack without crashing. > > > > I skimmed that bug and it's dependency's titles but didn't see anything > > about capturing stacks causing crashes. My understanding is that with FPO > > enabled on win32 we would get junk stacks so we just didn't bother, but it's > > possible we would walk off the stack as well which certainly could lead to a > > crash. > > To be clear, when I said "without crashing" I mean "without intentionally > triggering a crash to generate a minidump". > > > Also note that for nightly/aurora FPO was already disabled b/c we have > > profiling enabled, so I don't think capturing stacks should be blocking bug > > 1209131 at all. > > Well, it would assuming we wanted to use that telemetry on beta/release, > right? I see what you're saying now, yes this improvement is not isolated to crashes. It should help with getting useful stack traces across the board on win32. Bug 1209131 seems to be limiting itself to builds that have profiling enabled so that might be an issue as this doesn't change when profiling is enabled. That conversation is probably better discussed in that bug.
Blocks: 1065296
(In reply to Eric Rahm [:erahm] from comment #57) > This should go along with bug 1317968 which is already uplifted to Aurora 52. > That was backed out of aurora as far as I can tell.
(In reply to Julien Cristau [:jcristau] from comment #64) > (In reply to Eric Rahm [:erahm] from comment #57) > > This should go along with bug 1317968 which is already uplifted to Aurora 52. > > > That was backed out of aurora as far as I can tell. It got re-landed, see bug 1317968, comment 20.
Flags: needinfo?(jcristau)
And backed out again in https://hg.mozilla.org/releases/mozilla-aurora/rev/67635f34bbdc
Flags: needinfo?(jcristau) → needinfo?(erahm)
(In reply to Julien Cristau [:jcristau] from comment #66) > And backed out again in > https://hg.mozilla.org/releases/mozilla-aurora/rev/67635f34bbdc I see, it wasn't noted in the bug. I'll check if we still want to try to uplift bug 1317968 again.
Flags: needinfo?(erahm)
Too late for 51. Mark 51 won't fix.
status-firefox51: affected → wontfix
Comment on attachment 8820470 [details] Bug 1322735 - Enable frame pointers on 32-bit windows builds. don't omit frame pointers on win32, beta52+
Attachment #8820470 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
Comment on attachment 8820468 [details] Bug 1322735 - Remove MOZ_STACKWALKING define. drop no longer needed define, beta52+
Attachment #8820468 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
Setting qe-verify- per Comment 57.
Flags: qe-verify-
Blocks: 1340721
Component: Build Config → General
Product: Firefox → Firefox Build System
Target Milestone: Firefox 53 → mozilla53
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: