1457321 - Move bundled dictionaries into omni.ja

Status: RESOLVED FIXED

(Core :: Spelling checker, enhancement, P3)

Description

[Kris Maglione [:kmag]]

Once bug 1410214 lands, we'll be able to load dictionaries from arbitrary jar URIs. This means we'll be able to load directly from omni.ja or langpack XPIs, rather than requiring them to be separate, unpacked files.

This should be a pretty significant performance win.

The directory scan and file stat operations when loading dictionaries are expensive. So is opening extra plain files, compared to opening omnijar entries. If we bundle them in the omnijar, with a JSON manifest of available dictionaries and their locales, we can avoid most of the additional IO.

It also gives us the potential to store dictionaries compressed. Since most dictionary should compress fairly (en-US.dic gzips to 34% of its original size), that could potentially save about a half a meg of startup IO.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1457321: Part 1 - Add bundled dictionaries to built_in_addons.json.

Review commit: https://reviewboard.mozilla.org/r/240506/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/240506/

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1457321: Part 2 - Add dictionaries to omnijar.

Review commit: https://reviewboard.mozilla.org/r/240508/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/240508/

Comment 3

[Kris Maglione [:kmag]]

Attachment: Bug 1457321: Part 3 - Load built-in dictionaries from omnijar rather than scanning directories.

Review commit: https://reviewboard.mozilla.org/r/240510/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/240510/

Comment 4

[Robert Helmer [:rhelmer]]

Comment on attachment 8971740 [details]
Bug 1457321: Part 1 - Add bundled dictionaries to built_in_addons.json.

https://reviewboard.mozilla.org/r/240506/#review247060

Nice cleanup, thanks!

Quick question though - if dictionaries are moving into the omni jar, why is this file needed? I was thinking that we'd only need to read the index of the JAR (or read the directory in the case of local unpackaged builds)

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 8971740 [details]
Bug 1457321: Part 1 - Add bundled dictionaries to built_in_addons.json.

https://reviewboard.mozilla.org/r/240506/#review247368

I forgot we had added this in a Makefile rule. :-/ You're not making things worse, though (in fact you've cleaned it up a bit) so that's not on you to fix. I filed bug 1459004 to move that rule into moz.build.

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 8971741 [details]
Bug 1457321: Part 2 - Add dictionaries to omnijar.

https://reviewboard.mozilla.org/r/240508/#review247372

::: python/mozbuild/mozpack/packager/formats.py:343
(Diff revision 1)
>              'modules',
> +            'dictionaries',
>              'greprefs.js',
>              'hyphenation',
>              'localization',
>              'update.locale',

I think you should make sure that l10n repacks do the right thing here, but given that we already have update.locale in this list it ought to be OK.

This is quite the hidden bit of code, I had no idea it existed. :-/

Comment 7

[(no longer active)]

Comment on attachment 8971742 [details]
Bug 1457321: Part 3 - Load built-in dictionaries from omnijar rather than scanning directories.

https://reviewboard.mozilla.org/r/240510/#review247674

Sweet!

r=me on the spellchecker bits.  I also skimmed the rest and everything looked good, but I can't claim to understand all of of the XPIProvider bits...

Comment 8

[Kris Maglione [:kmag]]

(In reply to Robert Helmer [:rhelmer] from comment #4)
> Quick question though - if dictionaries are moving into the omni jar, why is
> this file needed? I was thinking that we'd only need to read the index of
> the JAR (or read the directory in the case of local unpackaged builds)

We can. And I considered it. But there are a few reasons I didn't want to:

1) It's pretty expensive. For jar archives, the only way to list the contents of a directory is to iterate over every entry in the index and pattern match it. There are hundreds of entries in omni jar, so that could get nontrivially expensive.
2) It requires separate code paths for packed and unpacked builds, which I'd like to avoid. We already have some helpers for simplifying that with extensions, but it still adds complexity, and increases the chances of developer builds behaving differently from release/automation builds. I've lost track of the number of times something has worked for me locally and then failed when running packaged on automation.
3) We already have this file, and already load it at startup, so it makes sense to use it. In the future, it might even make sense to expand the data that we add to it, if that means we can reduce the amount of computation we do at startup.

Comment 9

[Kris Maglione [:kmag]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #6)
> Comment on attachment 8971741 [details]
> Bug 1457321: Part 2 - Add dictionaries to omnijar.
> 
> https://reviewboard.mozilla.org/r/240508/#review247372
> 
> ::: python/mozbuild/mozpack/packager/formats.py:343
> (Diff revision 1)
> >              'modules',
> > +            'dictionaries',
> >              'greprefs.js',
> >              'hyphenation',
> >              'localization',
> >              'update.locale',
> 
> I think you should make sure that l10n repacks do the right thing here, but
> given that we already have update.locale in this list it ought to be OK.

I'm not super familiar with the l10n repack process, so I'll double check. It looks like we may have to manually update the dictionary list in this file during the repack. If so, I'll add a follow-up for that.

Fortunately, I think the multi-lingual Firefox work gandalf is doing will make this unnecessary in the future.

> This is quite the hidden bit of code, I had no idea it existed. :-/

Neither did I. It wasn't easy to find, either.

Comment 10

[Kris Maglione [:kmag]]

Attachment: Bug 1457321: Part 4 - Update built-in add-ons manifst during l10n repack.

This is slightly ugly, but is unfortunately necessary due to do the nature of
l10n repacks. Hopefully this can go away once we move to bundling lancpack
add-ons rather than repacking in the future.

Review commit: https://reviewboard.mozilla.org/r/242272/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/242272/

Comment 11

[Andrew Swan [:aswan]]

Comment on attachment 8971742 [details]
Bug 1457321: Part 3 - Load built-in dictionaries from omnijar rather than scanning directories.

https://reviewboard.mozilla.org/r/240510/#review248376

It would be nice to have all the dictionary logic in one place rather than split between Extension.jsm and XPIProvider.jsm...

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1447
(Diff revision 1)
> +   *        An object containing a property with a dictionary language
> +   *        code and a nsIURI value for each dictionary to be
> +   *        unregistered.
> +   */
> +  unregisterDictionaries(aDicts) {
> +    let origDict = spellCheck.dictionary;

This confused me on first reading, can we name it something like `currentLanguage` to be a little clearer?

::: toolkit/mozapps/extensions/test/xpcshell/test_dictionary.js:10
(Diff revision 1)
>  // This verifies that bootstrappable add-ons can be used without restarts.
>  ChromeUtils.import("resource://gre/modules/Services.jsm");
>  
>  Cu.importGlobalProperties(["XMLHttpRequest"]);
>  
> +// Our stub hunspell engine makes things a bit flaky.

:(

Comment 12

[Zibi Braniecki [:zbraniecki][:gandalf]]

Pike - I think you have a clearer vision of how you'd like the repacks to work, so down your alley. If you don't have an opinion, I can take the review back :)

Comment 13

[Kris Maglione [:kmag]]

Landing the first part now since it's blocking bug 1453691.

Comment 14

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe5d46cd794bfb3e8974d8213cb3fa752902ef3b
Bug 1457321: Part 1 - Add bundled dictionaries to built_in_addons.json. r=ted,rhelmer

Comment 15

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/fe5d46cd794b

Comment 16

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/fe5d46cd794b

Comment 17

[Axel Hecht [:Pike]]

Comment on attachment 8973941 [details]
Bug 1457321: Part 4 - Update built-in add-ons manifst during l10n repack.

https://reviewboard.mozilla.org/r/242272/#review248638

TBH, I have no idea what this code does, and how it blends in with the rest.

What I do know is that I can trigger the right things on try, and those built a German Firefox w/out a dictionary and a Dutch one with, in omni.ja and neither with the en-US dict. Both as expected.

On the implementation of that, I think this should be reviewed by a build peer. Sorry for adding yet another layer of redirection.

Consider my comment an f+ on the patch queue.

Comment 18

[Nick Alexander :nalexander [he/him]]

I think this is not playing well with mobile/android artifact builds (probably, non-browser/ artifact builds, but that's just Fennec these days): https://bugzilla.mozilla.org/show_bug.cgi?id=1460716.

Comment 19

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #18)
> I think this is not playing well with mobile/android artifact builds
> (probably, non-browser/ artifact builds, but that's just Fennec these days):
> https://bugzilla.mozilla.org/show_bug.cgi?id=1460716.

Further still, there was non-trivial work to try to get this working for mobile/android that ended up getting backed out, for reasons that look similar to what gbrown is witnessing: https://bugzilla.mozilla.org/show_bug.cgi?id=1440789.

Comment 20

[Kris Maglione [:kmag]]

(In reply to Nick Alexander :nalexander from comment #18)
> I think this is not playing well with mobile/android artifact builds
> (probably, non-browser/ artifact builds, but that's just Fennec these days):
> https://bugzilla.mozilla.org/show_bug.cgi?id=1460716.

It sounds like artifact builds may not be creating dictionaries correctly. Although it's possible that it's just creating them too late.

(In reply to Nick Alexander :nalexander from comment #19)
> Further still, there was non-trivial work to try to get this working for
> mobile/android that ended up getting backed out, for reasons that look
> similar to what gbrown is witnessing:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1440789.

Well, at the moment, this is needed for bug 1453691, which is why I landed it in the first place, so I don't think we have any choice but to address it now.

Comment 21

[(not currently active) Ted Mielczarek]

Comment on attachment 8973941 [details]
Bug 1457321: Part 4 - Update built-in add-ons manifst during l10n repack.

https://reviewboard.mozilla.org/r/242272/#review251478

I don't love this, but l10n repacks have a lot of ugly bits currently and it's hard to find nice solutions with them.

Comment 23

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d31a5556d42a6c9314f51fd115a0cb5531433bac
Bug 1457321: Part 2 - Add dictionaries to omnijar. r=ted

https://hg.mozilla.org/integration/mozilla-inbound/rev/8810007550b1fdeeae0f5d797c4a511818a57be7
Bug 1457321: Part 3 - Load built-in dictionaries from omnijar rather than scanning directories. r=aswan,ehsan

https://hg.mozilla.org/integration/mozilla-inbound/rev/43a358272d5e8ac3080e2afb192cfde755dc4e56
Bug 1457321: Part 4 - Update built-in add-ons manifst during l10n repack. r=ted f=pike

Comment 24

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/d31a5556d42a
https://hg.mozilla.org/mozilla-central/rev/8810007550b1
https://hg.mozilla.org/mozilla-central/rev/43a358272d5e