Last Comment Bug 515492 - change to wince-style jemalloc usage for win32
: change to wince-style jemalloc usage for win32
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Memory Allocator (show other bugs)
: Trunk
: x86 Windows NT
: -- normal with 3 votes (vote)
: mozilla7
Assigned To: Kyle Huey [:khuey] (khuey@mozilla.com)
:
Mentors:
Depends on:
Blocks: 563293 msvc2010 563319 586962 649607 668151 671441 1236330
  Show dependency treegraph
 
Reported: 2009-09-09 15:35 PDT by Ted Mielczarek [:ted.mielczarek]
Modified: 2016-01-04 08:22 PST (History)
36 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (21.59 KB, patch)
2011-06-13 14:03 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
no flags Details | Diff | Review
Patch (23.47 KB, patch)
2011-06-15 14:02 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
no flags Details | Diff | Review
Patch (15.56 KB, patch)
2011-06-17 13:44 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
no flags Details | Diff | Review
Patch (21.72 KB, patch)
2011-06-17 13:49 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
ted: review+
paul.biggar: review+
Details | Diff | Review

Description Ted Mielczarek [:ted.mielczarek] 2009-09-09 15:35:42 PDT
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.
Comment 1 Vladimir Vukicevic [:vlad] [:vladv] 2009-09-09 15:38:45 PDT
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.
Comment 2 Ted Mielczarek [:ted.mielczarek] 2009-09-09 15:40:07 PDT
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.
Comment 3 Stuart Parmenter 2009-09-09 17:14:37 PDT
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.
Comment 4 Mitchell Field [:Mitch] 2009-09-17 05:41:49 PDT
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.
Comment 5 neil@parkwaycc.co.uk 2010-04-27 11:14:25 PDT
What goes wrong if only our infallible wrappers use jemalloc?
Comment 6 Vladimir Vukicevic [:vlad] [:vladv] 2010-05-10 15:28:54 PDT
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.
Comment 7 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-05-10 15:38:37 PDT
(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.
Comment 8 neil@parkwaycc.co.uk 2010-05-10 16:55:02 PDT
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?
Comment 9 Ted Mielczarek [:ted.mielczarek] 2010-06-01 12:35:39 PDT
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.
Comment 10 Bas Schouten (:bas.schouten) 2010-06-19 08:28:56 PDT
(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.
Comment 11 Ted Mielczarek [:ted.mielczarek] 2010-06-20 14:38:29 PDT
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.
Comment 12 Mook (songbird dead account) 2010-06-21 11:38:23 PDT
(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.)
Comment 13 Mike Shaver (:shaver -- probably not reading bugmail closely) 2010-10-13 23:17:23 PDT
Adding jasone, who has been looking at some related things.
Comment 14 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-13 10:44:35 PDT
I have a fix in hand here.  Just beating it into a reviewable form now.
Comment 15 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-13 14:03:50 PDT
Created attachment 539006 [details] [diff] [review]
Patch

It has a few rough edges, but it's suitable for review I think.
Comment 16 Mitchell Field [:Mitch] 2011-06-15 06:16:05 PDT
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 17 Ted Mielczarek [:ted.mielczarek] 2011-06-15 09:25:04 PDT
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...
Comment 18 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-15 09:30:30 PDT
(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 19 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-15 10:54:47 PDT
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 20 Paul Biggar 2011-06-15 11:51:39 PDT
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 21 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-15 13:07:59 PDT
Comment on attachment 539006 [details] [diff] [review]
Patch

Updated patch later today, complete with a giant comment explaining the story here.
Comment 22 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-15 14:02:41 PDT
Created attachment 539653 [details] [diff] [review]
Patch
Comment 23 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-15 14:04:57 PDT
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 24 Paul Biggar 2011-06-15 14:38:34 PDT
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
Comment 25 Johannes Pfrang [:johnp] 2011-06-16 04:22:01 PDT
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.
Comment 26 Johannes Pfrang [:johnp] 2011-06-16 04:45:16 PDT
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 27 neil@parkwaycc.co.uk 2011-06-16 12:15:55 PDT
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 28 neil@parkwaycc.co.uk 2011-06-16 13:13:03 PDT
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
Comment 29 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-16 13:15:40 PDT
jemalloc only works in opt builds on windows, and changing that appears to be a substantial undertaking.
Comment 30 neil@parkwaycc.co.uk 2011-06-17 02:05:46 PDT
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.
Comment 31 neil@parkwaycc.co.uk 2011-06-17 02:10:08 PDT
Hangs trying to run Flash OOP though... maybe because I didn't clobber?
Comment 32 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-17 13:44:28 PDT
Created attachment 540119 [details] [diff] [review]
Patch
Comment 33 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-17 13:49:32 PDT
Created attachment 540121 [details] [diff] [review]
Patch
Comment 34 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-17 13:50:42 PDT
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 35 Paul Biggar 2011-06-17 17:09:58 PDT
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 36 Justin Lebar (not reading bugmail) 2011-06-18 12:20:39 PDT
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.
Comment 37 neil@parkwaycc.co.uk 2011-06-19 14:30:31 PDT
(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?
Comment 38 neil@parkwaycc.co.uk 2011-06-19 15:13:54 PDT
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.
Comment 39 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-19 20:08:19 PDT
(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).
Comment 40 neil@parkwaycc.co.uk 2011-06-21 03:31:41 PDT
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?)
Comment 41 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-21 03:39:16 PDT
(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.
Comment 42 neil@parkwaycc.co.uk 2011-06-21 16:15:19 PDT
Bah, forgot I had to patch js/src/config.mk too... anyway, VC9 debug for you.
Comment 43 neil@parkwaycc.co.uk 2011-06-22 03:12:08 PDT
Ah, plugin-container is trying to link to MSVCR90D.DLL which is failing. Similar problem as windbgdlg.exe I guess.
Comment 44 Ted Mielczarek [:ted.mielczarek] 2011-06-24 13:20:43 PDT
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?
Comment 45 Ted Mielczarek [:ted.mielczarek] 2011-06-26 10:18:23 PDT
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.
Comment 46 Ryan VanderMeulen [:RyanVM] 2011-06-27 16:13:48 PDT
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.
Comment 47 Ted Mielczarek [:ted.mielczarek] 2011-06-27 19:38:58 PDT
Ah, crap, you will need to add jemalloc.dll to the package manifest...
Comment 48 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-27 20:15:30 PDT
Yes, I was aware of that.
Comment 50 neil@parkwaycc.co.uk 2011-06-28 11:55:27 PDT
(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 :-(

Note You need to log in before you can comment on or make changes to this bug.