Closed
Bug 669953
Opened 13 years ago
Closed 13 years ago
build debug builds with optimization
Categories
(Release Engineering :: General, defect, P3)
Release Engineering
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: ted, Assigned: espindola)
References
Details
(Whiteboard: [buildfaster:p1])
Attachments
(2 files, 4 obsolete files)
1.40 KB,
patch
|
khuey
:
review+
espindola
:
checked-in+
|
Details | Diff | Splinter Review |
3.86 KB,
patch
|
ted
:
review+
espindola
:
checked-in+
|
Details | Diff | Splinter Review |
I was sure our debug builds were already optimized, but looking at the mozconfigs shows that I'm wrong. We explicitly --disable-optimize in their mozconfigs. Because they're not optimized, test runs are much slower than they are on opt builds. If we're worried about getting good assertion stacktraces, we could use --enable-profiling to ensure that we have frame pointers. Are there any other reasons to want these builds to be unoptimized?
Updated•13 years ago
|
Whiteboard: [buildfaster]
Well, we'd still loose a good bit of info in the stack traces due to inlining -- which I suspect is also where the bulk of the performance benefit comes from.
Reporter | ||
Comment 2•13 years ago
|
||
Seems like it's worth trying to see what the perf benefit is, and we can also gauge if the stack traces are a problem.
fwiw, I want to flip the default when you're doing a debug build to --disable-optimize, so you're going to want to explicitly specify --enable-optimize here.
Updated•13 years ago
|
Assignee: nobody → coop
Priority: -- → P3
Comment 4•13 years ago
|
||
I did a try build with "ac_add_options --enable-optimize" in mozconfig-extra, and got a bunch of failures: http://tinderbox.mozilla.org/showlog.cgi?log=Try/1311184952.1311187257.21307.gz&fulltext=1 http://tinderbox.mozilla.org/showlog.cgi?log=Try/1311188440.1311188932.29472.gz&fulltext=1 http://tinderbox.mozilla.org/showlog.cgi?log=Try/1311190801.1311190981.7116.gz&fulltext=1 the resulting build looks pretty crashy.
Updated•13 years ago
|
Assignee: coop → ehsan
Comment 6•13 years ago
|
||
(In reply to comment #4) > the resulting build looks pretty crashy. That _might_ be because you didn't have frame pointers enabled. Anyways, I'll have more info on this shortly.
Comment 7•13 years ago
|
||
Try normal debug builds: http://tbpl.mozilla.org/?tree=Try&rev=23957115624b Try "fast" debug builds: http://tbpl.mozilla.org/?tree=Try&rev=2dd95b3f917c Good news is that builds are less than 10% slower and tests are around 50% faster! Bad news is that Windows builds break, and Linux 32 bit debugs all crash in JSAssert. I was not able to reproduce either of the issues locally, but I should also say that I wasn't using our builder compilers (VC8 and gcc4.5). Maybe someone with the right tools can step in and look into this?
Assignee: ehsan → nobody
Reporter | ||
Comment 8•13 years ago
|
||
The Linux builds are crashing in unittests, so it should be feasible for developers to download the builds and run them locally to try to reproduce the crash.
Reporter | ||
Comment 9•13 years ago
|
||
Also, the Windows hang looks like it's hanging running xpcshell during "make package" (we run xpcshell to generate startupcache).
Assignee | ||
Comment 10•13 years ago
|
||
I reduced the linux failure to gcc removing "address < 46" in extern void JS_Assert(); typedef enum { eax, ecx, edx, ebx, esp, ebp, esi, edi } RegisterID; union StateRemat { RegisterID reg_; int offset_; }; static StateRemat FromRegister(RegisterID reg) { StateRemat sr; sr.reg_ = reg; return sr; } static StateRemat FromAddress3(int address) { StateRemat sr; sr.offset_ = address; //sr.offset_ = 0; if (address < 46 && address >= 0) { JS_Assert(); } return sr; } struct FrameState { StateRemat dataRematInfo2(bool y, int z) { if (y) return FromRegister(RegisterID(1)); return FromAddress3(z); } }; FrameState frame; StateRemat x; void jsop_setelem(bool y, int z) { x = frame.dataRematInfo2(y, z); } which I am almost sure is a gcc bug. I will report it
Assignee | ||
Comment 11•13 years ago
|
||
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49911
Reporter | ||
Comment 12•13 years ago
|
||
Rafael: is there a way we can work around that compiler bug?
Assignee | ||
Comment 13•13 years ago
|
||
It seems hard to work around. I will try to add the patch the Richi mentions on the bug to our spec file and report if that works.
Assignee | ||
Comment 14•13 years ago
|
||
Assignee | ||
Comment 15•13 years ago
|
||
I tried to reproduce the failure on windows, but it failed. My setup was vc 2005 on windows 7. I did a new push to try to see if the problem still exists: http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=9c8e5599d475
Updated•13 years ago
|
Whiteboard: [buildfaster:p2] → [buildfaster:p1]
Reporter | ||
Updated•13 years ago
|
Assignee: nobody → respindola
Assignee | ||
Comment 16•13 years ago
|
||
That was just an upload fail. I am still going to debug what the ### ERROR: WalkStack64: Only part of a ReadProcessMemory or WriteProcessMemory request was completed. was, but there is a new try run in: http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=644d28700788
Comment 17•13 years ago
|
||
(In reply to Rafael Ávila de Espíndola (:espindola) from comment #16) > That was just an upload fail. I am still going to debug what the > > ### ERROR: WalkStack64: Only part of a ReadProcessMemory or > WriteProcessMemory request was completed. > > was, but there is a new try run in: > > http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=644d28700788 We figured this out. This is just the Windows stack walking API failing to give us full stack information because of the lack of frame pointers. I filed bug 680277 to fix this issue.
Assignee | ||
Comment 18•13 years ago
|
||
Our configure script now disables vrp if it finds a broken gcc (like the ones in the bots) and it should be possible to work around frame pointers being disabled by adding --enable-profiling. There is a try push in: http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=e6b1e04eda75
Assignee | ||
Comment 19•13 years ago
|
||
Attachment #550489 -
Attachment is obsolete: true
Attachment #556819 -
Flags: review?(catlee)
Attachment #556819 -
Flags: feedback?(ehsan)
Reporter | ||
Comment 20•13 years ago
|
||
So did the Win32 hang turn out to be intermittent or something? Your try runs in comment 16 and comment 18 look good.
Assignee | ||
Comment 21•13 years ago
|
||
(In reply to Ted Mielczarek [:ted, :luser] from comment #20) > So did the Win32 hang turn out to be intermittent or something? Your try > runs in comment 16 and comment 18 look good. It was caused by some libraries having frame pointers and other don't. Bug 680277 tracks the real fix. This patch avoids it by also using --enable-profiling.
Updated•13 years ago
|
Attachment #556819 -
Flags: review?(catlee) → review+
Comment 22•13 years ago
|
||
Attachment 556819 [details] [diff] landed at http://hg.mozilla.org/build/buildbot-configs/rev/ff5fc1e933d0 and went into production at 'Thu Sep 01 14:05:36 2011 -0700' with http://hg.mozilla.org/build/buildbot-configs/rev/b44dceb10136 Subsequently backed out for bustage on default http://hg.mozilla.org/build/buildbot-configs/rev/af7327dcc81a and merged to production at 'Thu Sep 01 21:25:43 2011 -0700' with http://hg.mozilla.org/build/buildbot-configs/rev/9e669f6c07ec
Updated•13 years ago
|
Attachment #556819 -
Flags: feedback-
Comment 23•13 years ago
|
||
Comment on attachment 556819 [details] [diff] [review] Enable optimizations Oops, wrong flag.
Attachment #556819 -
Flags: feedback- → checked-in-
Comment 24•13 years ago
|
||
what caused this to bounce?
Comment 25•13 years ago
|
||
Linux32 debug crashing during make package - espindola and bz sounded like they were getting a handle on why last night.
Assignee | ||
Comment 26•13 years ago
|
||
The linux 32 bit problem was bug 684526.
Comment 27•13 years ago
|
||
Comment on attachment 556819 [details] [diff] [review] Enable optimizations I thought I had plused this...
Attachment #556819 -
Flags: feedback?(ehsan) → feedback+
Assignee | ||
Comment 28•13 years ago
|
||
New try at https://tbpl.mozilla.org/?tree=Try&rev=24392396a930
Assignee | ||
Comment 29•13 years ago
|
||
Attachment #556819 -
Attachment is obsolete: true
Attachment #561770 -
Flags: review?(catlee)
Assignee | ||
Comment 30•13 years ago
|
||
I was able to reproduce the 10.5 make check failure. I am debugging it.
Assignee | ||
Comment 31•13 years ago
|
||
I new try push on top of the fix for bug 689269 is at https://tbpl.mozilla.org/?tree=Try&rev=b7125de99fa8
Assignee | ||
Comment 32•13 years ago
|
||
After finding: * a bug in gcc * a strict aliasing violation on js * a compartment gc + ti bug * a build bug that caused js and the rest to have different flags * and a build bug that caused us to create debug builds without frame pointers it looks like this is finally working :-) Is the patch OK?
Updated•13 years ago
|
Attachment #561770 -
Flags: review?(catlee) → review+
Comment 33•13 years ago
|
||
This is now live.
Comment 35•13 years ago
|
||
Reopening to enable this on try and m-i too....
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 36•13 years ago
|
||
Attachment #563500 -
Flags: review?(aki)
Updated•13 years ago
|
Attachment #563500 -
Flags: review?(aki) → review+
Comment 38•13 years ago
|
||
This is now also live for try and m-i!
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
This caused WinXP debug reftest to go orange with: TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 256 bytes during test execution TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of Mutex with size 12 bytes TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of ReentrantMonitor with size 16 bytes TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsTArray_base with size 4 bytes TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsThread with size 128 bytes TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsTimerImpl with size 72 bytes on both mozilla-central and mozilla-inbound.
Comment 40•13 years ago
|
||
Backed out. http://hg.mozilla.org/build/buildbot-configs/rev/b275f12b6356 934 hg pull -u 935 hg up -r default 936 hg export 4822 | patch -R -p1 937 hg export 4816 | patch -R -p1 938 hg commit 939 hg up -r production 940 hg merge -r default 941 hg commit 942 hg push
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 41•13 years ago
|
||
Also: Incidentally, this ended up triggering this odd command, with a confused optimize on/off state, in Win Debug build logs: === running /bin/sh /e/builds/moz2_slave/m-cen-w32-dbg/build/nsprpub/configure --enable-application=browser --enable-jemalloc --enable-debug --enable-libxul --enable-trace-malloc --enable-tests --with-dist-prefix=e:/builds/moz2_slave/m-cen-w32-dbg/build/obj-firefox/dist --with-mozilla --enable-optimize --enable-debug --disable-optimize --cache-file=.././config.cache --srcdir=/e/builds/moz2_slave/m-cen-w32-dbg/build/nsprpub === (note the "--enable-optimize --enable-debug --disable-optimize" there) https://tbpl.mozilla.org/php/getParsedLog.php?id=6615313&tree=Firefox WINNT 5.2 mozilla-central leak test build on 2011-09-29 12:38:54 PDT for push 60e86b847759 This may merit a separate bug, but I thought I'd point it out in case it's worth fixing before this re-lands.
Assignee | ||
Comment 42•13 years ago
|
||
Was the leak permanent? A bugzilla search suggests: https://bugzilla.mozilla.org/show_bug.cgi?id=687857 I am out tomorrow, but if no one gets to it first, on Monday I will try to debug both the nsprpub/configure invocation and the leak.
Comment 43•13 years ago
|
||
Yes, permanent, and 256 bytes in reftest, while 687857 is 2-4 megabytes in mochitest-chrome.
Assignee | ||
Comment 44•13 years ago
|
||
I was able to reproduce the bug and I am trying to reduce the testcase a bit. While at it, I have a try run to try at least to fix the "--enable-optimize --enable-debug --disable-optimize" silliness. https://tbpl.mozilla.org/?tree=Try&rev=a88fa132ddaf
Assignee | ||
Comment 45•13 years ago
|
||
This patch makes us always pass --(enable|disable)-(optimizations|debug) to the nspr configure. That way we don't have to worry about defaults in two configures being in sync. It also removes the hack for trace malloc from the top level configure. If there are nspr only restrictions on tracing and optimizations, the nspr configure should handle them.
Attachment #564300 -
Flags: review?(khuey)
Assignee | ||
Comment 46•13 years ago
|
||
One interesting info, the leak only happens if NS_TRACE_MALLOC_DISABLE_STACKS is set.
(In reply to Rafael Ávila de Espíndola (:espindola) from comment #45) > It also removes the hack for trace malloc from the top level configure. If > there are nspr only restrictions on tracing and optimizations, the nspr > configure should handle them. Did you look at cvs/hg blame to see why it was added? (In reply to Rafael Ávila de Espíndola (:espindola) from comment #46) > One interesting info, the leak only happens if > NS_TRACE_MALLOC_DISABLE_STACKS is set. That seems most likely to be due to a change in performance characteristics.
Assignee | ||
Comment 48•13 years ago
|
||
Adding some logging shows that it is the mMainThread in the thread manager that is leaking.
Assignee | ||
Comment 49•13 years ago
|
||
A new try run that includes the fix te 692122 is at https://tbpl.mozilla.org/?tree=Try&rev=0d0283093e31
Comment on attachment 564300 [details] [diff] [review] don't pass "--enable-optimize --enable-debug --disable-optimize" to sub configure Sorry for the delay, I've been away from the computer for a few days.
Attachment #564300 -
Flags: review?(khuey) → review+
Comment 51•13 years ago
|
||
(In reply to Rafael Ávila de Espíndola (:espindola) from comment #49) > A new try run that includes the fix te 692122 is at > > https://tbpl.mozilla.org/?tree=Try&rev=0d0283093e31 There's a mochitest-chrome perma-leak on Windows XP debug with this patch. Also, all of the Android debug tests are orange too. Maybe somebody from the mobile team can tell us whether that's normal or not?
Comment 52•13 years ago
|
||
I think joel is tracking an issue with debug tests on Android, can't recall what bug though
Comment 53•13 years ago
|
||
Yes, debug tests on Android have been perma-orange for a while: bug 691071
Comment 54•13 years ago
|
||
That is not at all the problem with those runs. The problem with those runs is that some mobile meeting told aki that it would be useful to run debug tests even without having the assertions make it into the logs, which was, to put the best possible face on it, something which turns out not to be entirely the truth. That is 26 utterly wasted test runs which (absent a few infra failures) are orange because they don't match the expected assertion counts, doing things like having 8 assertions in min-width-1b.html. What assertions? Who knows? Annotate them? As what? File a bug on the assertions? Which assertions?
Assignee | ||
Comment 55•13 years ago
|
||
Just the configure changes are in https://hg.mozilla.org/integration/mozilla-inbound/rev/17eecce51c43
Assignee | ||
Updated•13 years ago
|
Attachment #564300 -
Flags: checked-in+
Assignee | ||
Comment 56•13 years ago
|
||
And a new try on top of the previous fixes is at https://tbpl.mozilla.org/?tree=Try&rev=f696664da2c0 I will debug the leak if it is still there on Tuesday.
Assignee | ||
Comment 57•13 years ago
|
||
(In reply to David Baron [:dbaron] from comment #47) > (In reply to Rafael Ávila de Espíndola (:espindola) from comment #45) > > It also removes the hack for trace malloc from the top level configure. If > > there are nspr only restrictions on tracing and optimizations, the nspr > > configure should handle them. > > Did you look at cvs/hg blame to see why it was added? That was https://github.com/ehsan/mozilla-history/commit/785dd8c8b40e7676ec0acfbffc964aee8177dbcd https://bugzilla.mozilla.org/show_bug.cgi?id=126915
Assignee | ||
Comment 58•13 years ago
|
||
Attachment #561770 -
Attachment is obsolete: true
Attachment #563500 -
Attachment is obsolete: true
Attachment #566113 -
Flags: review?(ted.mielczarek)
Reporter | ||
Comment 60•13 years ago
|
||
Comment on attachment 566113 [details] [diff] [review] Enable optimizations Review of attachment 566113 [details] [diff] [review]: ----------------------------------------------------------------- Go for it!
Attachment #566113 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Comment 61•13 years ago
|
||
Comment on attachment 566113 [details] [diff] [review] Enable optimizations https://hg.mozilla.org/integration/mozilla-inbound/rev/52103e8ae17e
Attachment #566113 -
Flags: checked-in+
Comment 62•13 years ago
|
||
(In reply to Rafael Ávila de Espíndola (:espindola) from comment #61) > Enable optimizations https://hg.mozilla.org/mozilla-central/rev/52103e8ae17e
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Product: mozilla.org → Release Engineering
You need to log in
before you can comment on or make changes to this bug.
Description
•