Jim Blandy :jimb Assignee
	Description • 10 years ago

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.

	Jim Blandy :jimb Assignee
	Comment 1 • 10 years ago

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.

	Sean Stangl [:sstangl]
	Comment 2 • 10 years ago

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

	Jim Blandy :jimb Assignee
	Comment 3 • 10 years ago

(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.)

	Jim Blandy :jimb Assignee
	Comment 4 • 10 years ago

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

	Jim Blandy :jimb Assignee
	Comment 5 • 10 years ago

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

	Jim Blandy :jimb Assignee
	Comment 6 • 10 years ago

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

	Jim Blandy :jimb Assignee
	Comment 7 • 10 years ago

(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.

	Jim Blandy :jimb Assignee
	Comment 8 • 10 years ago

(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.

	Jim Blandy :jimb Assignee
	Comment 9 • 10 years ago

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

	Mike Hommey [:glandium]
	Comment 10 • 10 years ago

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.

	Jim Blandy :jimb Assignee
	Comment 11 • 10 years ago

(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.

	Jim Blandy :jimb Assignee
	Comment 12 • 10 years ago

Okay, I think this addresses all review comments.

	Jim Blandy :jimb Assignee
	Comment 13 • 10 years ago

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

	Jim Blandy :jimb Assignee
	Comment 14 • 10 years ago

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.

	Mike Hommey [:glandium]
	Comment 15 • 10 years ago

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.

	Jim Blandy :jimb Assignee
	Comment 16 • 9 years ago

(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?

	Jim Blandy :jimb Assignee
	Comment 17 • 9 years ago

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

	Mike Hommey [:glandium]
	Comment 18 • 9 years ago

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.

	Jim Blandy :jimb Assignee
	Comment 19 • 9 years ago

I've made these changes. Thanks for the review.

Final try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb4ea15dceae

	Nicholas Nethercote [inactive]
	Comment 20 • 9 years ago

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

	Nicholas Nethercote [inactive]
	Comment 21 • 9 years ago

(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.

	Jim Blandy :jimb Assignee
	Comment 22 • 9 years ago

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.

	Jim Blandy :jimb Assignee
	Comment 23 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e45fba743aa

	David Baron :dbaron:
	Comment 24 • 9 years ago

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.

	Jim Blandy :jimb Assignee
	Comment 25 • 9 years ago

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

	Phil Ringnalda (:philor)
	Comment 26 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/5e45fba743aa

	Nicholas Nethercote [inactive]
	Comment 27 • 9 years ago

\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.

	Jim Blandy :jimb Assignee
	Comment 28 • 9 years ago

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

	Jan de Mooij [:jandem]
	Comment 29 • 9 years ago

Thanks! This is great for fuzzing and benchmarking.

	Jan de Mooij [:jandem]
	Comment 30 • 9 years ago

(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.

	Jim Blandy :jimb Assignee
	Comment 31 • 9 years ago

(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.