Closed Bug 1014976 Opened 6 years ago Closed 6 years ago

Enable jemalloc on windows debug builds

Categories

(Core :: Memory Allocator, defect)

All
Windows 7
defect
Not set
normal

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: nobody → mh+mozilla
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.
(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.
Something else worth mentioning: we can't enable DMD with the debug CRT.
> we can't enable DMD with the debug CRT.

Get rid of it! :)
Attachment #8427528 - Attachment is obsolete: true
Attachment #8427528 - Flags: review?(ryanvm)
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)
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)
(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
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)
Fixup to properly ship the CRT on debug builds not using the debug CRT.
Attachment #8427676 - Flags: review?(benjamin)
Attachment #8427533 - Attachment is obsolete: true
Attachment #8427533 - Flags: review?(benjamin)
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 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+
Attachment #8427526 - Flags: review?(mshal) → review+
Attachment #8427530 - Attachment is obsolete: true
Attachment #8427676 - Attachment is obsolete: true
Same patch as before, but with context updated.
Attachment #8429948 - Flags: review?(benjamin)
Attachment #8427534 - Attachment is obsolete: true
Attachment #8427534 - Flags: review?(benjamin)
Attachment #8427537 - Flags: review?(benjamin) → review+
Attachment #8427538 - Flags: review?(benjamin) → review+
Attachment #8429946 - Flags: review?(benjamin) → review+
Attachment #8429948 - Flags: review?(benjamin) → review+
Attachment #8427523 - Flags: review?(ted) → review+
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+
Depends on: 1017926
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.
(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.
You need to log in before you can comment on or make changes to this bug.