Closed
Bug 1014976
Opened 10 years ago
Closed 10 years ago
Enable jemalloc on windows debug builds
Categories
(Core :: Memory Allocator, defect)
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: glandium, Assigned: glandium)
References
Details
Attachments
(8 files, 5 obsolete files)
4.47 KB,
patch
|
ted
:
review+
ted
:
checkin+
|
Details | Diff | Splinter Review |
7.02 KB,
patch
|
mshal
:
review+
|
Details | Diff | Splinter Review |
874 bytes,
patch
|
mshal
:
review+
|
Details | Diff | Splinter Review |
1.49 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
1.04 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
3.73 KB,
patch
|
RyanVM
:
review+
|
Details | Diff | Splinter Review |
9.35 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
2.34 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
See bug 1013014 comment 25 and subsequent. Considering those problems, I'm seriously considering not using the msvc debug crt for our debug builds. This is what was said on irc, related to that: <glandium> now, i wonder, is the debug crt actually buying us anything? <glandium> iow, is it really worth bothering? can't we just not use it? <khuey> it does heap corruption checking <khuey> of course, if you don't use its heap ... <khuey> the debug CRT is also a PITA because it means you can't run our windows debug builds without the matching compiler installed <khuey> because the debug CRT is non-redistributable <njn> glandium: not using the debug crt sounds like a good idea
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → mh+mozilla
Comment 1•10 years ago
|
||
FWIW, we do occasionally hit the CRT heap corruption detector on TBPL. On the other hand, I don't know that it has ever resulted in any useful action.
Assignee | ||
Comment 2•10 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #1) > FWIW, we do occasionally hit the CRT heap corruption detector on TBPL. On > the other hand, I don't know that it has ever resulted in any useful action. But as khuey mentioned, if we don't use the CRT heap (which we wouldn't with jemalloc), the heap corruption detector won't help much. Note that jemalloc does have some additional things enabled in debug builds too, but i don't know how they compare to the msvc crt heap corruption detector.
Assignee | ||
Comment 3•10 years ago
|
||
Something else worth mentioning: we can't enable DMD with the debug CRT.
Comment 4•10 years ago
|
||
> we can't enable DMD with the debug CRT.
Get rid of it! :)
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8427523 -
Flags: review?(ted)
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8427526 -
Flags: review?(mshal)
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8427528 -
Flags: review?(ryanvm)
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8427530 -
Flags: review?(ryanvm)
Assignee | ||
Updated•10 years ago
|
Attachment #8427528 -
Attachment is obsolete: true
Attachment #8427528 -
Flags: review?(ryanvm)
Assignee | ||
Comment 9•10 years ago
|
||
This thing has been useless has soon as it was added to the tree, because it used to be "-MOZ_NO_DEBUG_RTL", not "-DMOZ_NO_DEBUG_RTL": http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/netwerk/streamconv/test&command=DIFF_FRAMESET&file=Makefile.in&rev2=1.12&rev1=1.11 (commit from december 2001) It only recently was promoted as a define in bug 874266 (and I'm guilty of not having seen then that it was useless)
Attachment #8427531 -
Flags: review?(mshal)
Assignee | ||
Comment 10•10 years ago
|
||
Turns out we have had MOZ_NO_DEBUG_RTL almost for ever. It's just not hooked to everything. The --disable-debug-rtl in configure.in is for nspr and requires the nspr patch from this bug to work properly. ICU fails to build because of -D_DEBUG triggering the use of a debug CRT symbol in udbgutil.c. I didn't feel like working that around in ICU. I don't think we have much need for --enable-debug in ICU anyways. Even if we do, it will still be possible to do a build *with* the debug rtl anyways.
Attachment #8427533 -
Flags: review?(benjamin)
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8427534 -
Flags: review?(benjamin)
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8427537 -
Flags: review?(benjamin)
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8427538 -
Flags: review?(benjamin)
Assignee | ||
Comment 14•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=490636f3eeb6
Assignee | ||
Comment 15•10 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #14) > https://tbpl.mozilla.org/?tree=Try&rev=490636f3eeb6 That push failed because I tried to work around the current win64 bustage due to libstagefright. New attempt, win32 only: https://tbpl.mozilla.org/?tree=Try&rev=fc2aec9905bb
Assignee | ||
Comment 16•10 years ago
|
||
Comment on attachment 8427530 [details] [diff] [review] Don't make --enable-debug imply using the debug CRT in FFI This part will need a different implementation. That one overrides too much of the CFLAGS.
Attachment #8427530 -
Flags: review?(ryanvm)
Assignee | ||
Comment 17•10 years ago
|
||
Fixup to properly ship the CRT on debug builds not using the debug CRT.
Attachment #8427676 -
Flags: review?(benjamin)
Assignee | ||
Updated•10 years ago
|
Attachment #8427533 -
Attachment is obsolete: true
Attachment #8427533 -
Flags: review?(benjamin)
Assignee | ||
Comment 18•10 years ago
|
||
New try: https://tbpl.mozilla.org/?tree=Try&rev=ba15384a4c98
Assignee | ||
Comment 19•10 years ago
|
||
Comment on attachment 8427676 [details] [diff] [review] Make MOZ_NO_DEBUG_RTL builds actually disable the MSVC debug CRT everywhere Sigh, there are remaining issues with WIN32_REDIST_DIR because it's mk_add_options'd.
Attachment #8427676 -
Flags: review?(benjamin)
Comment 20•10 years ago
|
||
Comment on attachment 8427531 [details] [diff] [review] Remove MOZ_NO_DEBUG_RTL define in netwerk/streamconv/test Speaking of useless defines, can NGPREFS go away as well? The only use of it I see in the tree is this moz.build file.
Attachment #8427531 -
Flags: review?(mshal) → review+
Updated•10 years ago
|
Attachment #8427526 -
Flags: review?(mshal) → review+
Assignee | ||
Comment 21•10 years ago
|
||
Attachment #8429801 -
Flags: review?(ryanvm)
Assignee | ||
Updated•10 years ago
|
Attachment #8427530 -
Attachment is obsolete: true
Assignee | ||
Comment 22•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=58121609767d
Attachment #8429946 -
Flags: review?(benjamin)
Assignee | ||
Updated•10 years ago
|
Attachment #8427676 -
Attachment is obsolete: true
Assignee | ||
Comment 23•10 years ago
|
||
Same patch as before, but with context updated.
Attachment #8429948 -
Flags: review?(benjamin)
Assignee | ||
Updated•10 years ago
|
Attachment #8427534 -
Attachment is obsolete: true
Attachment #8427534 -
Flags: review?(benjamin)
Updated•10 years ago
|
Attachment #8427537 -
Flags: review?(benjamin) → review+
Updated•10 years ago
|
Attachment #8427538 -
Flags: review?(benjamin) → review+
Updated•10 years ago
|
Attachment #8429946 -
Flags: review?(benjamin) → review+
Updated•10 years ago
|
Attachment #8429948 -
Flags: review?(benjamin) → review+
Updated•10 years ago
|
Attachment #8427523 -
Flags: review?(ted) → review+
Comment 24•10 years ago
|
||
Comment on attachment 8427523 [details] [diff] [review] Allow --enable-debug --disable-debug-rtl NSPR builds http://hg.mozilla.org/projects/nspr/rev/8175a7a0f186
Attachment #8427523 -
Flags: checkin+
Comment 25•10 years ago
|
||
Comment on attachment 8429801 [details] [diff] [review] Don't make --enable-debug imply using the debug CRT in FFI Review of attachment 8429801 [details] [diff] [review]: ----------------------------------------------------------------- Upstream's cool with it, so fire away :) ::: js/src/ctypes/libffi-patches/01-rtl.patch @@ +1,1 @@ > +Don't make --enable-debug imply using the debug CRT in FFI Include the bug number please? :)
Attachment #8429801 -
Flags: review?(ryanvm) → review+
Assignee | ||
Comment 26•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d959285c827e https://hg.mozilla.org/integration/mozilla-inbound/rev/b37443c16b8a https://hg.mozilla.org/integration/mozilla-inbound/rev/9ad1fff3aad7 https://hg.mozilla.org/integration/mozilla-inbound/rev/ca0abd3d08bf https://hg.mozilla.org/integration/mozilla-inbound/rev/c43eac27b959 https://hg.mozilla.org/integration/mozilla-inbound/rev/c01b7a9531a9 https://hg.mozilla.org/integration/mozilla-inbound/rev/0e158358e811
Comment 27•10 years ago
|
||
And a CLOBBER touch for Windows debug bustage. https://hg.mozilla.org/integration/mozilla-inbound/rev/db25dbf0e8d0
Comment 28•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d959285c827e https://hg.mozilla.org/mozilla-central/rev/b37443c16b8a https://hg.mozilla.org/mozilla-central/rev/9ad1fff3aad7 https://hg.mozilla.org/mozilla-central/rev/ca0abd3d08bf https://hg.mozilla.org/mozilla-central/rev/c43eac27b959 https://hg.mozilla.org/mozilla-central/rev/c01b7a9531a9 https://hg.mozilla.org/mozilla-central/rev/0e158358e811 https://hg.mozilla.org/mozilla-central/rev/db25dbf0e8d0
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Comment 29•10 years ago
|
||
As of 185771, my builds (and clobber-builds) are failing, starting with this error: 3:23.22 e:\dev\mozilla\central\memory\jemalloc\src\include\msvc_compat\stdbool.h(8) : error C2628: 'BOOL' followed by 'bool' is illegal (did you forget a ';'?) This is with mozbuild 1.9.0 and msvc2013.
Assignee | ||
Comment 30•10 years ago
|
||
(In reply to Jeff Gilbert [:jgilbert] from comment #29) > As of 185771, my builds (and clobber-builds) are failing, starting with this > error: > 3:23.22 > e:\dev\mozilla\central\memory\jemalloc\src\include\msvc_compat\stdbool.h(8) > : error C2628: 'BOOL' followed by 'bool' is illegal (did you forget a ';'?) > > This is with mozbuild 1.9.0 and msvc2013. That's bug 945582.
Comment 31•10 years ago
|
||
(In reply to Carsten Book [:Tomcat] from comment #28) > https://hg.mozilla.org/mozilla-central/rev/b37443c16b8a Merged upstream: https://github.com/atgreen/libffi/commit/df31a85103b0cc232bbc340d7e782a3853c7fad5
You need to log in
before you can comment on or make changes to this bug.
Description
•