468011 - Combine all chrome into browser+toolkit jars

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Dave Townsend [:mossop]]

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

[Benjamin Smedberg]

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?

Comment 2

[Dave Townsend [:mossop]]

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.

Comment 3

[Dave Townsend [:mossop]]

(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

[(not currently active) Ted Mielczarek]

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

[Axel Hecht [:Pike]]

That'd make repackaging significantly harder.

We have some different separation of l10n and non-l10n content in the installer setup, too.

Comment 6

[Dave Townsend [:mossop]]

Attachment: patch 1 - combine all non-locale jar files

This is the main patch, it combines all the non-locale jar files into application.jar

Comment 7

[Dave Townsend [:mossop]]

Attachment: patch 2 - add locale into application.jar

This additional patch include the locale as well

Comment 8

[Dave Townsend [:mossop]]

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

[Benjamin Smedberg]

I'd still love to see a comparative profile... we may just be doing something stupid.

Comment 10

[Dave Townsend [:mossop]]

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.

Comment 11

[Dietrich Ayala (:dietrich)]

(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.

Comment 12

[(dormant account)]

Attachment: combine (toolkit|classic|reporter|browser).jar into all.jar

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.

Comment 13

[Benjamin Smedberg]

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.

Comment 14

[(dormant account)]

(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

[Benjamin Smedberg]

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

[Mark Finkle (:mfinkle) (use needinfo?)]

(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

Comment 17

[(dormant account)]

(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

[Benjamin Smedberg]

Because if 4 JARs provides the necessary speed it's a lot better solution than mixing up the different things together.

Comment 19

[(dormant account)]

(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

[Mark Finkle (:mfinkle) (use needinfo?)]

(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)

Comment 21

[(dormant account)]

Attachment: toolkit + chrome jars

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?

Comment 22

[(dormant account)]

http://hg.mozilla.org/mozilla-central/rev/e6d6596ef49a

Comment 23

[(dormant account)]

had to back this out to fix some unit tests

Comment 24

[Philip Chee]

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?

Comment 25

[Mark Banner (:standard8)]

(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

[Philip Chee]

> 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

[Benjamin Smedberg]

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.

Comment 28

[(dormant account)]

Attachment: cleanup references to classic/reporter.jar in the tree [Checkin: Comment 32]

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.

Comment 29

[Steffen Wilberg]

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

Comment 30

[(dormant account)]

(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

[Simon Paquet [:sipaq]]

(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.

Comment 32

[(dormant account)]

http://hg.mozilla.org/mozilla-central/rev/8f7892be6141

Comment 33

[Serge Gautherie (:sgautherie)]

(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

[Mark Banner (:standard8)]

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

Comment 35

[Eric Shepherd [:sheppy]]

Is this going into 1.9.2? Looks like 1.9.3 from what I see.

Comment 36

[Eric Shepherd [:sheppy]]

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.

Comment 37

[Eric Shepherd [:sheppy]]

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.

Comment 38

[(dormant account)]

(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