631100 - Validator: no-jars suggested always when FX4 and jar

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Developer Pages, defect, P3)

Description

[Nils Maier [:nmaier]]

The validator always suggests not to use jars when the XPI targets Firefox 4 and contains jars.
This is wrong when the XPI targets old versions of Firefox/other applications as well, as the performance improvement is actually marginal in Firefox 4, but older versions and other apps might actually get a nasty performance hit, as the XPIs will be unpacked.

Expected:
- Only suggest no-jars when all compatible apps/versions have do no unpack by default.

Comment 1

[Matt Basta [:basta]]

This fight was pretty thoroughly fought back in bug 551714 and may conflict with bug 597255. I'm not an expert on this, but perhaps someone else can shed some light.

Comment 2

[Nils Maier [:nmaier]]

It is a known fact that performance will decrease in Firefox 3 and friends if you omit the jar, for example:
- Load jar once, fetch again from OS level disk cache.
- Placement of files on the disk. Usually one jar is less fragmented than several files spread all across the disk. (The likelihood of such "fragmentation" increases with flat files)
- Fetch files over the network, e.g. nfs or smb/cifs. Fetching one jar is clearly better than requesting files one-by-one.

Many, if not most, add-ons currently listed on AMO and compatible with fx4 have a minVersion pre-fx4.

Firefox 3 is by far the dominant version right now. All those flat-packaged updates will in fact decrease the performance for millions of users (except the fx4 beta and nightly users), right now.

Comment 3

[Jorge Villalobos [:jorgev] (he/him)]

I haven't seen the language in the message the validator shows. If there's something we can change in it to make it clearer, I'm OK with it, but taking it out is not an option.

Comment 4

[Nils Maier [:nmaier]]

Language:
https://github.com/mattbasta/amo-validator/blob/master/validator/testcases/packagelayout.py#L177

The message is entirely wrong, no matter what.

Firefox 3:
As authors decide to drop the jars to get rid of the warning, the number of file stats and accesses will increase, hurting performance in all cases, even when the xul fastload caches are fully active, because XPIs are always unpacked.
And lots of authors just do that, as the warning recommends it.
Furthermore existing extension installations won't be packed when upgraded from Firefox 3 to Firefox 4.

Firefox 4:
Even jar-in-a-xpi packages in Firefox 4 will in fact load faster in a majority of cases than unpack, as the XPI is read into the memory once and the additional zipping would be less of burden (even if deflate is involved) than additional disk accesses as is the case with unpack. This is only the no-cache case, i.e. after addon installation.
In the cached case, the current code will file stat the installation to check for cache invalidation, leading to only one stat with XPIs and multiple stats when unpack.

Right now it hurts performance of the majority of users, basically all, from mildly to a damn lot.

So in conclusion:
Right now we're making the situation worse for all users, and partly reversing the work the startup/performance folks did in Firefox 4 by recommending wrong things!
unpack never is a valid solutions; At best it's a work-around to get binary components and similar working again, which won't work without unpacking.

- The language must be rephrased.
- Only recommend to drop jar files if both minVersion and maxVersion are in the Firefox 4 range.

Comment 5

[Jorge Villalobos [:jorgev] (he/him)]

I'm OK with the current message. It says that there *can* be performance issues and we *recommend* not using the JAR file.

Matt, can you change the message level to "info", please? I think showing it as a warning is a little excessive.

Comment 6

[Nils Maier [:nmaier]]

(In reply to comment #5)
> I'm OK with the current message. It says that there *can* be performance issues
> and we *recommend* not using the JAR file.

That's the point, really. The recommendation is utterly wrong!

Comment 7

[Wil Clouser [:clouserw]]

> Matt, can you change the message level to "info", please? I think showing it as
> a warning is a little excessive.

Do we have an info level?

Nils, how would you word it?

Comment 8

[Nils Maier [:nmaier]]

Something along the lines of:

The add-on contains JAR files and does not set <em:unpack> to 'true'.
If your add-on targets Firefox 4 (Beta) or later exclusively, it is recommended to no longer use JARs to package your chrome files, as JARs inside XPIs have a slightly worse performance.
If you're addon, however, also targets earlier earlier versions of Firefox or other applications that unpack XPIs during installation, then please keep the additional JAR packaging as-is, or the performance of such applications may significantly decrease.
Setting <em:unpack> to 'true' is not an alternative as it will always result in worse performance and hence we advise against it, unless it is required in order for your add-on to work correctly.


The message would be more concise if it was broken up into several messages that would be displayed depending on the actual target applications.

Comment 9

[Matt Basta [:basta]]

We do have an info level. Updated:

https://github.com/mattbasta/amo-validator/commit/07a14c94ae4d66329939ddbbf0c938f48401ae76

I also updated the wording to sound more like a recommendation. Let me know if there is anything else.

Comment 10

[Nils Maier [:nmaier]]

This misses the bits about major performance loss in Firefox 3 if you flat-package your XPIs.

At least change this:
"It is recommended that you consider no longer using JAR files to package your chrome files."
to this:
"It is recommended that you consider no longer using JAR files to package your chrome files if your add-on doesn't also support Firefox 3.6 or earlier."

Comment 11

[Jorge Villalobos [:jorgev] (he/him)]

Calling this fixed.
We want to encourage performance optimization for Firefox 4 (and other versions if possible), and if this means there will be a negative impact on older versions, so be it. The majority of add-on users will be using Firefox 4 soon enough, and we can't wait for developers to drop all older Firefox versions before we begin *suggesting* to them how to make their add-ons load faster.

Comment 12

[Nils Maier [:nmaier]]

We're hurting Firefox 4 as well, as extensions won't be repackaged during the Firefox 3 -> Firefox 4 upgrade.
And we're hurting Firefox 3 a lot while jars in Firefox 4 are negligible performance wise.

Comment 13

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to comment #12)
> We're hurting Firefox 4 as well, as extensions won't be repackaged during the
> Firefox 3 -> Firefox 4 upgrade.

They will be repackaged when the extensions are updated, which arguably should happen for most in the next quarter or so.

> And we're hurting Firefox 3 a lot while jars in Firefox 4 are negligible
> performance wise.

Like I said, the focus is Firefox 4 now. Developers can choose not to do this, and we're giving a pretty soft recommendation.

Marking as fixed, again.

Comment 14

[Nils Maier [:nmaier]]

(In reply to comment #13)
> (In reply to comment #12)
> > We're hurting Firefox 4 as well, as extensions won't be repackaged during the
> > Firefox 3 -> Firefox 4 upgrade.
> 
> They will be repackaged when the extensions are updated, which arguably should
> happen for most in the next quarter or so.

"Most" simply doesn't cut it. We're reversing a bit of the fine startup perf work the platform team and others made with every extension that is installed flat-packaged and unpacked because of our recommendation.

Furthermore you're suggesting to punish those users that make the Firefox 3 -> 4 switch late, after most add-on upgrades already happened.
- 3 month after the Firefox 3.5 release it had a Firefox share of ~50%
- 3 month after the Firefox 3.6 release it had a Firefox share of ~75%
So, not everybody is an necessarily an early adopter.

Even if we get 80%+ after the first quarter there will be still 20% of late upgraders affected. Punishing 5%-40% of the Firefox 3 *and* 4 user base with worse startup performance than it has to be because of a packaging recommendation gone wrong is clearly a WTF?! in my book.

> > And we're hurting Firefox 3 a lot while jars in Firefox 4 are negligible
> > performance wise.
> 
> Like I said, the focus is Firefox 4 now. Developers can choose not to do this,
> and we're giving a pretty soft recommendation.

The current recommendation still is unclear and dangerous with Firefox 4 in mind, and with Firefox pre-4 in mind as well. That plain and simple.

And for the "can choose not to": We're recommending to flat-package, hence the sensible thing is to follow our bad advise, not knowing any better.

> Marking as fixed, again.

Yeah, I'm not inclined to start a REOPEN war because we both are stubborn :p
Still, I had to post this comment, because for that I'm more than stubborn enough.

To quote Mike Hommey[1]:
> Disks seeks hurt. Badly.

This, of course, applies to seeking around to read a lot of small flat-packaged files, too.
And now take a moment to ponder what happens when nfs/smb comes into play...

[1] http://glandium.org/blog/?p=1665

Comment 15

[Matt Basta [:basta]]

Nils: On an unrelated note to the bug, I'm curious to see the benchmarks of the performance issues that you're basing your assertions on. I'd like to see how much of a performance hit something like this actually incurs (FX3.6 vs FX4, unpack vs no unpack, etc.)

Comment 16

[Nils Maier [:nmaier]]

(In reply to comment #15)
> Nils: On an unrelated note to the bug, I'm curious to see the benchmarks of the
> performance issues that you're basing your assertions on. I'd like to see how
> much of a performance hit something like this actually incurs (FX3.6 vs FX4,
> unpack vs no unpack, etc.)

There were a lot articles and discussion in the mozilla blogs and bugs about startup performance. I mostly don't have all the URLs and bug numbers lying around, sorry. Some examples:
- Blog post(s) about plain I/O times concerning extensions and what that means for startup times. Resulting in e.g. the fastload caches work (bug 511761), eliminating stat calls and reviving no-unpack for extensions.
E.g. http://blog.mozilla.com/tglek/2010/03/11/extensions-startup/

- Omni-jar discussion and analysis. (Includes some nice zip-format tricks Taras made for optimized loading)
E.g. http://blog.mozilla.com/tglek/2010/09/14/firefox-4-jar-jar-jar/ and follow-ups

- Preload and reordering of symbols discussion and analysis (Taras, Mike Hommey recently)
E.g. http://glandium.org/blog/?cat=25


If you want to dig deeper and get more general knowledge, I suggest you look into:
- General harddisk operation and disk latency (sequential reads are dirt cheap, seeking is not)
- Disk scheduling decisions and trade offs.
E.g. http://science.kennesaw.edu/~khoganso/CS3530/L20_OS_Ch09.pdf (money quotes on slide 33)
- File systems architectures and design decisions, OS caches (incl. MS ReadyBoost and similar)
- Always an interesting read: database systems internal on-disk designs, access semantics and I/O scheduling (always large block read/writes)

The actual performance strongly depends on the location of files ("clean" hard disk, fragmented hard disk, what type of hard disk, file systems, OS caches, application caches, files from network locations, etc), so there are no numbers that universally valid.
It's pretty difficult to benchmark even on a single system in a way that the results are valid and reproducible.

Now, inter-session fastload caches and similar work have eliminated much of the "expensive" I/O related to extensions, but some things, such as images, are not cached that way.

Comment 17

[krupa raj[:krupa]]

nmaier/jorgev,

Do you mind verifying this bug in addons-next.allizom.org? Thanks!

Comment 18

[Jorge Villalobos [:jorgev] (he/him)]

I tried with Fire.fm, but I don't see a warning or info message: https://addons-next.allizom.org/en-US/developers/upload/e802b84e0f374acfb73e6f55e74f7903
I don't see it in prod either, FWIW.

Comment 19

[Jorge Villalobos [:jorgev] (he/him)]

Reclassifying editor bugs and changing to a new whiteboard flag. Spam, spam, spam, spam...