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

RESOLVED FIXED in Firefox 48

Status

()

Core
JavaScript Engine
--
enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: RyanVM, Assigned: glandium)

Tracking

Trunk
mozilla48
Points:
---

Firefox Tracking Flags

(firefox47 affected, firefox48 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

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

Comment 2

2 years ago
Created attachment 8723922 [details] [diff] [review]
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
Attachment #8723922 - Flags: review?(mh+mozilla)
(Assignee)

Comment 3

2 years ago
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.
Flags: needinfo?(gps)

Comment 4

2 years ago
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.
Flags: needinfo?(gps)
(Reporter)

Comment 5

2 years ago
Can we take this patch for now and build off it in another bug?
(Assignee)

Comment 6

2 years ago
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.
Attachment #8723922 - Flags: review?(mh+mozilla)
(Assignee)

Comment 7

2 years ago
Created attachment 8728269 [details]
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/
Attachment #8728269 - Flags: review?(cmanchester)
(Assignee)

Updated

2 years ago
Blocks: 1254873
(Assignee)

Comment 8

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

Comment 9

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

Updated

2 years ago
Attachment #8723922 - Attachment is obsolete: true
(Reporter)

Updated

2 years ago
Assignee: ryanvm → mh+mozilla
Attachment #8728269 - Flags: review?(cmanchester)
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.
(Assignee)

Comment 11

2 years ago
(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
(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 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
Attachment #8728269 - Flags: review+

Comment 14

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

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e877446c8336
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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....
You need to log in before you can comment on or make changes to this bug.