At present, a JS shell built as part of a browser build uses jemalloc, but a JS shell built stand-alone (by running js/src/configure directly) does not use jemalloc. The JS shell build process should use jemalloc in both cases. This should make the allocator behavior SM hackers see more consistent with that in the browser. It will also make writing tests for devtools that measure memory consumption a bit easier.

Here's what seems to be necessary to make JS shell stand-alone builds use jemalloc, based on an IRC conversation with Mike Hommey (glandium). moz.build files can check `CONFIG['JS_STANDALONE']` to see whether we're building the whole tree, or just js/src. (That value is cleared in the top-level configure.in, and then consulted (and defaulted on) in js/src/configure.in. For Linux and OSX: - The top-level moz.build needs to include 'memory' in DIRS for stand-alone shell builds. - js/src/moz.build needs to have: USE_LIBS += ['memory']. However, replacing malloc on Windows is much more involved, and is managed while building libmozglue. The file `mozglue/crt/Makefile.in` takes the standard Microsoft CRT import library; extracts certain pieces from it; tweaks them; replaces other pieces; and reassembles a new import library, `mozcrt.lib`. Placing this first on the link command line supplants the normal Windows CRT sufficiently well to make almost all allocation use jemalloc, and prevent the allocations we miss from doing any harm. The stand-alone shell build will need to use libmozglue, although it doesn't need many of the other files included there. Specifically, in a JS_STANDALONE build, in `mozglue/build/moz.build`, we should pare down what's included in libmozglue, except for the following: - SOURCES should still include BionicGlue.cpp and AsanOptions.cpp. - We should still link against mfbt. - We should still set DEFFILE on Windows. Naturally, the script `js/src/make-source-package.sh` will need to be updated to include the new source directories.

For embedder JS_STANDALONE builds, it would still be useful to keep a configure option to use system malloc.

(In reply to Sean Stangl [:sstangl] from comment #2) > For embedder JS_STANDALONE builds, it would still be useful to keep a > configure option to use system malloc. Yeah, all we'd be doing here is flipping the default in js/src/configure.in (and making the flag actually have an effect! Right now it's dangling.)

Here's a patch that seems to work on Linux, and possibly OSX. Windows hacks coming next.

Try push for the above. Note that our try farm doesn't exercise stand-alone builds. https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8221bd11505

Okay, this version of the patch should work on Windows and Android as well.

(In reply to Jim Blandy :jimb from comment #5) > Note that our try farm doesn't exercise stand-alone builds. This is false; the SM(...) builds are stand-alone builds.

(In reply to Jim Blandy :jimb from comment #7) > (In reply to Jim Blandy :jimb from comment #5) > > Note that our try farm doesn't exercise stand-alone builds. > > This is false; the SM(...) builds are stand-alone builds. And there are even SM(...) builds for Windows, but they're hidden by default. You must click on the "mystery box" to the right of the "unclassified" count in the upper right to see them.

Try push looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb5a1145a0b3

Comment on attachment 8567249 [details] [diff] [review] Enable jemalloc by default for stand-alone SpiderMonkey builds. Review of attachment 8567249 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/make-source-package.sh @@ +69,5 @@ > > cp -t ${tgtpath} -dRp \ > + ${SRCDIR}/../../python \ > + ${SRCDIR}/../../memory \ > + ${SRCDIR}/../../mozglue It would be better to only import mozglue/build, mozglue/crt and mozglue/moz.build. ::: js/src/moz.build @@ +31,5 @@ > + if CONFIG['MOZ_MEMORY']: > + USE_LIBS += [ > + 'memory', > + 'mozglue', > + ] Try removing https://dxr.mozilla.org/mozilla-central/source/build/gecko_templates.mozbuild#55 instead, remove the USE_LIBS += ['mfbt'] above, and make it build mozglue whether or not jemalloc is enabled. ::: mozglue/tests/moz.build @@ +5,5 @@ > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > DISABLE_STL_WRAPPING = True > > +if not CONFIG['JS_STANDALONE']: This would be better in mozglue/moz.build.

(In reply to Mike Hommey [:glandium] from comment #10) > It would be better to only import mozglue/build, mozglue/crt and > mozglue/moz.build. Okay. And mozglue won't mind missing its 'test' subdirectory because we're also moving the |if not CONFIG['JS_STANDALONE']:| to mozglue/moz.build. > remove the USE_LIBS += ['mfbt'] above Does this work because mozglue will bring in mfbt anyway? It seems weird for js/src/moz.build not to mention mfbt, as SpiderMonkey makes extensive use of mfbt.

Okay, I think this addresses all review comments.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f8066dfd9e0

That try push looks good, except for the OSX 10.6 failures, which are unrelated (bug 1142006). Once that's resolved, I'll do a separate push for OSX 10.6 only.

Comment on attachment 8576223 [details] [diff] [review] Make SpiderMonkey standalone (JS_STANDALONE) builds use jemalloc and mozglue by default. Review of attachment 8576223 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/make-source-package.sh @@ +68,5 @@ > fi > > cp -t ${tgtpath} -dRp \ > + ${SRCDIR}/../../python \ > + ${SRCDIR}/../../memory It would probably be better to remove memory/replace. It's never going to be used since the js build system never sets MOZ_REPLACE_MALLOC. ::: js/src/moz.build @@ +25,5 @@ > > if CONFIG['JS_STANDALONE']: > DEFINES['IMPL_MFBT'] = True > + if CONFIG['MOZ_MEMORY']: > + USE_LIBS += [ 'memory' ] That's already done appropriately in the GeckoBinary template. IOW, this change shouldn't be necessary.

(In reply to Mike Hommey [:glandium] from comment #15) > That's already done appropriately in the GeckoBinary template. IOW, this > change shouldn't be necessary. Dropping it yielded JS binaries that don't use jemalloc. The check for MOZ_MEMORY in gecko_templates.mozbuild is supposed to bring it in for us, but it's never being reached because MOZ_GLUE_IN_PROGRAM isn't set. Looking at the code in $top/configure.in that sets MOZ_GLUE_IN_PROGRAM, it seemed like js/src/configure should do the same. Adding that code caused problems when linking libmozjs.so: $top/js/src/jsmath.cpp:757: error: undefined reference to 'mozilla::unused' it seemed that the hack described here to mark mfbt references in the .so as 'weak' wasn't working: https://hg.mozilla.org/mozilla-central/file/41a61514461e/mfbt/Types.h#l85 For example, jsmath.o contained the following reference: 2163: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND _ZN7mozilla6unusedE I guessed that this might be because the moz.build files for GeckoBinaries under js/src all set IMPL_MFBT, creating strong references, which are verboten in .so files. Would it be all right if I left deleting that MOZ_MEMORY check from js/src/moz.build for a follow-up bug?

Revised per comments. Changes to IMPL_MFBT and MOZ_GLUE_IN_PROGRAM as discussed on IRC.

Comment on attachment 8579703 [details] [diff] [review] jemalloc-standalone.patch Review of attachment 8579703 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/configure.in @@ +3046,5 @@ > +*) > + dnl On !Android !Windows !OSX, we only want to link executables against mozglue > + MOZ_GLUE_IN_PROGRAM=1 > + ;; > +esac Can you put this below --enable-jemalloc, like in top-level configure.in? Can you also remove the export MOZ_GLUE_IN_PROGRAM near the end of top-level configure.in? It's not necessary anymore with this. ::: js/src/make-source-package.sh @@ +97,5 @@ > + cp -t ${tgtpath}/memory \ > + ${SRCDIR}/../../memory/moz.build \ > + ${SRCDIR}/../../memory/build \ > + ${SRCDIR}/../../memory/jemalloc \ > + ${SRCDIR}/../../memory/mozjemalloc Seeing all those aligned ${SRCDIR}/../../, I can't help but thinking we might as well set TOPSRCDIR to that, use ${TOPSRCDIR} instead.

I've made these changes. Thanks for the review. Final try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb4ea15dceae

It'll be interesting to see how this affects the numbers on AWFY.

(In reply to Nicholas Nethercote [:njn] from comment #20) > It'll be interesting to see how this affects the numbers on AWFY. BTW, I have no idea what the effect will be. Regardless of what happens, reducing the difference between shell builds and browser builds can only be a good thing for AWFY.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9f44c21af57 In earlier try pushes, I've seen the message: Assertion failure: T(double(t)) == t (value creation would be lossy), at ../../dist/include/js/Value.h:1596 By the time I'd hacked in something to get me a backtrace, it had stopped occurring. I filed bug 1145997 to give people something to star if it recurs.

I'm inclined to back this out from mozilla-inbound for now, unless you expect that a fix for bug 1140773 is forthcoming soon, given that I think the increase in orange from bug 1140773 that this triggered: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-searchStr=spidermonkey probably means that mozilla-inbound is not in a mergeable (to mozilla-central, etc.) state.

I just attached a fix to that bug. If we can get a review, it's ready to land.

\o/ jimb, can you watch AWFY and report back once the new code has had a run through, about whether it made any perf differences? Thanks.

Sure thing; I'll leave the needinfo on until then, to remind me.

Thanks! This is great for fuzzing and benchmarking.

(In reply to Nicholas Nethercote [:njn] from comment #27) > jimb, can you watch AWFY and report back once the new code has had a run > through, about whether it made any perf differences? Thanks. This broke OS X shell builds (bug 1146267) and Hannes added --disable-jemalloc to AWFY's shell builds. So we should fix bug 1146267 and remove --disable-jemalloc from AWFY before we can say anything.

(In reply to Nicholas Nethercote [:njn] (on vacation until April 30th) from comment #27) > jimb, can you watch AWFY and report back once the new code has had a run > through, about whether it made any perf differences? Thanks. Okay, we have some results back. For example, this changeset includes the jemalloc-by-default patch, and has gone through AWFY: http://hg.mozilla.org/integration/mozilla-inbound/rev/bf22c9e5c5a3 There doesn't seem to be any significant effect on performance.