1251324 - Make building the JS shell something people can opt out from in .mozconfig

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Ryan VanderMeulen [:RyanVM]]

I do my own local builds. I also use whole-program optimization. As a result, linking js.exe adds a nontrivial amount of time to my builds that is completely unnecessary since I just want to build Firefox.

At the moment, building the JS shell is guarded on a |if not CONFIG['JS_DISABLE_SHELL']| check. However, configure.in for the JS engine initializes JS_DISABLE_SHELL to false by default and doesn't provide any way of overriding that. It doesn't seem like adjusting configure.in to only set |JS_DISABLE_SHELL=| if it's not already set would be too difficult to accomplish.

Comment 1

[Nicolas B. Pierron [:nbp]]

CC ted, glandium,

Sounds like this could save a lot of time [1] for all non-JS engine developers in general.

As this implicitly implies not running the JS shell tests.  I'll suggest that we should also disable the compilation of the jsapi-tests [2] as these are taking even more time than linking the js shell.

[1] https://dxr.mozilla.org/mozilla-central/source/js/src/shell/moz.build
[2] https://dxr.mozilla.org/mozilla-central/source/js/src/moz.build#60

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Attachment: Add --disable-js-shell option

Appears to work as expected. Local testing confirms that adding --disable-js-shell to my .mozconfig skips the shell build and my push to Try confirms no change in behavior for CI builds (as expected). I also took nbp's suggestion and made the jsapi-tests conditional on the shell being built as well.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6cb242c74f26&group_state=expanded&exclusion_profile=false

Comment 3

[Mike Hommey [:glandium]]

You know what? I'm tempted to say we should make this the default for non-js-standalone builds, and make automation builds enable it through their mozconfig (and we don't even need to opt-in in all of them, I'm fairly sure there are build types that don't care about the js shell and the jit tests)

gps, what do you think?

Another option would be leave it alone, and see how long it takes to link without LTCG:PGOPTIMIZE.

Comment 4

[Gregory Szorc [:gps]]

If it is true that most users won't need the JS shell and won't need to run the JS shell and jsapi tests, then I think we should disable them by default to make builds faster for the average developer.

I've been spending a lot of time doing whole program optimization builds for PGO testing lately and I can confirm that PGO instrumentation of various JS foo takes a really long time. Not as long as xul.dll. But still several minutes. I think every little bit helps.

Let's disable JS shell and related foo. Let's do this by keeping them enabled by default in js/src's configure. But let's have the top-level configure pass the disable flags into js/src. That way there is no change for people just doing js/src builds (who likely care about these features) and Gecko/Firefox developers get the speedup without having to change their configuration.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Can we take this patch for now and build off it in another bug?

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8723922 [details] [diff] [review]
Add --disable-js-shell option

Review of attachment 8723922 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/moz.build
@@ +48,5 @@
>  
>  with Files('../public/TrackedOptimizationInfo.h'):
>      BUG_COMPONENT = component_jit
>  
> +# editline needs to get built before the shell

This comment is irrelevant nowadays.

::: js/src/old-configure.in
@@ +3380,5 @@
> +dnl ========================================================
> +MOZ_ARG_DISABLE_BOOL(js-shell,
> +[  --disable-js-shell         Do not build the JS shell],
> +    JS_DISABLE_SHELL=1,
> +    JS_DISABLE_SHELL= )

I'd rather add this in python configure instead, and since this is simple enough, I'll just do it.

Comment 7

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1251324 - Add a --disable-js-shell option to js/src/configure

Review commit: https://reviewboard.mozilla.org/r/38885/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38885/

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 8728269 [details]
MozReview Request: Bug 1251324 - Add a --disable-js-shell option to js/src/configure

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38885/diff/1-2/

Comment 9

[Mike Hommey [:glandium]]

I'm leaving jsapi-tests for a followup, because it makes things more complicated than necessary for bug 1254873, which does some necessary not-so-related cleanup as well, and I'd rather have than done quickly rather than having to wait to figure out what to do about jsapi-tests.

Comment 10

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8728269 [details]
MozReview Request: Bug 1251324 - Add a --disable-js-shell option to js/src/configure

https://reviewboard.mozilla.org/r/38885/#review35691

::: js/moz.configure:7
(Diff revision 2)
> +option('--disable-js-shell', help='Do not build the JS shell')
> +
> +@depends('--disable-js-shell')

We only include this file if we're building js, but as I understand it we want to let people turn off the js shell when building the browser.

Comment 11

[Mike Hommey [:glandium]]

(In reply to Chris Manchester (:chmanchester) from comment #10)
> Comment on attachment 8728269 [details]
> MozReview Request: Bug 1251324 - Add a --disable-js-shell option to
> js/src/configure
> 
> https://reviewboard.mozilla.org/r/38885/#review35691
> 
> ::: js/moz.configure:7
> (Diff revision 2)
> > +option('--disable-js-shell', help='Do not build the JS shell')
> > +
> > +@depends('--disable-js-shell')
> 
> We only include this file if we're building js, but as I understand it we
> want to let people turn off the js shell when building the browser.

That's handled in bug 1254873

Comment 12

[Chris Manchester (limited bugmail, email directly)]

(In reply to Mike Hommey [:glandium] from comment #11)
> (In reply to Chris Manchester (:chmanchester) from comment #10)
> > Comment on attachment 8728269 [details]
> > MozReview Request: Bug 1251324 - Add a --disable-js-shell option to
> > js/src/configure
> > 
> > https://reviewboard.mozilla.org/r/38885/#review35691
> > 
> > ::: js/moz.configure:7
> > (Diff revision 2)
> > > +option('--disable-js-shell', help='Do not build the JS shell')
> > > +
> > > +@depends('--disable-js-shell')
> > 
> > We only include this file if we're building js, but as I understand it we
> > want to let people turn off the js shell when building the browser.
> 
> That's handled in bug 1254873

Ok.

Comment 13

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8728269 [details]
MozReview Request: Bug 1251324 - Add a --disable-js-shell option to js/src/configure

https://reviewboard.mozilla.org/r/38885/#review35713

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e877446c8336

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e877446c8336

Comment 16

[Boris Zbarsky [:bzbarsky]]

Could we have at least announced this or something?  I discovered the lack of a shell today the hard way, when I needed it pretty urgently, and had to ask on IRC to find out what was going on....