507038 - combine Toolkit JS components into a single file at package-time

Status: RESOLVED WONTFIX

(Toolkit :: Places, defect)

Description

[Dietrich Ayala (:dietrich)]

Attachment: wip

Comment 1

[Shawn Wilsher :sdwilsh]

I asked about doing this for all toolkit components, and all browser components in the past but was told that this was all fastloaded anyway.  This also means we cannot make changes to these files and have them reflected in the browser without rebuilding, which we can do now.

Comment 2

[Dietrich Ayala (:dietrich)]

Attachment: more

Comment 3

[Dietrich Ayala (:dietrich)]

(In reply to comment #1)
> I asked about doing this for all toolkit components, and all browser components
> in the past but was told that this was all fastloaded anyway. 

fastload reduces time of js_Compile, has nothing to do with this bug. this change results in a reduction of filesystem calls, which are extremely expensive on resource constrained devices.

> This also means
> we cannot make changes to these files and have them reflected in the browser
> without rebuilding, which we can do now.

wah. tell it to our 300 million users.

Comment 4

[Shawn Wilsher :sdwilsh]

What about doing this for all of toolkit, and for all of browser?  If it's good enough for one module, it's good enough for all of them, and I don't see the benefit of putting the effort into doing this in /every/ module.

Comment 5

[Dietrich Ayala (:dietrich)]

totally agree. if this is a proven win, this should be done all over. and should be *required* for new components.

Comment 6

[Shawn Wilsher :sdwilsh]

Just to be clear, I'm saying this bug should be morphed into "combine Toolkit JS components into a single file at build time".  Could probably do some build system magic for this too.

Comment 7

[Dietrich Ayala (:dietrich)]

I'm saying we should do one file for toolkit, and one for browser, as you mentioned above.

However, any action on this requires data proving it's a win. I'm going to see if Taras can test this patch on his WinCE device with his startup measurement code in place.

Comment 8

[Shawn Wilsher :sdwilsh]

Wanted to make sure we are all on the same page :)

Comment 9

[(not currently active) Ted Mielczarek]

If this is a win, and we want it, we could also look into a packaging-time solution, which would leave development builds alone, but get the results for end users.

Comment 10

[Dietrich Ayala (:dietrich)]

It would be great to do this at package-time only.

A problem is that things like NSGetModule() are defined in each separate file, and need to be combined into a single call for all included components. eg: see the attached patch for how i did this for places.

Maybe could update all of the XPCOMUtils.NSGetModule() callers to use something friendlier to our purpose. Worst-case, we could have them all preprocessed, and do like I did in the patch here.

Comment 11

[Florian Quèze [:florian]]

Attachment: shell script to 'link' all the js components

A few days ago, I played a bit with a shell script to 'link' all JS components together. Not sure if it can be of any help here. I'm attaching it just in case...

Comment 12

[Dietrich Ayala (:dietrich)]

Fantastic, thanks! I'll give it a try.

Comment 13

[Alex Vincent [:WeirdAl]]

Oh, I see what Florian's script does... it places each individual JS file into an anonymous function (thus avoiding scope conflicts), then adds each file's NSGetModule to a global map.  Then the global NSGetModule simply references each individual NSGetModule.  Pretty clever.

That said, if we go forward with this, I'd request this be enabled/disabled by a mozconfig flag.  It may be more efficient for the computer to have a single component file, but it's painful for a developer to debug through thousands of lines of JavaScript in a single file.  Been there, done that, don't want to do it again.

Regarding comment 8, I agree, package time is better than build time.

Comment 14

[Alex Vincent [:WeirdAl]]

It might also be worth mentioning that a single syntax error in any component file would bring all these components down.  I wonder if there's any practical way to detect a JS syntax error at build- or package-time, but that's a discussion for a separate bug.

Comment 15

[Dietrich Ayala (:dietrich)]

build config option for this sounds ok to me.

(In reply to comment #14)
> It might also be worth mentioning that a single syntax error in any component
> file would bring all these components down.  I wonder if there's any practical
> way to detect a JS syntax error at build- or package-time, but that's a
> discussion for a separate bug.

depending on the component, the app could be hugely broken regardless in this scenario whether in one big file or not. i think this would likely be caught in post-checkin unit tests anyways.

Comment 16

[Dietrich Ayala (:dietrich)]

Danger Will Robinson! That script ran forever, filling up my drive w/ a 7gb file when i redirected the output to file.

Comment 17

[Alex Vincent [:WeirdAl]]

Comment on attachment 393888 [details]
shell script to 'link' all the js components

I think it might be this:
>for i in components/*.js;
...
>    cat <<EOF
>//@line 13 "linkedComponents.js"

Is this script grabbing components/linkedComponents.js and trying to wrap that code?

Comment 18

[Florian Quèze [:florian]]

If you redirected the output to components/linkedComponents.js then yes, it's likely to recurse.

When I played with it, I redirected the output to a file in another folder, then I moved components/*.js away from the components folder and placed the linked components there.

That script was just an experiment. I was wondering if reducing the number of JS XPCOM components would significantly help to reduce startup time of Instantbird on my wince device. After linking the components, the application (Instantbird) still worked, so it looked promising, but I haven't found a correct way to time JS components registration on the device so I gave up.

If we want to go forward in the direction of using such a script by default, you may want to improve some details. A few things I've noticed:

 - In the getClassObject method, I forgot to copy these lines from the default implementation of XPCOMUtils.jsm:
159         // We only support nsIFactory queries, not nsIClassInfo
160         if (!iid.equals(Ci.nsIFactory))
161           throw Cr.NS_ERROR_NOT_IMPLEMENTED;

 - If some components rely on the value of their filename, they may be broken (I don't think any component actually has such a problem). The only usecase I've noticed for filenames is the debug messages of XPCOMUtils.jsm, for example:
173         debug("*** registering " + fileSpec.leafName + ": [ ");
If we care about this, workarounds are probably possible. However if we do the script linking thing at packaging time, as I don't think it's even possible to package a debug build, this specific issue will probably never be visible.

 - The indent of that line is wrong in the script:
    cat <<EOF

I don't know if Components.utils.import calls are expensive once the module has already been imported once. If it turns out they are, then we can import the XPCOMUtils module once at the beginning of the linked components, and tweak the script to remove any Components.utils.import("resource://gre/modules/XPCOMUtils.jsm"); or Cu.import("resource://gre/modules/XPCOMUtils.jsm"); line from the processed scripts.

Comment 19

[Robert Kaiser]

I hope this only will be done at packaging time, so that self-made builds used by developers keep the modules apart for easier development.

Comment 20

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

This bug should be WONTFIX -- the right way to get the win we want here is by fixing bug 512827.  Any combining of JS as described here will be fragile, and will only possibly help on first-run startup, as JS components are fastloaded.  There's no reason to add this complexity.

Comment 21

[Dietrich Ayala (:dietrich)]

bah, i noted as such here, after we talked about it on irc.

http://autonome.wordpress.com/2009/09/06/firefox-startup-performance-weekly-summary/

i just forgot to close the bug.