Last Comment Bug 1134039 - Stand-alone JavaScript shell builds should use jemalloc too
: Stand-alone JavaScript shell builds should use jemalloc too
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
-- normal (vote)
: mozilla39
Assigned To: Jim Blandy :jimb
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 1150659 1153683 1134428 1140773 1146267 1155438 1160267
Blocks: 1062473
  Show dependency treegraph
 
Reported: 2015-02-17 15:50 PST by Jim Blandy :jimb
Modified: 2015-04-30 14:56 PDT (History)
13 users (show)
jimb: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Enable jemalloc by default for stand-alone SpiderMonkey builds (for Linux and OSX; Windows, Android left broken). (4.10 KB, patch)
2015-02-18 15:39 PST, Jim Blandy :jimb
no flags Details | Diff | Splinter Review
Enable jemalloc by default for stand-alone SpiderMonkey builds. (8.49 KB, patch)
2015-02-20 11:43 PST, Jim Blandy :jimb
mh+mozilla: feedback+
Details | Diff | Splinter Review
Make SpiderMonkey standalone (JS_STANDALONE) builds use jemalloc and mozglue by default. (9.70 KB, patch)
2015-03-11 13:25 PDT, Jim Blandy :jimb
mh+mozilla: feedback+
Details | Diff | Splinter Review
jemalloc-standalone.patch (10.03 KB, patch)
2015-03-18 17:08 PDT, Jim Blandy :jimb
mh+mozilla: review+
Details | Diff | Splinter Review

Description User image Jim Blandy :jimb 2015-02-17 15:50:06 PST
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.
Comment 1 User image Jim Blandy :jimb 2015-02-17 23:24:34 PST
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.
Comment 2 User image Sean Stangl [:sstangl] 2015-02-18 10:51:14 PST
For embedder JS_STANDALONE builds, it would still be useful to keep a configure option to use system malloc.
Comment 3 User image Jim Blandy :jimb 2015-02-18 11:46:13 PST
(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.)
Comment 4 User image Jim Blandy :jimb 2015-02-18 15:39:42 PST
Created attachment 8566287 [details] [diff] [review]
Enable jemalloc by default for stand-alone SpiderMonkey builds (for Linux and OSX; Windows, Android left broken).

Here's a patch that seems to work on Linux, and possibly OSX. Windows hacks coming next.
Comment 5 User image Jim Blandy :jimb 2015-02-19 13:24:28 PST
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
Comment 6 User image Jim Blandy :jimb 2015-02-20 11:43:19 PST
Created attachment 8567249 [details] [diff] [review]
Enable jemalloc by default for stand-alone SpiderMonkey builds.

Okay, this version of the patch should work on Windows and Android as well.
Comment 7 User image Jim Blandy :jimb 2015-02-21 14:50:57 PST
(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.
Comment 8 User image Jim Blandy :jimb 2015-02-21 14:52:06 PST
(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.
Comment 9 User image Jim Blandy :jimb 2015-02-23 13:30:05 PST
Try push looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb5a1145a0b3
Comment 10 User image Mike Hommey [:glandium] 2015-02-23 20:01:42 PST
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.
Comment 11 User image Jim Blandy :jimb 2015-02-25 15:22:51 PST
(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.
Comment 12 User image Jim Blandy :jimb 2015-03-11 13:25:00 PDT
Created attachment 8576223 [details] [diff] [review]
Make SpiderMonkey standalone (JS_STANDALONE) builds use jemalloc and mozglue by default.

Okay, I think this addresses all review comments.
Comment 13 User image Jim Blandy :jimb 2015-03-11 13:27:49 PDT
Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f8066dfd9e0
Comment 14 User image Jim Blandy :jimb 2015-03-12 15:08:36 PDT
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 15 User image Mike Hommey [:glandium] 2015-03-12 18:45:25 PDT
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.
Comment 16 User image Jim Blandy :jimb 2015-03-18 15:22:09 PDT
(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?
Comment 17 User image Jim Blandy :jimb 2015-03-18 17:08:01 PDT
Created attachment 8579703 [details] [diff] [review]
jemalloc-standalone.patch

Revised per comments. Changes to IMPL_MFBT and MOZ_GLUE_IN_PROGRAM as discussed on IRC.
Comment 18 User image Mike Hommey [:glandium] 2015-03-18 23:45:09 PDT
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.
Comment 19 User image Jim Blandy :jimb 2015-03-19 02:34:03 PDT
I've made these changes. Thanks for the review.

Final try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb4ea15dceae
Comment 20 User image Nicholas Nethercote [:njn] 2015-03-19 14:41:49 PDT
It'll be interesting to see how this affects the numbers on AWFY.
Comment 21 User image Nicholas Nethercote [:njn] 2015-03-19 18:39:25 PDT
(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.
Comment 22 User image Jim Blandy :jimb 2015-03-21 10:22:40 PDT
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.
Comment 24 User image David Baron :dbaron: ⌚️UTC-8 2015-03-21 22:00:10 PDT
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.
Comment 25 User image Jim Blandy :jimb 2015-03-21 22:27:25 PDT
I just attached a fix to that bug. If we can get a review, it's ready to land.
Comment 26 User image Phil Ringnalda (:philor) 2015-03-22 14:00:58 PDT
https://hg.mozilla.org/mozilla-central/rev/5e45fba743aa
Comment 27 User image Nicholas Nethercote [:njn] 2015-03-22 16:23:46 PDT
\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.
Comment 28 User image Jim Blandy :jimb 2015-03-22 22:36:11 PDT
Sure thing; I'll leave the needinfo on until then, to remind me.
Comment 29 User image Jan de Mooij [:jandem] 2015-03-23 02:12:00 PDT
Thanks! This is great for fuzzing and benchmarking.
Comment 30 User image Jan de Mooij [:jandem] 2015-03-23 02:45:55 PDT
(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.
Comment 31 User image Jim Blandy :jimb 2015-03-30 09:32:09 PDT
(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.

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