496217 - delay initialization of spellcheck dict until actually needed

Status: RESOLVED WORKSFORME

(Core :: Spelling checker, defect, P3)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

On a cold startup, initializing spell check (and reading the dictionary files) takes 100ms+.  There should be no reason to do this on startup.  In an ideal world, we would delay until we actually needed to spell check, and would then do the initialization (dict reading) on a thread to avoid getting in the way of the user.

Comment 1

[Dietrich Ayala (:dietrich)]

taras confirmed this is costly (70ms) -> P1.

Comment 2

[Dietrich Ayala (:dietrich)]

should delay initialization, and move the dict files into a jar.

Comment 3

[Axel Hecht [:Pike]]

Any changes to dictionary package impact l10n repacks, and other variants of builds we do, fwiw.

Comment 4

[Ryan Flint [:rflint] (ping via IRC for reviews)]

The only time we load dictionaries on startup is if you're restoring a session that contains <textarea>s or other inputs with spell checking enabled. I've messed around with a few potential solutions like not initializing spell check until the editor is focused, but most I've tried so far don't seem so great in terms of UX and UI responsiveness.

I think the ideal solution would be to have a way for session restore to mark docshells as being in the background so we can lazily initialize spell check or anything else we'd potentially encounter while restoring a session.

Speeding up dictionary loading and/or offloading it to separate thread are certainly things we can do, but will come at the cost of maintaining our own Hunspell, which without active ownership of this component seems like a losing proposition (Hunspell itself doesn't look like it's had much activity since last year, so maybe we're already headed in that direction).
If do wind up making changes to our in-tree Hunspell, chromium has a binary format for their dictionaries and affix files (bug 468779) which we may want to consider using. The version of Hunspell we have in-tree appears to support some sort of zipped format which might also be an option. I'm not sure JARing the files would win us as much as either of those and might actually wind up being detrimental, since I doubt the vast majority of our users are hitting the init path during startup.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Are you guys aware of bug 468779, some Chromium changes to speed up spellcheck initialization?

Comment 6

[Mike Conley (:mconley) (:⚙️)]

We should check to see if this is still a relevant bug.

Comment 7

[Florian Quèze [:florian]]

I don't remember ever seeing this in my startup profiles, so I doubt this is still done during startup. I don't know if this is done on the main thread or with async I/O, but I hope the later (in which case we could just resolve this as WFM).

Comment 8

[:Gijs (out for now; he/him)]

(In reply to Florian Quèze [:florian] from comment #7)

I don't remember ever seeing this in my startup profiles, so I doubt this is
still done during startup. I don't know if this is done on the main thread
or with async I/O, but I hope the later (in which case we could just resolve
this as WFM).

At least the personal dictionary loading was moved off the mainthread in bug 880864.

... however, it seems we wait for it in profile-do-change: https://searchfox.org/mozilla-central/rev/47aea2f603cc18144afcedbd604a418f11e90f9b/extensions/spellcheck/src/mozPersonalDictionary.cpp#422-427 . However however, it doesn't look like we hit that in practice, because Init() gets called too late for that, so the observer is not registered in time. I'm not sure why there does appear to be coverage for that code - perhaps from xpcshell or other tests that force that codepath to be hit.

We initialize spellcheck later on startup from this JS stack:

0 registerBuiltinDictionaries() ["resource://gre/modules/addons/XPIProvider.jsm":2447:22]
1 startup(aAppChanged = "undefined", aOldAppVersion = "null", aOldPlatformVersion = """") ["resource://gre/modules/addons/XPIProvider.jsm":2513:11]
2 callProvider(aProvider = "[object Object]", aMethod = ""startup"", aDefault = "null", aArgs = ",,", "null", """") ["resource://gre/modules/AddonManager.jsm":246:30]
3 _startProvider(aProvider = "[object Object]", aAppChanged = "undefined", aOldAppVersion = "null", aOldPlatformVersion = """") ["resource://gre/modules/AddonManager.jsm":554:16]
4 startup() ["resource://gre/modules/AddonManager.jsm":760:13]
5 startup() ["resource://gre/modules/AddonManager.jsm":3678:25]
6 observe(aSubject = "null", aTopic = ""addons-startup"", aData = "null") ["resource://gre/modules/addonManager.js":75:28]

via this bit of C++:

#01: mozPersonalDictionary::Init()[/toolkit/library/build/XUL +0x3f8a0a4]
#02: mozilla::xpcom::CreateInstanceImpl(mozilla::xpcom::ModuleID, nsID const&, void**)[/toolkit/library/build/XUL +0x229bac]
#03: nsComponentManagerImpl::GetServiceLocked(mozilla::Maybe<mozilla::detail::BaseMonitorAutoLock<mozilla::Monitor> >&, (anonymous namespace)::EntryWrapper&, nsID const&, void**)[/toolkit/library/build/XUL +0x233480]
#04: nsComponentManagerImpl::GetServiceByContractID(char const*, nsID const&, void**)[/toolkit/library/build/XUL +0x233c2c]
#05: nsGetServiceByContractID::operator()(nsID const&, void**) const[/toolkit/library/build/XUL +0x235be0]
#06: nsCOMPtr_base::assign_from_gs_contractid(nsGetServiceByContractID, nsID const&)[/toolkit/library/build/XUL +0x1b2b14]
#07: mozSpellChecker::Init()[/toolkit/library/build/XUL +0x3f8b4d4]
#08: mozSpellChecker::Create()[/toolkit/library/build/XUL +0x2a197c8]
#09: mozilla::dom::ContentParent::NotifyUpdatedDictionaries()[/toolkit/library/build/XUL +0x2a206e4]
#10: mozHunspell::DictionariesChanged(bool)[/toolkit/library/build/XUL +0x3f661b0]
#11: mozHunspell::AddDictionary(nsTSubstring<char16_t> const&, nsIURI*)[/toolkit/library/build/XUL +0x3f66d3c]

I think given we do the IO on a background thread, this is probably better than sync-loading them "just in time", so going to resolve this WFM. If there is still a problem here in practice, let's create a new bug with more specific details.