Closed Bug 686691 Opened 10 years ago Closed 10 years ago

PrefixSet loading should use readahead

Categories

(Toolkit :: Safe Browsing, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 10

People

(Reporter: gcp, Assigned: gcp)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 5 obsolete files)

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.
Assignee: nobody → tglek
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.
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.
Assignee: tglek → gpascutto
Attachment #561033 - Attachment is obsolete: true
Attachment #561702 - Flags: review?(tglek)
Attachment #561703 - Flags: review?(tglek)
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 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.
Attachment #561703 - Flags: review?(tglek) → review+
Attachment #561702 - Attachment is obsolete: true
Attachment #561702 - Flags: review?(tglek)
Attachment #561893 - Flags: review?(benjamin)
Do not do readhead() on android. I will address fallocate & readahead for android in a followup.
Attachment #561893 - Attachment is obsolete: true
Attachment #561893 - Flags: review?(benjamin)
Attachment #561926 - Flags: review?(benjamin)
Sorry, didn't qref above.
Attachment #561926 - Attachment is obsolete: true
Attachment #561926 - Flags: review?(benjamin)
Attachment #561927 - Flags: review?(benjamin)
Added Telemetry probe.
Attachment #561703 - Attachment is obsolete: true
Attachment #562230 - Flags: review+
Attachment #561927 - Flags: review?(benjamin) → review+
Keywords: checkin-needed
Blocks: 693485
Depends on: 726503
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.