Closed
Bug 857830
Opened 11 years ago
Closed 11 years ago
Consider reading ahead the spellchecker dictionary
Categories
(Core :: Spelling checker, defect)
Tracking
()
RESOLVED
FIXED
mozilla23
People
(Reporter: ehsan.akhgari, Assigned: bugzilla)
References
Details
Attachments
(2 files, 2 obsolete files)
1.54 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
3.35 KB,
patch
|
bugzilla
:
review+
|
Details | Diff | Splinter Review |
We read the dictionary once but by terrible fread calls. We should consider ReadAhead-ing it beforehand. Aaron, are you interested in taking this?
Reporter | ||
Comment 2•11 years ago
|
||
Thanks, let me know if you need help.
Assignee | ||
Comment 3•11 years ago
|
||
ReadAheadFile should set aOutFd to error if the function is failing.
Attachment #743702 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 4•11 years ago
|
||
The entire dictionary file is being read sequentially via an fgets loop. In this patch I use the aOutFd parameter when calling ReadAheadFile, allowing me to pass the file descriptor used for readahead into fdopen, reusing it for the fgets loop.
Attachment #743715 -
Flags: review?(ehsan)
Updated•11 years ago
|
Attachment #743702 -
Flags: review?(mh+mozilla) → review+
Reporter | ||
Comment 5•11 years ago
|
||
Comment on attachment 743715 [details] [diff] [review] Part 2: Readahead for the spellchecker dictionary Review of attachment 743715 [details] [diff] [review]: ----------------------------------------------------------------- Please list this bug number in the "Additional patches" in README.hunspell too.
Attachment #743715 -
Flags: review?(ehsan) → review+
Comment 6•11 years ago
|
||
It would be better to avoid patching hunspell altogether if we can avoid it.
Reporter | ||
Comment 7•11 years ago
|
||
(In reply to comment #6) > It would be better to avoid patching hunspell altogether if we can avoid it. That ship has sailed a long time ago! This is fine since it's very well localized.
Comment 8•11 years ago
|
||
I'd rather see something like extensions/spellcheck/hunspell/src/hunspell_alloc_hooks.h
Reporter | ||
Comment 9•11 years ago
|
||
(In reply to comment #8) > I'd rather see something like > extensions/spellcheck/hunspell/src/hunspell_alloc_hooks.h Yeah you're right, that's a good idea!
Reporter | ||
Comment 10•11 years ago
|
||
Comment on attachment 743715 [details] [diff] [review] Part 2: Readahead for the spellchecker dictionary Sorry for changing my mind!
Attachment #743715 -
Flags: review+ → review-
Assignee | ||
Comment 11•11 years ago
|
||
This version hooks fopen via macro. It uses the readahead code path if mode is equivalent to O_RDONLY, otherwise it falls back to the C runtime's fopen.
Attachment #743715 -
Attachment is obsolete: true
Attachment #745933 -
Flags: review?(ehsan)
Reporter | ||
Comment 12•11 years ago
|
||
Comment on attachment 745933 [details] [diff] [review] Part 2: Readahead for the spellchecker dictionary using fopen hook Review of attachment 745933 [details] [diff] [review]: ----------------------------------------------------------------- ::: mozilla-config.h.in @@ +20,5 @@ > * unless --enable-system-hunspell is defined. > */ > #if defined(HUNSPELL_STATIC) > #include "hunspell_alloc_hooks.h" > +#include "hunspell_fopen_hook.h" Nit: please name this hunspell_fopen_hooks.h for consistency.
Attachment #745933 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 13•11 years ago
|
||
Fixed nit, carrying forward r+
Attachment #745933 -
Attachment is obsolete: true
Attachment #746247 -
Flags: review+
Assignee | ||
Comment 14•11 years ago
|
||
Part 1: https://hg.mozilla.org/integration/mozilla-inbound/rev/e1f30dbb8108 Part 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/abaf789d8030 Try builds: All other platforms: https://tbpl.mozilla.org/?tree=Try&rev=b0f03bcc2984 Fixed for Windows: https://tbpl.mozilla.org/?tree=Try&rev=892f1edadc45
Comment 15•11 years ago
|
||
CCing the Hunspell guys in case there's any interest in upstreaming this.
Comment 16•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e1f30dbb8108 https://hg.mozilla.org/mozilla-central/rev/abaf789d8030
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in
before you can comment on or make changes to this bug.
Description
•