Stand-alone JavaScript shell builds should use jemalloc too

RESOLVED FIXED in Firefox 39

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: jimb, Assigned: jimb)

Tracking

(Depends on: 2 bugs)

unspecified
mozilla39
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

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

Comment 1

3 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.
For embedder JS_STANDALONE builds, it would still be useful to keep a configure option to use system malloc.
(Assignee)

Comment 3

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

Updated

3 years ago
Depends on: 1134428
(Assignee)

Comment 4

3 years ago
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.
Assignee: nobody → jimb
Status: NEW → ASSIGNED
(Assignee)

Comment 5

3 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
(Assignee)

Comment 6

3 years ago
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.
Attachment #8566287 - Attachment is obsolete: true
Attachment #8567249 - Flags: review?(mh+mozilla)
(Assignee)

Updated

3 years ago
Blocks: 1062473
(Assignee)

Comment 7

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

Comment 8

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

Comment 9

3 years ago
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.
Attachment #8567249 - Flags: review?(mh+mozilla) → feedback+
(Assignee)

Comment 11

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

Comment 12

3 years ago
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.
Attachment #8567249 - Attachment is obsolete: true
Attachment #8576223 - Flags: review?(mh+mozilla)
(Assignee)

Comment 13

3 years ago
Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f8066dfd9e0
(Assignee)

Comment 14

3 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.
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.
Attachment #8576223 - Flags: review?(mh+mozilla) → feedback+
(Assignee)

Comment 16

2 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?
(Assignee)

Comment 17

2 years ago
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.
Attachment #8576223 - Attachment is obsolete: true
Attachment #8579703 - Flags: review?(mh+mozilla)
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.
Attachment #8579703 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 19

2 years ago
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.
(Assignee)

Comment 22

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

Comment 23

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e45fba743aa
Flags: in-testsuite-
Target Milestone: --- → mozilla39
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.
Flags: needinfo?(jimb)
(Assignee)

Comment 25

2 years ago
I just attached a fix to that bug. If we can get a review, it's ready to land.
Flags: needinfo?(jimb)
Depends on: 1140773
https://hg.mozilla.org/mozilla-central/rev/5e45fba743aa
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
\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.
Flags: needinfo?(jimb)
(Assignee)

Comment 28

2 years ago
Sure thing; I'll leave the needinfo on until then, to remind me.
Thanks! This is great for fuzzing and benchmarking.

Updated

2 years ago
Depends on: 1146267
(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.
(Assignee)

Comment 31

2 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.
Flags: needinfo?(jimb)
Depends on: 1150659

Updated

2 years ago
Depends on: 1155438

Updated

2 years ago
Depends on: 1153683
Depends on: 1160267
You need to log in before you can comment on or make changes to this bug.