Closed
Bug 468011
Opened 16 years ago
Closed 15 years ago
Combine all chrome into browser+toolkit jars
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla3.7a1
People
(Reporter: mossop, Assigned: taras.mozilla)
References
Details
(Keywords: dev-doc-complete, perf, Whiteboard: [ts][dev-doc-needed see comment 34])
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
I've been running some Ts numbers for various changes to how the chrome files are packaged and I have the following results (20 Ts runs per case, only modifying browser.jar, classic.jar, toolkit.jar and en-US.jar):
Baseline: 927.25ms
JAR files compressed: 914.95ms
No JAR files (simple directory chrome): 1038.95
All chrome in a single JAR: 909.25
All chrome in a single compressed JAR: 925.25
The numbers are slightly odd, I'm not sure why compression improves the current case and makes the single JAR case worse, but it still suggests that we could get a performance improvement out of putting all of the chrome into a single jar file.
I'm in the process of putting together an example patch to do this so I can get some try server results, but I think we also need to consider some other issues. Does This cause problems for XULRunner and localised builds f.e.?
Comment 1•16 years ago
|
||
Unifying en-US.jar with anything else is not feasible with l10n.
I hate to unify toolkit/browser/classic.jar because it makes XULRunner more painful, but we can do it if the 2% win is really worth it.
I'm surprised that going from 4 to 1 makes a measurable difference: what OS is this? is it possible we could get some comparative shark profiles or something to see where exactly we're saving/spending time?
Reporter | ||
Comment 2•16 years ago
|
||
This was on OSX so we could get something from shark I imagine. I'll probably have a patch ready tomorrow that we can play with.
Reporter | ||
Comment 3•16 years ago
|
||
(In reply to comment #1)
> Unifying en-US.jar with anything else is not feasible with l10n.
What is it that actually makes this non-feasible? Because the interesting thing is that in further tests I get no perf win if I combine all the main content and skin jars, but a measurable win if I include the locale into the same jar. I've sent some patches to tryserver to try to verify this.
Comment 4•16 years ago
|
||
As long as the repacking is smart enough to strip out the en-US content and stick in the ab-CD content, it should be fine, no?
Comment 5•16 years ago
|
||
That'd make repackaging significantly harder.
We have some different separation of l10n and non-l10n content in the installer setup, too.
Reporter | ||
Comment 6•16 years ago
|
||
This is the main patch, it combines all the non-locale jar files into application.jar
Reporter | ||
Comment 7•16 years ago
|
||
This additional patch include the locale as well
Reporter | ||
Comment 8•16 years ago
|
||
My try server tests with these patches suggest that maybe this isn't all that worth it after all. It appears that there is only a win when combining everything including the locale into a single jar, and even then it only appears to affect OSX. Since this change would complicate other things this might have to be a WONTFIX.
Comment 9•16 years ago
|
||
I'd still love to see a comparative profile... we may just be doing something stupid.
Reporter | ||
Comment 10•16 years ago
|
||
It seems that this is not necessarily going to be worthwhile, and I don't really have the sharking expertise to figure out just where the wins are either. If someone else wants to dig in then the patch is there but otherwise I don't have the time for this right now.
Assignee: dtownsend → nobody
Comment 11•15 years ago
|
||
(In reply to comment #8)
> My try server tests with these patches suggest that maybe this isn't all that
> worth it after all.
We should check it into the Places branch temporarily to get perf numbers over a few runs.
Whiteboard: [ts]
Assignee | ||
Comment 12•15 years ago
|
||
This patch combines all of the non-locale jars firefox reads at startup into all.jar .
This is a better approach than combing all jars because there is some cost to reading jar indexes and some memory overhead to keeping them in memory.
Assignee: nobody → tglek
Attachment #392755 -
Flags: review?(benjamin)
Comment 13•15 years ago
|
||
Comment on attachment 392755 [details] [diff] [review]
combine (toolkit|classic|reporter|browser).jar into all.jar
As a first step I'd prefer to end up with browser.jar (Firefox chrome) and toolkit.jar (all the XULRunner stuff).
Also if you remove a JAR file (and it's associated manifest) you're going to need to add stuff to removed-files so that when we do updates the old files aren't left behind.
Attachment #392755 -
Flags: review?(benjamin) → review-
Assignee | ||
Comment 14•15 years ago
|
||
(In reply to comment #13)
> (From update of attachment 392755 [details] [diff] [review])
> As a first step I'd prefer to end up with browser.jar (Firefox chrome) and
> toolkit.jar (all the XULRunner stuff).
why?
>
> Also if you remove a JAR file (and it's associated manifest) you're going to
> need to add stuff to removed-files so that when we do updates the old files
> aren't left behind.
Didn't know about that, thanks.
Comment 15•15 years ago
|
||
I'm worried about Linux distros and the separate-XULRunner config and I'd like to see where we are after mmap-JAR and four-JARs land.
Comment 16•15 years ago
|
||
(In reply to comment #15)
> I'm worried about Linux distros and the separate-XULRunner config and I'd like
> to see where we are after mmap-JAR and four-JARs land.
I second this plan
Assignee | ||
Comment 17•15 years ago
|
||
(In reply to comment #16)
> (In reply to comment #15)
> > I'm worried about Linux distros and the separate-XULRunner config and I'd like
> > to see where we are after mmap-JAR and four-JARs land.
>
> I second this plan
I am trying to figure out why this is a problem. We already ship jars that have the same name in xulrunner configs, why would this particular case cause a problem?
Comment 18•15 years ago
|
||
Because if 4 JARs provides the necessary speed it's a lot better solution than mixing up the different things together.
Assignee | ||
Comment 19•15 years ago
|
||
(In reply to comment #18)
> Because if 4 JARs provides the necessary speed it's a lot better solution than
> mixing up the different things together.
I'm trying to understand the benefits of treating them as "different things"
Comment 20•15 years ago
|
||
(In reply to comment #19)
> (In reply to comment #18)
> > Because if 4 JARs provides the necessary speed it's a lot better solution than
> > mixing up the different things together.
>
> I'm trying to understand the benefits of treating them as "different things"
An application that uses XULRunner as a runtime can't put it's JARs in the XULRunner JARs. But we still want XULRunner as a runtime to be as fast as possible, so combining the XULRunner specific JARs is a good thing.
Firefox on Linux is a XULRunner app. Keeping the browser JAR out of XULRunner JARs seems like a good idea there.
(that's my angle on it anyway)
Assignee | ||
Comment 21•15 years ago
|
||
broke up my all.jar into 2 jars. I still disapprove of keeping these files separate.
Doesn't current en-US.jar contain both xulrunner + browser locales?
Attachment #392755 -
Attachment is obsolete: true
Attachment #392834 -
Flags: review?(benjamin)
Updated•15 years ago
|
Attachment #392834 -
Flags: review?(benjamin) → review+
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 22•15 years ago
|
||
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 23•15 years ago
|
||
had to back this out to fix some unit tests
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 24•15 years ago
|
||
Firefox is not the only consumer of toolkit.
> --- a/extensions/reporter/jar.mn
> +++ b/extensions/reporter/jar.mn
> @@ -1,4 +1,4 @@
> -reporter.jar:
> +browser.jar:
SeaMonkey does not have a browser.jar. Neither does Thunderbird, Sunbird, etc.
...global changes...
> -classic.jar:
> +toolkit.jar:
How will this affect SeaMonkey and Thunderbird?
Updated•15 years ago
|
Blocks: CcMcBuildIssues
Comment 25•15 years ago
|
||
(In reply to comment #24)
> ...global changes...
> > -classic.jar:
> > +toolkit.jar:
>
> How will this affect SeaMonkey and Thunderbird?
From the looks of it, we'll be able be able to take it without issue. Although that would be assuming we don't try and replace anything in the existing classic.jar.
However, the bigger issue I see here is for theme authors. I know we're driving personas more, but this will totally break the existing mechanism of taking classic.jar and unpacking it to do your changes. Now theme authors will have to unpack n * .jar and get just the relevant bit.
Here's the existing documentation:
https://developer.mozilla.org/en/Creating_a_Skin_for_Firefox/Getting_Started
If this change is continued, then that needs to be updated as well.
Comment 26•15 years ago
|
||
> Now theme authors will have to
> unpack n * .jar and get just the relevant bit.
Can this be #IFDEFed Fennec only? I can see a lot of annoyed theme authors.
Comment 27•15 years ago
|
||
No, this should not be fennec-only. I'm sorry we're making it slightly more painful for theme authors but the advantage to startup time (and reduced number of files in general) is worth it.
Assignee | ||
Updated•15 years ago
|
Summary: Combine all chrome into a single jar file → Combine all chrome into browser+toolkit jars
Assignee | ||
Comment 28•15 years ago
|
||
looks like there was only 1 testcase referring to classic.jar.
The only other issue is that ext manager refered to classic.jar, remaning which is likely to bust stuff in thunderbird. Mossop says he'll post a proper fix for that soon.
Attachment #392834 -
Attachment is obsolete: true
Attachment #393231 -
Flags: review?(benjamin)
Comment 29•15 years ago
|
||
Can't we also finally get rid of comm.jar?
It only contains 4 files:
toolkit/components/cookie/content/cookieAcceptDialog.xul and
toolkit/components/cookie/content/cookieAcceptDialog.js are packaged from
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/cookie/jar.mn,
layout/style/xbl-marquee/xbl-marquee.xml and
layout/style/xbl-marquee/xbl-marquee.css are packaged from
http://mxr.mozilla.org/mozilla-central/source/layout/style/xbl-marquee/jar.mn.
Especially the cookieAcceptDialog files should be packaged into toolkit.jar, since the files already live in toolkit/.
The xbl-marquee should be packed into toolkit.jar as well, just like the files from docshell/ or content/:
http://mxr.mozilla.org/mozilla-central/source/docshell/resources/content/jar.mn
http://mxr.mozilla.org/mozilla-central/source/content/xbl/builtin/win/jar.mn
http://mxr.mozilla.org/mozilla-central/source/content/xml/document/resources/jar.mn
Assignee | ||
Comment 30•15 years ago
|
||
(In reply to comment #29)
> Can't we also finally get rid of comm.jar?
> It only contains 4 files:
I'd be happy to, right now I'm trying to minimize number of jars read on startup. Lets do comm.jar(and others?) in a followup bug.
Comment 31•15 years ago
|
||
(In reply to comment #30)
>> Can't we also finally get rid of comm.jar?
>> It only contains 4 files:
>
> I'd be happy to, right now I'm trying to minimize number of jars read on
> startup. Lets do comm.jar(and others?) in a followup bug.
No need. You can use bug 221602 and finally put it to rest.
Updated•15 years ago
|
Attachment #393231 -
Flags: review?(benjamin) → review+
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 32•15 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED
Updated•15 years ago
|
Keywords: checkin-needed
Updated•15 years ago
|
No longer blocks: CcMcBuildIssues
Updated•15 years ago
|
Attachment #393231 -
Attachment description: cleanup references to classic/reporter.jar in the tree → cleanup references to classic/reporter.jar in the tree
[Checkin: Comment 32]
Updated•15 years ago
|
Attachment #352719 -
Attachment is obsolete: true
Updated•15 years ago
|
Attachment #352720 -
Attachment is obsolete: true
Updated•15 years ago
|
Flags: in-testsuite+
Target Milestone: --- → Firefox 3.7a1
Comment 33•15 years ago
|
||
(In reply to comment #24)
> > +++ b/extensions/reporter/jar.mn
> > -reporter.jar:
> > +browser.jar:
>
> SeaMonkey does not have a browser.jar. Neither does Thunderbird, Sunbird, etc.
I filed bug 512064.
Comment 34•15 years ago
|
||
Adding dev-doc-needed keyword - the documents for creating themes should be updated with the fact that theme authors now need to extract toolkit.jar and another .jar (browser.jar on Firefox, classic.jar on comm-central apps) and munge the two skin directories together. Or something like that.
So that's starting with this page and possibly others:
https://developer.mozilla.org/en/Creating_a_Skin_for_Firefox/Getting_Started
Keywords: dev-doc-needed
Whiteboard: [ts] → [ts][dev-doc-needed see comment 34]
Comment 35•15 years ago
|
||
Is this going into 1.9.2? Looks like 1.9.3 from what I see.
Updated•15 years ago
|
Whiteboard: [ts][dev-doc-needed see comment 34] → [ts][dev-doc-needed see comment 34][doc-waiting-1.9.3]
Updated•15 years ago
|
Flags: wanted-firefox3.6?
Comment 36•14 years ago
|
||
For docs, this needs to mention the issues raised in bug 595473; namely, that some zip utilities don't like the file, and certain anti-virus apps raise false positives on omni.jar.
Updated•14 years ago
|
Flags: wanted-firefox3.6?
Updated•14 years ago
|
Whiteboard: [ts][dev-doc-needed see comment 34][doc-waiting-1.9.3] → [ts][dev-doc-needed see comment 34]
Comment 37•14 years ago
|
||
Initial article about these changes in particular:
https://developer.mozilla.org/en/Theme_changes_in_Firefox_4
There's also a link to this and a brief summary of the situation added in the article linked in comment 34.
If there are details that are needed that I don't have, let me know.
Keywords: dev-doc-needed → dev-doc-complete
Assignee | ||
Comment 38•14 years ago
|
||
(In reply to comment #37)
> Initial article about these changes in particular:
>
> https://developer.mozilla.org/en/Theme_changes_in_Firefox_4
>
> There's also a link to this and a brief summary of the situation added in the
> article linked in comment 34.
>
> If there are details that are needed that I don't have, let me know.
Near the antiviral note mention that omni.jar is optimized for startup so certain zip programs no longer work with it.
https://bugzilla.mozilla.org/show_bug.cgi?id=605524#c2
Updated•6 years ago
|
Component: Build Config → General
Product: Firefox → Firefox Build System
Updated•6 years ago
|
Keywords: dev-doc-complete,
perf
Target Milestone: Firefox 3.7a1 → mozilla3.7a1
Updated•6 years ago
|
Keywords: dev-doc-complete,
perf
You need to log in
before you can comment on or make changes to this bug.
Description
•