Open Bug 949700 Opened 6 years ago Updated 2 years ago

Disable stack walking for NS_ASSERTION in mochitests

Categories

(Testing :: Mochitest, defect)

x86_64
Windows 7
defect
Not set

Tracking

(Not tracked)

People

(Reporter: dmajor, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

In debug mochitests I see a fairly large amount of address space being reserved by dbghelp, the Windows symbol engine. It's called by, among other things, the stack walk for assertions in NS_DebugBreak. 

Disabling the stack output saves a consistent 60-100MB in both vsize and contiguous on Win7 BC. I was so surprised that I ran it through several times to confirm. It may not seem like much but it makes a big difference when we get low at the end. http://people.mozilla.org/~sguo/mochimem/viewer.html?url=http%3A//ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-win32-debug/1386851350/mozilla-central_win7-ix-debug_test-mochitest-browser-chrome-bm69-tests1-windows-build223.txt.gz&url=http%3A//ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dmajor@mozilla.com-73b928559b85/try-win32-debug/try_win7-ix-debug_test-mochitest-browser-chrome-bm70-tests1-windows-build471.txt.gz&
Some things to note:
* This does not turn off the assert text. The message, expression, file, and line are still there.
* The stacks aren't working in linux anyway: search "###!!! ASSERTION:" in https://tbpl.mozilla.org/php/getParsedLog.php?id=31875485&tree=Mozilla-Central&full=1
* If someone still wants to debug after this change, it's easy to push a try job with this reverted

I don't know whether people's workflows are relying on the stacks, so one option might be to keep this around as a back-pocket patch for the next time we reach OOM levels. I suppose another option is to fix the assertions!
Attachment #8346844 - Attachment description: trywarnruntestpy → Disable stack walking for NS_ASSERTION in mochitests
Component: General → Mochitest
Product: Core → Testing
Filed bug 949715 for the lack of symbols on linux
I think this is probably going to break people being able to fix assertions when they occur for test suites like Mochitest where they're not fatal. (When they're fatal we just kill the process and get a stack out of the minidump.)
I didn't think this through well enough. I was only thinking about this from the TBPL angle and didn't consider that a lot of debugging is done on a dev machine before a push ever happens. This patch probably shouldn't be checked in.
(In reply to David Major [:dmajor] from comment #4)
> I didn't think this through well enough. I was only thinking about this from
> the TBPL angle and didn't consider that a lot of debugging is done on a dev
> machine before a push ever happens. This patch probably shouldn't be checked
> in.

We have the ability to tweak things on a developer-vs-tbpl basis; could we make the assertions output stacks on development builds and not output stacks on tbpl?
I'm not sure this would be acceptable on TBPL either--that would mean that someone whose patch caused a new assertion which caused a Mochitest to fail would not get a stack for that assertion, making it a lot harder to figure out what happened.

This does suck though, maybe we could reuse the Breakpad unwinder we build for the profiler to get stacks instead of using DbgHelp? (I'm not sure Breakpad would do a lot better in terms of vsize though, unfortunately, debug symbols are big.)
Removing the MemShrink tag since it doesn't affect production builds.
Whiteboard: [MemShrink]
Being ignorant of how things work on Windows, is it possibly to lazily load dbghelp.dll for when we actually need it?  Or do we assert often enough that it would wind up loaded anyway?
Flags: needinfo?(dmajor)
(In reply to Nathan Froyd (:froydnj) from comment #8)
> Being ignorant of how things work on Windows, is it possibly to lazily load
> dbghelp.dll for when we actually need it?  Or do we assert often enough that
> it would wind up loaded anyway?

That stuff is already lazy init. But if we assert at all (which we do) then the init eventually happens.
Flags: needinfo?(dmajor)
You need to log in before you can comment on or make changes to this bug.