Closed Bug 1457321 Opened Last year Closed Last year

Move bundled dictionaries into omni.ja

Categories

(Core :: Spelling checker, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

(Blocks 1 open bug)

Details

(Keywords: perf, Whiteboard: [qf:p1:f64][fxperf:p2])

Attachments

(4 files)

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.
Keywords: perf
Whiteboard: [qf]
Whiteboard: [qf] → [qf][fxperf]
Whiteboard: [qf][fxperf] → [qf][fxperf:p2]
Whiteboard: [qf][fxperf:p2] → [qf:f64][qf:p1][fxperf:p2]
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)
Attachment #8971740 - Flags: review?(rhelmer) → review+
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.
Attachment #8971740 - Flags: review?(ted) → review+
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. :-/
Attachment #8971741 - Flags: review?(ted) → review+
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...
Attachment #8971742 - Flags: review?(ehsan) → review+
(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.
(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.
Priority: -- → P3
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.

:(
Attachment #8971742 - Flags: review?(aswan) → review+
Attachment #8973941 - Flags: review?(gandalf) → review?(l10n)
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 :)
Landing the first part now since it's blocking bug 1453691.
Keywords: leave-open
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 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.
Attachment #8973941 - Flags: review?(l10n)
Attachment #8973941 - Flags: review?(ted)
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.
Blocks: 1460716
Depends on: 1460748
(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.
(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.
No longer depends on: 1460748
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.
Attachment #8973941 - Flags: review?(ted) → review+
Duplicate of this bug: 1440789
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
Whiteboard: [qf:f64][qf:p1][fxperf:p2] → [qf:p1:f64][fxperf:p2]
Status: NEW → RESOLVED
Closed: Last year
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Depends on: 1496995
You need to log in before you can comment on or make changes to this bug.