Last Comment Bug 686691 - PrefixSet loading should use readahead
: PrefixSet loading should use readahead
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Safe Browsing (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 10
Assigned To: Gian-Carlo Pascutto [:gcp]
:
Mentors:
Depends on: 726503
Blocks: 693485
  Show dependency treegraph
 
Reported: 2011-09-14 09:36 PDT by Gian-Carlo Pascutto [:gcp]
Modified: 2014-05-27 12:25 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
plumbing to expose os-level readhead (2.23 KB, patch)
2011-09-19 14:57 PDT, (dormant account)
no flags Details | Diff | Splinter Review
Patch 1. Expose OS-level readahead (3.68 KB, patch)
2011-09-22 04:58 PDT, Gian-Carlo Pascutto [:gcp]
no flags Details | Diff | Splinter Review
Patch 2. Optimize for serial load/write (1.40 KB, patch)
2011-09-22 04:59 PDT, Gian-Carlo Pascutto [:gcp]
taras.mozilla: review+
Details | Diff | Splinter Review
Patch 1. Expose OS-level readahead (2.44 KB, patch)
2011-09-22 14:46 PDT, (dormant account)
no flags Details | Diff | Splinter Review
Patch 1. Expose OS-level readahead (2.44 KB, patch)
2011-09-22 16:44 PDT, (dormant account)
no flags Details | Diff | Splinter Review
Patch 1. Expose OS-level readahead (2.46 KB, patch)
2011-09-22 16:46 PDT, (dormant account)
benjamin: review+
Details | Diff | Splinter Review
Patch 2. Optimize for serial load/write (3.16 KB, patch)
2011-09-24 02:20 PDT, Gian-Carlo Pascutto [:gcp]
gpascutto: review+
Details | Diff | Splinter Review

Description Gian-Carlo Pascutto [:gcp] 2011-09-14 09:36:08 PDT
The loading of the PrefixSet datastructure in the url-classifier is a linear +-1.2M read from disk. From Telemetry we see that nevertheless this is taking >1s for some users. Readahead could help here.
Comment 1 (dormant account) 2011-09-19 14:57:14 PDT
Created attachment 561033 [details] [diff] [review]
plumbing to expose os-level readhead

use something like 
rv = file->OpenNSPRFileDesc(PR_RDONLY | nsILocalFile::OS_READAHEAD, 0, &fileFd); 
This will cause os-level readahead to kickin, usually this means windows/linux will read the file in 2mb increments instead of <=128K.
Comment 2 (dormant account) 2011-09-19 15:00:00 PDT
It might also help to use mozilla::fallocate when you write out the file(if you can predict the file size). If you can't predict an exact size you can allocate a bigger file and then ftruncate. This avoids fragmentation.
Comment 3 Gian-Carlo Pascutto [:gcp] 2011-09-22 04:58:49 PDT
Created attachment 561702 [details] [diff] [review]
Patch 1. Expose OS-level readahead
Comment 4 Gian-Carlo Pascutto [:gcp] 2011-09-22 04:59:46 PDT
Created attachment 561703 [details] [diff] [review]
Patch 2. Optimize for serial load/write
Comment 5 Gian-Carlo Pascutto [:gcp] 2011-09-22 05:19:17 PDT
I fixed 2 things in your code:

- posix_fadvise isn't in Android's libc. I added checks for the relevant levels of POSIX conformance.
- #include "private/pprio.h" is required for PR_FileDesc2NativeHandle.
Comment 6 (dormant account) 2011-09-22 11:00:52 PDT
Comment on attachment 561703 [details] [diff] [review]
Patch 2. Optimize for serial load/write

> 
> nsresult
> nsUrlClassifierPrefixSet::StoreToFd(AutoFDClose & fileFd)
> {
>+  PRInt64 size = 4 * sizeof(PRUint32);
>+  size += 2 * mIndexStarts.Length() * sizeof(PRUint32);
>+  size +=     mDeltas.Length() * sizeof(PRUint16);
>+
>+  mozilla::fallocate(fileFd, size);

One thing I forgot to mention. On crappy filesystems fallocate can be an expensive call. This needs to get a telemetry probe to see if this is expensive, then we can decide to turn this off on RFS/VFAT/etc.
Comment 7 (dormant account) 2011-09-22 14:46:52 PDT
Created attachment 561893 [details] [diff] [review]
Patch 1. Expose OS-level readahead
Comment 8 (dormant account) 2011-09-22 16:44:52 PDT
Created attachment 561926 [details] [diff] [review]
Patch 1. Expose OS-level readahead

Do not do readhead() on android. I will address fallocate & readahead for android in a followup.
Comment 9 (dormant account) 2011-09-22 16:46:18 PDT
Created attachment 561927 [details] [diff] [review]
Patch 1. Expose OS-level readahead

Sorry, didn't qref above.
Comment 10 Gian-Carlo Pascutto [:gcp] 2011-09-24 02:20:47 PDT
Created attachment 562230 [details] [diff] [review]
Patch 2. Optimize for serial load/write

Added Telemetry probe.
Comment 11 Gian-Carlo Pascutto [:gcp] 2011-09-24 02:22:30 PDT
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=d7569eaa6ced

Note You need to log in before you can comment on or make changes to this bug.