Currently for Win32 we build a custom CRT and patch it to hook jemalloc in. This is a pain. It requires a Pro version of Visual C++ to build with jemalloc, the patch is extremely version specific (down to the service pack), and the build hackery is quite awful. For WinCE builds, we have a much nicer solution that involves overloading all the memory allocating symbols in a separate jemalloc.dll that gets loaded before the CRT. We should switch to this approach for Win32 builds as well. Aside: getting breakpad symbols for CRT stuff isn't so bad anymore since I fixed bug 419882. It should happen automatically.

Note that on CE, we have mozce_shunt.dll which is where jemalloc goes into and that everything links to; we don't have any such thing on win32, so we'd have to create jemalloc.dll and make sure everything links to it.

I believe that's basically what we do on Linux anyway (libjemalloc.so), so it's probably just a little bit of Makefile hacking to make that work.

we'll have to start shipping the vcredist stuff again if we do this. we'll also waste some space creating the default windows heap that we otherwise avoid here.

I was pointed to this earlier: http://groups.google.com/group/google-perftools/browse_thread/thread/41cd3710af85e57b It's probably just interesting, and not really an appropriate solution here.

What goes wrong if only our infallible wrappers use jemalloc?

The CRT changed pretty drastically with VS2010 -- we'd have to either recreate the jemalloc patch from first principles, or do something like this. It's probably worth investigating this at least so that we don't have to go through this every time MS revs the compiler/crt.

(In reply to comment #5) > What goes wrong if only our infallible wrappers use jemalloc? "Wrong" is somewhat open ended, but we could have mismatched malloc/free with system libs etc., and there would be two heaps (potentially wasting memory). Although the android team implemented that, BTW.

Well, I have a custom build which tags all allocations that go through a) operator new b) PL_strdup c) PR_Malloc d) NS_Alloc and their respective deallocators then abort if the tag mismatches (and the system free also aborts since it won't understand a tagged allocation) and it very mostly runs OK. Wouldn't this jemalloc usage also help OSX, where my understanding is that system malloc can't be overridden any other way?

bug 563316 comment 2 notes that in Visual C++ 2010, Microsoft has removed the nmakefiles for building the CRT, and no longer supports building it. (The source is still shipped with the compiler, though.) Given that, this looks like the only viable way to use jemalloc with VC10.

(In reply to comment #3) > we'll have to start shipping the vcredist stuff again if we do this. we'll > also waste some space creating the default windows heap that we otherwise avoid > here. Or we could use the static CRT right? As long as we're careful about stuff that moves between different shared libraries.

Right now that's probably not a good idea, since we'd wind up shipping multiple copies of it (in NSPR, NSS, libjs, libxul, etc). It'd be a net loss. Maybe if we start shipping a single library (bug 561842) we can do that.

(In reply to comment #10) > Or we could use the static CRT right? As long as we're careful about stuff that > moves between different shared libraries. I don't know if this matters, but there's a limit of ~128 copies of the CRT (static or dynamic) that can exist in a process at once, due to the number of fiber local storage slots available per process. See songbird bug 17788 if you want to see us actually hitting this ;) (The fix there was to move everything to a shared mozcrt, hence my comment here.)

Adding jasone, who has been looking at some related things.

I have a fix in hand here. Just beating it into a reviewable form now.

It has a few rough edges, but it's suitable for review I think.

Comment on attachment 539006 [details] [diff] [review] Patch >+CRTDLL_FULLPATH=f:\\dd\\vctools\\crt_bld\\SELF_X86\\crt\\src\\build\\INTEL\\dll_obj\\crtdll.obj "CRTDLL_FULLPATH=f:\dd\vctools\crt_bld\SELF_64_amd64\crt\src\build\amd64\dll_obj\crtdll.obj" for x64.

Comment on attachment 539006 [details] [diff] [review] Patch Review of attachment 539006 [details] [diff] [review]: ----------------------------------------------------------------- Overall this looks less crappy than our existing jemalloc setup, so I'd say that's a win. I need to look over a few more bits of it, but I don't have time right now. I'll try to check it out again in the next two days. ::: configure.in @@ +7435,5 @@ > *-mingw*) > AC_DEFINE(MOZ_MEMORY_WINDOWS) > dnl This is sort of awful. Will revisit if we add support for more versions > + if test "$CC_VERSION" == "14.00.50727.762" -o "$CC_VERSION" == "15.00.30729.01"; then > + WIN32_OLD_STYLE_JEMALLOC=1 Any reason not to just switch to new-style jemalloc everywhere and rip out the old code? (Is it just that you haven't tested it on 2005/2008?) If we can use the new style everywhere, we can enable jemalloc by default, which would be really nice. @@ +7440,5 @@ > + elif test "$CC_VERSION" == "16.00.30319.01"; then > + WIN32_NEW_STYLE_JEMALLOC=1 > + AC_DEFINE(WIN32_NEW_STYLE_JEMALLOC) > + else > + AC_MSG_ERROR([Building jemalloc requires exactly Visual C++ 2005 SP1 or 2008 SP1 currently.]) This error message is no longer correct. ::: memory/jemalloc/Makefile.in @@ +166,5 @@ > + > +ifeq (WINNT,$(OS_TARGET)) > +ifndef WIN32_OLD_STYLE_JEMALLOC > + > +# Roll our own custom logic here for the import library I uh, need some more time to look at this. ::: memory/jemalloc/jemalloc.c @@ +205,5 @@ > #include <string.h> > > #ifdef MOZ_MEMORY_WINDOWS > > +#ifdef OLD_STYLE_JEMALLOC These changes need review from someone else. Preferably jasone, if he has time. ::: memory/jemalloc/sed.py @@ +1,1 @@ > +# ***** BEGIN LICENSE BLOCK ***** Probably call this something more specific, since it's not really sed. fix_crtdll.py or something. @@ +36,5 @@ > +# ***** END LICENSE BLOCK ***** > + > +# A simple sed utility that supports the non-standard options I need. > + > +file = open('crtdll.obj', 'rb') with open(...) as file: ... @@ +40,5 @@ > +file = open('crtdll.obj', 'rb') > +data = file.read() > +file.close() > +file = open('crtdll_fixed.obj', 'wb') > +data = data.replace(b'__imp__free', b'__imp__frex') I think byte strings are only in Python 2.6, but this is only going to be run on Windows, where we might already require that...

(In reply to comment #17) > Comment on attachment 539006 [details] [diff] [review] [review] > Patch > > Review of attachment 539006 [details] [diff] [review] [review]: > ----------------------------------------------------------------- > > Overall this looks less crappy than our existing jemalloc setup, so I'd say > that's a win. I need to look over a few more bits of it, but I don't have > time right now. I'll try to check it out again in the next two days. > > ::: configure.in > @@ +7435,5 @@ > > *-mingw*) > > AC_DEFINE(MOZ_MEMORY_WINDOWS) > > dnl This is sort of awful. Will revisit if we add support for more versions > > + if test "$CC_VERSION" == "14.00.50727.762" -o "$CC_VERSION" == "15.00.30729.01"; then > > + WIN32_OLD_STYLE_JEMALLOC=1 > > Any reason not to just switch to new-style jemalloc everywhere and rip out > the old code? (Is it just that you haven't tested it on 2005/2008?) If we > can use the new style everywhere, we can enable jemalloc by default, which > would be really nice. Lack of testing. > @@ +7440,5 @@ > > + elif test "$CC_VERSION" == "16.00.30319.01"; then > > + WIN32_NEW_STYLE_JEMALLOC=1 > > + AC_DEFINE(WIN32_NEW_STYLE_JEMALLOC) > > + else > > + AC_MSG_ERROR([Building jemalloc requires exactly Visual C++ 2005 SP1 or 2008 SP1 currently.]) > > This error message is no longer correct. Indeed. > ::: memory/jemalloc/Makefile.in > @@ +166,5 @@ > > + > > +ifeq (WINNT,$(OS_TARGET)) > > +ifndef WIN32_OLD_STYLE_JEMALLOC > > + > > +# Roll our own custom logic here for the import library > > I uh, need some more time to look at this. ;-) > ::: memory/jemalloc/jemalloc.c > @@ +205,5 @@ > > #include <string.h> > > > > #ifdef MOZ_MEMORY_WINDOWS > > > > +#ifdef OLD_STYLE_JEMALLOC > > These changes need review from someone else. Preferably jasone, if he has > time. Hmm, that might be fun. I think he works for Facebook these days, not sure if I could drag a review out of him. I wonder if pbiggar can count as a peer these days. > ::: memory/jemalloc/sed.py > @@ +1,1 @@ > > +# ***** BEGIN LICENSE BLOCK ***** > > Probably call this something more specific, since it's not really sed. > fix_crtdll.py or something. Sure. > @@ +36,5 @@ > > +# ***** END LICENSE BLOCK ***** > > + > > +# A simple sed utility that supports the non-standard options I need. > > + > > +file = open('crtdll.obj', 'rb') > > with open(...) as file: > ... Python and its shiny Python-isms :-P > @@ +40,5 @@ > > +file = open('crtdll.obj', 'rb') > > +data = file.read() > > +file.close() > > +file = open('crtdll_fixed.obj', 'wb') > > +data = data.replace(b'__imp__free', b'__imp__frex') > > I think byte strings are only in Python 2.6, but this is only going to be > run on Windows, where we might already require that... Hmm, not sure that we do.

Comment on attachment 539006 [details] [diff] [review] Patch pbiggar, can you look at the jemalloc.c changes here? Those are relatively straightforward, we add macro defines for a few things that are only defined inside the CRT, add a DllMain that initializes the heap, a wcsdup impl that libxul needs, and a dummy function we can use in place of free to fix the mismatched free at shutdown that the CRT burdens us with.

Comment on attachment 539006 [details] [diff] [review] Patch Review of attachment 539006 [details] [diff] [review]: ----------------------------------------------------------------- I'm happy to r+ this, though I don't understand the windows idioms. It looks right, but it needs many more comments. Am leaving it as r? for now til I see the comments. ::: js/src/jsregexpinlines.h @@ +134,1 @@ > } You can probably omit "JSC::Yarr::BytecodePattern" from this. C++ templates have type-inference. Why is this change in this patch? ::: memory/jemalloc/Makefile.in @@ +166,5 @@ > + > +ifeq (WINNT,$(OS_TARGET)) > +ifndef WIN32_OLD_STYLE_JEMALLOC > + > +# Roll our own custom logic here for the import library I'm leaving this to ted (my r+ doesn't cover this). ::: memory/jemalloc/jemalloc.c @@ +205,5 @@ > #include <string.h> > > #ifdef MOZ_MEMORY_WINDOWS > > +#ifdef OLD_STYLE_JEMALLOC Is this an ifdef for easy backout-ability, or to keep the old functionality? We need the former, but I'd be happy to see it die (file a bug, add a FIXME with the bug reference and a nice) after a release cycle or so. @@ +210,5 @@ > #include <cruntime.h> > #include <internal.h> > +#else > +#define _CRT_SPINCOUNT 5000 > +#define __crtInitCritSecAndSpinCount InitializeCriticalSectionAndSpinCount Needs a comment. @@ +6465,5 @@ > * visibility be used for interposition where available? > */ > # error "Interposing malloc is unsafe on this system without libc malloc hooks." > #endif > + Needs a large and detailed comment. @@ +6474,5 @@ > +{ > + switch (reason) { > + case DLL_PROCESS_ATTACH: > + malloc_init_hard(); > + break; This looks like what you'd do, but I really don't windows programming at all. I'm happy to r+ it, since there's no way bugs will get very far here. Try to land early in the release cycle though - this definitely shoudn't go through just before an Aurora branch. @@ +6497,5 @@ > + return; > +} > + > +#include <wchar.h> > + weird spacing. ::: memory/jemalloc/jemalloc.def @@ +33,5 @@ > +; > +; ***** END LICENSE BLOCK ***** > + > +LIBRARY jemalloc.dll > + Needs a comment (perhaps a redirection to a larger comment in jemalloc.c)

Comment on attachment 539006 [details] [diff] [review] Patch Updated patch later today, complete with a giant comment explaining the story here.

Comment on attachment 539653 [details] [diff] [review] Patch (In reply to comment #20) > Comment on attachment 539006 [details] [diff] [review] [review] > Patch > > Review of attachment 539006 [details] [diff] [review] [review]: > ----------------------------------------------------------------- > > I'm happy to r+ this, though I don't understand the windows idioms. It looks > right, but it needs many more comments. Am leaving it as r? for now til I > see the comments. > > ::: js/src/jsregexpinlines.h > @@ +134,1 @@ > > } > > You can probably omit "JSC::Yarr::BytecodePattern" from this. C++ templates > have type-inference. > > Why is this change in this patch? It was triggering a mismatched new/free problem, but it's since been fixed. > ::: memory/jemalloc/jemalloc.c > @@ +205,5 @@ > > #include <string.h> > > > > #ifdef MOZ_MEMORY_WINDOWS > > > > +#ifdef OLD_STYLE_JEMALLOC > > Is this an ifdef for easy backout-ability, or to keep the old functionality? > We need the former, but I'd be happy to see it die (file a bug, add a FIXME > with the bug reference and a nice) after a release cycle or so. Yeah, we're going to kill the old style eventually. > @@ +210,5 @@ > > #include <cruntime.h> > > #include <internal.h> > > +#else > > +#define _CRT_SPINCOUNT 5000 > > +#define __crtInitCritSecAndSpinCount InitializeCriticalSectionAndSpinCount > > Needs a comment. Done. > @@ +6465,5 @@ > > * visibility be used for interposition where available? > > */ > > # error "Interposing malloc is unsafe on this system without libc malloc hooks." > > #endif > > + > > Needs a large and detailed comment. Done. Perhaps a bit too much. > @@ +6474,5 @@ > > +{ > > + switch (reason) { > > + case DLL_PROCESS_ATTACH: > > + malloc_init_hard(); > > + break; > > This looks like what you'd do, but I really don't windows programming at > all. I'm happy to r+ it, since there's no way bugs will get very far here. > Try to land early in the release cycle though - this definitely shoudn't go > through just before an Aurora branch. Yeah, plan at the moment is to switch to this right after the next Aurora branch.

Comment on attachment 539653 [details] [diff] [review] Patch Review of attachment 539653 [details] [diff] [review]: ----------------------------------------------------------------- A few more comments and clarifications needed. Nearly there :) ::: memory/jemalloc/Makefile.in @@ +171,5 @@ > + > +############################################################################### > +# > +# In the elder days, Microsoft smiled upon their developers and provided them > +# the source and makefiles of the CRT to do with as they pleased. And in those While I really like this sort of fun commenting, you may want to reconsider whether this is the most clear way to comment this. More below. @@ +185,5 @@ > +# had removed the makefiles from the CRT source. And then Mozilla developers > +# said, 'Let us not patch the CRT, let us link solely to our own custom > +# allocator'. And it was almost so. > +# > +# But in the elder days Microsoft had said, 'let this piece of pre-compiled i'm unclear what 'elder days' means. I probably need to read more fantasy. @@ +199,5 @@ > +# their fix. So the Mozilla developers then said, 'Let us take the CRT import > +# library apart, rip out the object file that has offended us so, and replace > +# its unclean import symbol '__imp__free' with one that does not offend us > +# so'. And it was so. And the Mozilla developers saw that it was good. > +# I think a lot of it is history, which isn't the important thing to comment on. We want to understand what we do now, and why, not what we did. If I understand this correctly, some piece of object code (what piece is this, is that relevant?) is allocated via malloc_crt (object code is allocated? I'm confused. What do you mean by object code exactly?), and so can't be freed using plain free(). So since we can't touch the CRT, we replace its symbol '__impl_free' (which does what?) with one which doesn't "offend us" (in what way?). @@ +221,5 @@ > +crtdll_fixed.obj:: crtdll.obj > + $(PYTHON) $(srcdir)/fixcrt.py > + > +CRTDLL_FULLPATH=f:\\dd\\vctools\\crt_bld\\SELF_X86\\crt\\src\\build\\INTEL\\dll_obj\\crtdll.obj > + Why is there a hard coded path? ::: memory/jemalloc/jemalloc.c @@ +205,5 @@ > #include <string.h> > > #ifdef MOZ_MEMORY_WINDOWS > > +/* XXXkhuey switch to not patching the CRT for jemalloc all the time */ Can you file a bug for this, and put the bug number in the code next to a FIXME? In the bug, can you put an approximate date? @@ +211,4 @@ > #include <cruntime.h> > #include <internal.h> > +#else > +/* Some defines from the CRT internal headers that we need here. */ That's obvious. Is there no better comment we could write here, like why we need this? If we don't know why, say so (eg "MSDN page http://whatever says we need to do this, but there is no documentation on why"). @@ +6472,5 @@ > +#ifdef WIN32_NEW_STYLE_JEMALLOC > +/* > + * In the new style jemalloc integration jemalloc is built as a separate > + * shared library. Since we're no longer hooking into the CRT binary, > + * we need to initialize the heap at the first opportunity we get. Referencing the old behaviour is confusing, so better to remove it I think. @@ +6473,5 @@ > +/* > + * In the new style jemalloc integration jemalloc is built as a separate > + * shared library. Since we're no longer hooking into the CRT binary, > + * we need to initialize the heap at the first opportunity we get. > + * DLL_PROCESS_ATTACH in DllMain is that opportunity. So DllMain occurs during startup? Does it happen before _any_ other startup code? If so, mention it, if not, we may have a problem. @@ +6484,5 @@ > + case DLL_PROCESS_ATTACH: > + /* Don't force the system to page DllMain back in every time > + * we create/destroy a thread */ > + DisableThreadLibraryCalls(hModule); > + /* Initialize the heap */ \n ::: memory/jemalloc/jemalloc.h @@ +55,5 @@ > void *je_valloc(size_t size); > void *je_calloc(size_t num, size_t size); > void *je_realloc(void *ptr, size_t size); > void je_free(void *ptr); > +void *je_memalign(size_t alignment, size_t size); alignment

AFAIK shouldn't ::: configure.in > elif test "$CC_VERSION" == "16.00.30319.01"; then be sth like this: > elif test "$CC_VERSION" == "16.00.30319.01" -o "$CC_VERSION" == "16.00.40219.01"; then Just to support MSVC 2010 SP1 and WinSDK 7.1. ::: memory/jemalloc/jemalloc.c > void je_dumb_free_thunk(void *ptr) is already defined in line 6654 w/o a comment.

Oh for the second one it seems that, in my local repo, there were some pieces left over from the first patch, so there's only the SP1 thing.

Comment on attachment 539653 [details] [diff] [review] Patch >+CRTDLL_FULLPATH=f:\\dd\\vctools\\crt_bld\\SELF_X86\\crt\\src\\build\\INTEL\\dll_obj\\crtdll.obj Just trying to get this to work with VC8; I changed this to CRTDLL_FULLPATH=$(subst \,\\,$(shell lib -list msvc_combined.lib | grep crtdll\\.obj)) [not sure whether I've got the right number of \s but it seems to work]

Comment on attachment 539653 [details] [diff] [review] Patch >+# Grab both CRT libraries and combine them into one library to simplify things >+msvc_combined.lib:: >+ lib -OUT:$@ msvcrt.lib msvcprt.lib ifdef MOZ_DEBUG lib -OUT:$@ msvcrtd.lib msvcprtd.lib else lib -OUT:$@ msvcrt.lib msvcprt.lib endif

jemalloc only works in opt builds on windows, and changing that appears to be a substantial undertaking.

This posted using a debug jemalloc VC8 build. At least, the debugger tells me that it's using je_malloc()... I thought there was some UI that showed jemalloc was enabled, but I can't find it now.

Hangs trying to run Flash OOP though... maybe because I didn't clobber?

Comment on attachment 540121 [details] [diff] [review] Patch Changes: - Better comment - Removed unnecessary code move in config.mk - Synced config/ and js/src/config/ - Added check for VS 2010 SP 1 and verified that this works there.

Comment on attachment 540121 [details] [diff] [review] Patch Review of attachment 540121 [details] [diff] [review]: ----------------------------------------------------------------- jemalloc parts look great. ::: memory/jemalloc/Makefile.in @@ +209,5 @@ > +# import library and link the rest of Mozilla to that. > +# > +# The result? A binary that uses jemalloc, doesn't crash, and leaks a tiny > +# amount of memory (32 words per DLL in the 2010 CRT) at shutdown. > +# It's ambiguous whether you mean that the leak occurs early in the program and manifests at shutdown, or occurs at shutdown. I presume the latter, but might be worth clarifying. ::: memory/jemalloc/jemalloc.c @@ +206,5 @@ > > #ifdef MOZ_MEMORY_WINDOWS > > +/* XXXkhuey switch to not patching the CRT for jemalloc all the time */ > +#ifdef OLD_STYLE_JEMALLOC File a bug and make a comment here similar to: "FIXME: Bug 66xxxx - remove old jemalloc code"

Comment on attachment 540121 [details] [diff] [review] Patch >+# Linking Mozilla itself to jemalloc is not particularly difficult. >+# [snip] We discussed on IRC that I should read this comment and try to understand it. Although it's not as amusing as the last one, I think it's a lot clearer.

(In reply to comment #28) > (From update of attachment 539653 [details] [diff] [review]) > >+# Grab both CRT libraries and combine them into one library to simplify things > >+msvc_combined.lib:: > >+ lib -OUT:$@ msvcrt.lib msvcprt.lib > ifdef MOZ_DEBUG > lib -OUT:$@ msvcrtd.lib msvcprtd.lib > else > lib -OUT:$@ msvcrt.lib msvcprt.lib > endif Better still, use $(WIN32_CRT_LIBS) - that's why you created it, right?

Hmm, it turns out I also had to patch config.mk to move the use of MOZ_MEMORY_LDFLAGS out of the ifndef MOZ_DEBUG block.

(In reply to comment #36) > Comment on attachment 540121 [details] [diff] [review] [review] > Patch > > >+# Linking Mozilla itself to jemalloc is not particularly difficult. > >+# [snip] > > We discussed on IRC that I should read this comment and try to understand > it. Although it's not as amusing as the last one, I think it's a lot > clearer. Thanks for taking a look at it. (In reply to comment #37) > (In reply to comment #28) > > (From update of attachment 539653 [details] [diff] [review] [review]) > > >+# Grab both CRT libraries and combine them into one library to simplify things > > >+msvc_combined.lib:: > > >+ lib -OUT:$@ msvcrt.lib msvcprt.lib > > ifdef MOZ_DEBUG > > lib -OUT:$@ msvcrtd.lib msvcprtd.lib > > else > > lib -OUT:$@ msvcrt.lib msvcprt.lib > > endif > Better still, use $(WIN32_CRT_LIBS) - that's why you created it, right? Indeed! (In reply to comment #38) > Hmm, it turns out I also had to patch config.mk to move the use of > MOZ_MEMORY_LDFLAGS out of the ifndef MOZ_DEBUG block. Yes, I believe I had that in an earlier version but removed it since I didn't care about getting it working in debug builds. I would also expect that the mismatched free that we're replacing here is a problem in debug builds too (and the symbol would be '__imp__free_dbg' I believe).

windbgdlg.exe won't run if linked to mozcrt because it can't find MSVCR80D.dll so I worked around it by adding USE_STATIC_LIBS=1 to its Makefile. (In reply to comment #39) > (In reply to comment #38) > > Hmm, it turns out I also had to patch config.mk to move the use of > > MOZ_MEMORY_LDFLAGS out of the ifndef MOZ_DEBUG block. > Yes, I believe I had that in an earlier version but removed it since I > didn't care about getting it working in debug builds. Ah, that would explain why my VC8 build worked but my VC9 build didn't. > I would also expect that the mismatched free that we're replacing here is a > problem in debug builds too (and the symbol would be '__imp__free_dbg' I > believe). Doesn't affect VC8/9 of course. (Does VC10 even run on XP?)

(In reply to comment #40) > > I would also expect that the mismatched free that we're replacing here is a > > problem in debug builds too (and the symbol would be '__imp__free_dbg' I > > believe). > Doesn't affect VC8/9 of course. (Does VC10 even run on XP?) The compiler requires XP SP 3 or later, the binaries require XP SP 2 or later.

Bah, forgot I had to patch js/src/config.mk too... anyway, VC9 debug for you.

Ah, plugin-container is trying to link to MSVCR90D.DLL which is failing. Similar problem as windbgdlg.exe I guess.

Comment on attachment 540121 [details] [diff] [review] Patch Review of attachment 540121 [details] [diff] [review]: ----------------------------------------------------------------- r+, just some nitpicky comments. ::: config/autoconf.mk.in @@ +637,5 @@ > MSMANIFEST_TOOL = @MSMANIFEST_TOOL@ > WIN32_REDIST_DIR = @WIN32_REDIST_DIR@ > WIN32_CRT_SRC_DIR = @WIN32_CRT_SRC_DIR@ > MOZ_MEMORY_LDFLAGS = @MOZ_MEMORY_LDFLAGS@ > +WIN32_OLD_STYLE_JEMALLOC = @WIN32_OLD_STYLE_JEMALLOC@ You said you're going to test this new-style on 2005. I'm okay with checking this in as-is, but please file a followup on that. If it's too much of a pain, then the followup will just be "Remove this code when we no longer support VC2005/2008". ::: configure.in @@ +7440,5 @@ > + elif test "$CC_VERSION" == "16.00.30319.01" -o "$CC_VERSION" == "16.00.40219.01"; then > + WIN32_NEW_STYLE_JEMALLOC=1 > + AC_DEFINE(WIN32_NEW_STYLE_JEMALLOC) > + else > + AC_MSG_ERROR([Building jemalloc requires exactly Visual C++ 2005 SP1 or 2008 SP1 or 2010 currently.]) This could probably be formatted a little more clearly "exactly Visual C++ 2005 SP1, 2008 SP1, or 2010". Might as well drop the "currently" too. ::: memory/jemalloc/Makefile.in @@ +215,5 @@ > + > +libs:: $(DIST)/lib/mozcrt.lib > + > +$(DIST)/lib/mozcrt.lib:: mozcrt.lib > + $(INSTALL) $(IFLAGS2) mozcrt.lib $(DIST)/lib Hrm. I guess there's just no way to get the ordering correct here to avoid installing the other import lib without the build machinery you added? Also, use $< instead of repeating the filename. Also, you could just merge this into the mozcrt.lib rule below, it's not like you really need dependencies here. @@ +218,5 @@ > +$(DIST)/lib/mozcrt.lib:: mozcrt.lib > + $(INSTALL) $(IFLAGS2) mozcrt.lib $(DIST)/lib > + > +# And finally combine that with the jemalloc import library to get an import > +# library that has our malloc/free/etc and the CRT's everything else These comments read in reverse order from top to bottom which is...odd. @@ +219,5 @@ > + $(INSTALL) $(IFLAGS2) mozcrt.lib $(DIST)/lib > + > +# And finally combine that with the jemalloc import library to get an import > +# library that has our malloc/free/etc and the CRT's everything else > +mozcrt.lib:: $(IMPORT_LIBRARY) msvc_modified.lib None of these rules need to be double-colon, FWIW, they're all standard rules. @@ +223,5 @@ > +mozcrt.lib:: $(IMPORT_LIBRARY) msvc_modified.lib > + lib -OUT:$@ $^ > + > +# Put the fixed object file back in > +msvc_modified.lib:: msvc_removed.lib crtdll_fixed.obj You've got a lot of rules here, feels like you could combine these down to a single rule since they all fall in lockstep anyway, like: msvc_modified.lib: ... build msvc_combined.lib extract crtdll.obj remove from combined -> removed fix object file put it back in That might make this a bit easier to read, since things would be in order. I guess you could merge this and the mozcrt.lib rule above, too... @@ +230,5 @@ > +# Fix the object file > +crtdll_fixed.obj:: crtdll.obj > + $(PYTHON) $(srcdir)/fixcrt.py > + > +CRTDLL_FULLPATH=f:\\dd\\vctools\\crt_bld\\SELF_X86\\crt\\src\\build\\INTEL\\dll_obj\\crtdll.obj This could probably stand a comment. @@ +242,5 @@ > + lib -OUT:$@ $^ -EXTRACT:$(CRTDLL_FULLPATH) > + > +# Grab both CRT libraries and combine them into one library to simplify things > +msvc_combined.lib:: > + lib -OUT:$@ msvcrt.lib msvcprt.lib Did you mean to use $(WIN32_CRT_LIBS) here, since you defined it in configure? ::: memory/jemalloc/fixcrt.py @@ +34,5 @@ > +# the terms of any one of the MPL, the GPL or the LGPL. > +# > +# ***** END LICENSE BLOCK ***** > + > +with open('crtdll.obj', 'rb') as infile: You should probably throw a "from __future__ import with_statement" up top just in case we wind up running this on Python 2.5. Can you put a small comment block explaining what this file does as well?

One thing that I just thought of, this is probably going to screw up packaging as-is, because we'll need to start shipping the CRT redist again. If you make this work for all VC++ versions, then you just need to remove the ifdef MOZ_MEMORY here: http://mxr.mozilla.org/mozilla-central/source/build/win32/Makefile.in#65 and ensure that WIN32_REDIST_DIR gets set in our official mozconfigs. If not, you'll probably need a goofier ifdef to only package those for VC2010 ifdef MOZ_MEMORY, but still get the mozconfigs updated.

I just did a build with this patch, and jemalloc.dll wasn't copied to dist/firefox after running |make package|. I was able to launch the browser fine after copying it from dist/bin, though.

Ah, crap, you will need to add jemalloc.dll to the package manifest...

Yes, I was aware of that.

I landed this and a number of android fixes (silly macros) on b-s and now m-c http://hg.mozilla.org/mozilla-central/rev/d77cddcc9bd6 http://hg.mozilla.org/mozilla-central/rev/929f956a1a42 http://hg.mozilla.org/mozilla-central/rev/749eeb73cda4 http://hg.mozilla.org/mozilla-central/rev/1773f004b699 http://hg.mozilla.org/mozilla-central/rev/df1eee89e031 http://hg.mozilla.org/mozilla-central/rev/f17ad44b0d87 http://hg.mozilla.org/mozilla-central/rev/afb7bc7bc3fa http://hg.mozilla.org/mozilla-central/rev/ab19fc2d0dfe

(In reply to comment #44) > (From update of attachment 540121 [details] [diff] [review]) > > +WIN32_OLD_STYLE_JEMALLOC = @WIN32_OLD_STYLE_JEMALLOC@ > You said you're going to test this new-style on 2005. I'm okay with checking > this in as-is, but please file a followup on that. If it's too much of a > pain, then the followup will just be "Remove this code when we no longer > support VC2005/2008". Works fine on VC2005, except debug plugin-container.exe and windbgdlg.exe can't find MSVCR90D.dll :-(