Closed Bug 507038 Opened 11 years ago Closed 11 years ago

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

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: dietrich, Assigned: dietrich)

Details

(Keywords: perf, Whiteboard: [ts])

Attachments

(2 files, 1 obsolete file)

Attached patch wip (obsolete) — Splinter Review
No description provided.
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.
Attached patch moreSplinter Review
Assignee: nobody → dietrich
Attachment #391222 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Whiteboard: [ts]
(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.
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.
totally agree. if this is a proven win, this should be done all over. and should be *required* for new components.
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.
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.
Wanted to make sure we are all on the same page :)
Summary: combine Places JS components into a single file at build-time → combine Toolkit JS components into a single file at build-time
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.
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.
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...
Fantastic, thanks! I'll give it a try.
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.
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.
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.
Danger Will Robinson! That script ran forever, filling up my drive w/ a 7gb file when i redirected the output to file.
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?
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.
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.
Summary: combine Toolkit JS components into a single file at build-time → combine Toolkit JS components into a single file at package-time
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.
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.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.