Closed Bug 551714 Opened 11 years ago Closed 10 years ago

Validator should recommend not using JAR files when <em:unpack> is not set to true

Categories

(addons.mozilla.org Graveyard :: Admin/Editor Tools, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
5.12.3

People

(Reporter: dietrich, Assigned: basta)

References

Details

(Whiteboard: [see comment 23])

See this comment:

http://autonome.wordpress.com/2010/03/10/firefox-extensions-and-performance/#comment-708

Extensions with unpacked files are slower (see bug 533038).

When an extension is uploaded, we could unpack it, detect if the files are in a JAR, and if not, move them to a JAR and update the chrome manifest to reflect it.
I like the sentiment, but this I feel may be a bad idea. I don't think AMO should be modifying the uploads in any way for risk of breaking things. Sometimes a JAR is not even an option, as with bug 406538. I'm not sure, but there is also the potential for license-related complaints if uploads are modified.

I think the best approach is to simply change policy to not allow public approval of any file that is not already JAR packed. (unless the file count is under some small threshold where it doesn't matter) The addon validator could be modified to detect and warn of this and developers could simply be told to fix the problem themselves.
OS: Linux → All
Hardware: x86 → All
Also, JARing automatically would be a problem for signed addons.
It should be recommended everywhere (like in https://developer.mozilla.org) and a test for JAR-ed files can be added to the AMO add-ons test (providing a warning and explaining).
> It should be recommended everywhere (like in https://developer.mozilla.org)
Also all counter-recommendations on DevMo to use the un-jarred format should be removed (except during development).
updating bug summary to reflect the comments. sounds like these things need to happen:

- have the validator act on detection of non-JAR files. should it warn, or reject?

- make sure reviewers know about this

- update the guide: https://developer.mozilla.org/En/Firefox_addons_dev_guide
Summary: When an extension is uploaded, AMO should put unpacked files in JARs → recommend JARing files in all possible places
> - have the validator act on detection of non-JAR files. should it warn, or
> reject?

Warn. This is only a recommendation and a performance enhancement. I don't think it's reasonable to require it.
I think it's reasonable to have editors reject addons without JAR packing if they have over a couple dozen files or so. (after some phase in period) I would at least like to see this start with themes which can have hundreds and hundreds of files to load separately. Themes in particularly have always bugged me on this front, if just for the installer size waste.

Oh, and anything that talks about this needs to explicitly point out one thing that plenty of people miss: the JAR should not be compressed, just stored. It's counter-intuitive for some people, but compressing the JAR will actually raise the end XPI size. It's also better to load up the JAR without having to uncompress parts and there's usually no need for compression after install anyway.
(In reply to comment #5)
> updating bug summary to reflect the comments. sounds like these things need to
> happen:
> 
> - have the validator act on detection of non-JAR files. should it warn, or
> reject?
> 
> - make sure reviewers know about this
> 
> - update the guide: https://developer.mozilla.org/En/Firefox_addons_dev_guide

where should the information go within that guide, and how can we best get the word out on this?

can the verifier produce a warning, and maybe reject at some reasonable threashhold?
Jorge, can you be specific about what the validator should do here and then assign to :basta?  thanks
Assignee: nobody → jorge
This is probably WONTFIX.

The work being done for the Omnijar in bug 552121 might also include some work for add-ons that is easier to accomplish if there's no JAR file, so we may end up recommending the complete opposite of this. None of this is final, though. We should wait for all of this to land and see what's the best recommendation we can give.
See the work being done in bug 533038 which allows most extensions to be installed without unpacking any files. It would actually be more efficient for addons to not jar their chrome files in that case. We get faster startups and extension developers don't have to mess around with chrome jars. It's not currently 100% compatible with all the things extensions can do so there's an option to opt out of not unpacking.
My 2 cents feedback. Our addon has been at first self-hosted. Few monthes later we also published our addon on AMO. Thus the code has been at first developed and tested outside AMO guidelines. When we came on AMO, we took very seriously the validator and its warnings. IMHO, a developper can create an addon without reading the official guidlines, thus it seems to me that "recommending JARing files in all possible places" might be difficult and the best place would be a warning inside the validator with a link to "why should I JAR my files?" document.
Updating this bug to reflect the changes implemented in Firefox 4.
From now on, add-ons won't be extracted in the profile directory by default. They will remain packaged, and this means that it is more efficient not to use an internal JAR file.
So, this bug is now about improving our validator so that it recommends not using a JAR file, unless the <em:unpack> flag is set on install.rdf. It would also be nice to limit this flag to add-ons that support Firefox 4 and above, but it's OK not to do this if it complicates the code too much.
Assignee: jorge → mbasta
Priority: -- → P3
Summary: recommend JARing files in all possible places → Validator should recommend not using JAR files when <em:unpack> is set to true
Whiteboard: [see comment 13]
Target Milestone: --- → Q4 2010
Summary: Validator should recommend not using JAR files when <em:unpack> is set to true → Validator should recommend not using JAR files when <em:unpack> is not set to true
Using flat XPIs with no JARs increases XPI file sizes and thus bandwidth usage for updates. XPIs are just ZIPs, which do not support solid archives [1]. The sub-JAR procedure is a poor-man's solid archive. Thus, if you have an uncompressed JAR in a compressed XPI your end XPI file size will be smaller than either no JAR or even with a _compressed_ JAR. For little extensions this may be irrelevant; for others it is not. In other words, even if you have an extension that doesn't have to opt-out of extractionless mode, it still may want to use a JAR to reduce XPI file size.

Ideally, XPIs would just support better compression than crappy archaic ZIP and use LZMA or something. (bug 93126) Almost anything newer should be able to support solid archives without sub-JAR hacks.

[1] http://en.wikipedia.org/wiki/Solid_compression
Jars within xpis are less efficient to access without some care in selecting which files go in what archive, but can give you better compression. However, in practice, there's really not much difference (in time or size) unless you have an extension with a particularly large number of files. The most important part is not scattering extension files in the first place. The larger xpi size can be largely mitigated in many cases by compressed http.

In other words, I think this current plan is fine. Most people will end up compressing the inner jar anyway since that's the default for zip programs. It'll take a bit more space, though. May need to add a warning to not compress the inner jar (while continuing to compress the files within the inner jar) since it uses more memory and cpu to access.
There are two basic times when an inner JAR is really beneficial:
1) Lots of icons or similar
2) Lots of locale files

Quick test builds of Flagfox 4.1a3pre, and I have both of the above:
with JAR: 563.2 KiB
no JAR:   743.3 KiB
that's +180.1KiB, +32%

As to reason #2, this problem goes up if you have multiple dialects of the same language. For example, I have es-AR/es-ES, pt-BR/pt-PT, and zh-CN/zh-TW. These are different dialects of the same language, so their files are different, but very similar in many cases. With a solid archive you only end up storing the differences between them. Without a solid archive each new dialect is a full new load. A solid archive can even help many times between different languages. I'm up to 35 full locales now, so this is where a lot of the benefit comes in for Flagfox.

I do agree that in probably most cases the simplicity of not JARing anymore may be best to recommend (eventually, as 3.6 doesn't go away right away). I'm just saying this recommendation has to come with a footnote to say that sometimes you will see a big benefit from still using a JAR.

(In reply to comment #15)
> May need to add a warning to not compress the inner jar

Yes, I mentioned that above. (comment 7) It's an all too common mistake.

> The larger xpi size can be largely mitigated in many cases by compressed http

That's an interesting point. How much can this help? Compressing on top of something already compressed usually has low benefits, even if the outer compression is better.
(In reply to comment #16)
> > The larger xpi size can be largely mitigated in many cases by compressed http
> 
> That's an interesting point. How much can this help? Compressing on top of
> something already compressed usually has low benefits, even if the outer
> compression is better.

The central directory at the end of the file should compress very well since it's mostly strings and repeated signatures. gziping or ziping (with deflate) your extension will give you a good idea of how much http compression helps.
(In reply to comment #17)
gziping the same files in comment 16:
with JAR + gzip: 548.4 KiB
no JAR + gzip:   610.5 KiB

So it does help a fair bit in my case, though http compression will only help with the bandwidth issues, not the file size on disk and its i/o on load.

That being said, I'm sure I'm not a common case with Flagfox, as I have to opt-out of extractionless anyway to access a binary file and its metadata.
I'm working on getting this implemented in the new validator. In terms of how it will affect output, an improper JARing will result in a warning (as it doesn't break functionality).

Can someone clarify the guidelines for this test? Here's what I've gathered:
 - If an add-on is set to <em:unpack>true</em:unpack> (or is recommended to unpack, as with dictionaries and such), it should use JAR files
 - If an add-on is not set to unpack, it should continue doing whatever it was doing before extractionless add-ons were invented

Can someone confirm?
That's roughly it. Basically warn about the lack of JARs if the extension uses em:unpack.
Based on the current bug title, the intent was also to suggest not using JARs if not unpacking. In this case I don't disagree with a suggestion that it might not need the JAR anymore, but it's only a matter of providing info and not outright recommending the developer do something different if they don't need to.
Jorge: can you summarize what needs to happen here?  What we look for, what we do when we find it, etc.  What is the message to the user?
Target Milestone: Q4 2010 → 5.12.3
If an add-on doesn't have the <em:unpack> flag in install.rdf, or if it's set to false (its default value), AND the file has an internal JAR file AND the add-on supports Firefox 4, the warning must appear. The warning should say something like "To improve startup performance in Firefox it is recommended that you no longer use a JAR file to package your chrome files".
Whiteboard: [see comment 13] → [see comment 23]
(In reply to comment #23)
> something like "To improve startup performance in Firefox it is recommended
> that you no longer use a JAR file to package your chrome files".

-> "To improve startup performance in Firefox 4+ it is recommended
    that you no longer use a JAR file to package your chrome files"

Firefox 4 should be mentioned by version in this note, as it's not like Firefox 3.x support vanishes the moment it comes out. ;)
This should be taken care of in f95b910. I'll be writing tests soon; I'll close the bug once they're written.
(In reply to comment #25)
> This should be taken care of in f95b910. I'll be writing tests soon; I'll close
> the bug once they're written.

Leaving in 5.12.3 then
Tests were added in c30ce88.

https://github.com/mattbasta/amo-validator/commit/ce0ce883bdce595d5c5fc3457072f3a53b772f7a
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
(In reply to comment #27)
> Tests were added in c30ce88.
> 
> https://github.com/mattbasta/amo-validator/commit/ce0ce883bdce595d5c5fc3457072f3a53b772f7a

+def test_unpack_ff4():
+    "When FF4 is supported and unpack is true, JARs should throw errors."
+
+    assert _do_test(unpack="true",
+                    contents=("foo.jar", ),
+                    is_ff4=True).failed()

Isn't this backwards? If FF4 and unpack is true, then the only way to avoid a full extraction is to use a JAR. If unpack is false and we're on FF4 then a warning, not a full-stop error, is what we want if it sees a JAR.
(In reply to comment #28)
> Isn't this backwards?

Check out bug 597255. That test is from over there. I could be wrong; let me know if I did gum this all up.
Oh wait, cancel that. I did flub that up. I'll fix it now.
Depends on: 597255
This is now taken care of in 5868523.
I don't know what that number is. Link please?
Github is down; I'll post the link to the commit as soon as it comes back up.
Depends on: 705551
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.