Closed Bug 857830 Opened 7 years ago Closed 7 years ago

Consider reading ahead the spellchecker dictionary

Categories

(Core :: Spelling checker, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: ehsan, Assigned: aklotz)

References

Details

Attachments

(2 files, 2 obsolete files)

We read the dictionary once but by terrible fread calls.  We should consider ReadAhead-ing it beforehand.  Aaron, are you interested in taking this?
Sure, I'll look into it.
Assignee: nobody → aklotz
Thanks, let me know if you need help.
ReadAheadFile should set aOutFd to error if the function is failing.
Attachment #743702 - Flags: review?(mh+mozilla)
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)
Attachment #743702 - Flags: review?(mh+mozilla) → review+
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+
It would be better to avoid patching hunspell altogether if we can avoid it.
(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.
I'd rather see something like extensions/spellcheck/hunspell/src/hunspell_alloc_hooks.h
(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!
Comment on attachment 743715 [details] [diff] [review]
Part 2: Readahead for the spellchecker dictionary

Sorry for changing my mind!
Attachment #743715 - Flags: review+ → review-
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)
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+
Fixed nit, carrying forward r+
Attachment #745933 - Attachment is obsolete: true
Attachment #746247 - Flags: review+
CCing the Hunspell guys in case there's any interest in upstreaming this.
https://hg.mozilla.org/mozilla-central/rev/e1f30dbb8108
https://hg.mozilla.org/mozilla-central/rev/abaf789d8030
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Depends on: 902532
You need to log in before you can comment on or make changes to this bug.