Closed Bug 468011 Opened 11 years ago Closed 11 years ago

Combine all chrome into browser+toolkit jars

Categories

(Firefox Build System :: General, defect)

defect
Not set

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)

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.?
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?
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.
(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.
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?
That'd make repackaging significantly harder.

We have some different separation of l10n and non-l10n content in the installer setup, too.
This is the main patch, it combines all the non-locale jar files into application.jar
This additional patch include the locale as well
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.
I'd still love to see a comparative profile... we may just be doing something stupid.
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
Blocks: 459117
(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]
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 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-
(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.
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.
(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
(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?
Because if 4 JARs provides the necessary speed it's a lot better solution than mixing up the different things together.
(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"
(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)
Attached patch toolkit + chrome jars (obsolete) — Splinter Review
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)
Attachment #392834 - Flags: review?(benjamin) → review+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/e6d6596ef49a
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
had to back this out to fix some unit tests
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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?
(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.
> 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.
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.
Summary: Combine all chrome into a single jar file → Combine all chrome into browser+toolkit jars
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)
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
(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.
Depends on: 509194
(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.
Attachment #393231 - Flags: review?(benjamin) → review+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/8f7892be6141
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Depends on: 512064
No longer blocks: CcMcBuildIssues
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]
Attachment #352719 - Attachment is obsolete: true
Attachment #352720 - Attachment is obsolete: true
Flags: in-testsuite+
Target Milestone: --- → Firefox 3.7a1
(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.
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]
Depends on: 515862
Is this going into 1.9.2? Looks like 1.9.3 from what I see.
Whiteboard: [ts][dev-doc-needed see comment 34] → [ts][dev-doc-needed see comment 34][doc-waiting-1.9.3]
Blocks: 519121
No longer blocks: 519121
Flags: wanted-firefox3.6?
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.
Flags: wanted-firefox3.6?
Whiteboard: [ts][dev-doc-needed see comment 34][doc-waiting-1.9.3] → [ts][dev-doc-needed see comment 34]
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.
(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
Component: Build Config → General
Product: Firefox → Firefox Build System
Target Milestone: Firefox 3.7a1 → mozilla3.7a1
You need to log in before you can comment on or make changes to this bug.