Closed
Bug 496217
Opened 15 years ago
Closed 2 years ago
delay initialization of spellcheck dict until actually needed
Categories
(Core :: Spelling checker, defect, P3)
Core
Spelling checker
Tracking
()
RESOLVED
WORKSFORME
| Tracking | Status | |
|---|---|---|
| status1.9.1 | --- | wanted |
People
(Reporter: vlad, Unassigned)
References
(Blocks 1 open bug)
Details
(Keywords: perf, Whiteboard: [ts][fxperf:p3])
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.
Flags: wanted1.9.2+
Flags: wanted1.9.1.x+
|
Reporter | |
Updated•15 years ago
|
Whiteboard: startup
|
||
Updated•15 years ago
|
Whiteboard: startup → startup [ts]
|
||
Updated•15 years ago
|
status1.9.1:
--- → wanted
Flags: wanted1.9.1.x+
|
|
||
Comment 1•15 years ago
|
||
taras confirmed this is costly (70ms) -> P1.
Assignee: nobody → rflint
Priority: -- → P1
|
|
||
Comment 2•15 years ago
|
||
should delay initialization, and move the dict files into a jar.
|
||
Comment 3•15 years ago
|
||
Any changes to dictionary package impact l10n repacks, and other variants of builds we do, fwiw.
|
||
Comment 4•15 years ago
|
||
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.
Are you guys aware of bug 468779, some Chromium changes to speed up spellcheck initialization?
|
||
Comment 6•7 years ago
|
||
We should check to see if this is still a relevant bug.
Whiteboard: [ts] → [ts][fxperf]
|
||
Comment 7•7 years ago
|
||
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).
Whiteboard: [ts][fxperf] → [ts][fxperf:p3]
|
||
Updated•7 years ago
|
Priority: -- → P3
|
||
Updated•2 years ago
|
Severity: normal → S3
|
||
Comment 8•2 years ago
|
||
(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.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WORKSFORME
You need to log in
before you can comment on or make changes to this bug.
Description
•