The default bug view has changed. See this FAQ.

PrefixSet loading should use readahead

RESOLVED FIXED in Firefox 10

Status

()

Toolkit
Safe Browsing
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: gcp, Assigned: gcp)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

6 years ago
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)

Updated

6 years ago
Assignee: nobody → tglek

Comment 1

6 years ago
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

6 years ago
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
(Assignee)

Comment 3

6 years ago
Created attachment 561702 [details] [diff] [review]
Patch 1. Expose OS-level readahead
Attachment #561033 - Attachment is obsolete: true
Attachment #561702 - Flags: review?(tglek)
(Assignee)

Comment 4

6 years ago
Created attachment 561703 [details] [diff] [review]
Patch 2. Optimize for serial load/write
Attachment #561703 - Flags: review?(tglek)
(Assignee)

Comment 5

6 years ago
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

6 years ago
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+

Comment 7

6 years ago
Created attachment 561893 [details] [diff] [review]
Patch 1. Expose OS-level readahead
Attachment #561702 - Attachment is obsolete: true
Attachment #561702 - Flags: review?(tglek)
Attachment #561893 - Flags: review?(benjamin)

Comment 8

6 years ago
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.
Attachment #561893 - Attachment is obsolete: true
Attachment #561893 - Flags: review?(benjamin)
Attachment #561926 - Flags: review?(benjamin)

Comment 9

6 years ago
Created attachment 561927 [details] [diff] [review]
Patch 1. Expose OS-level readahead

Sorry, didn't qref above.
Attachment #561926 - Attachment is obsolete: true
Attachment #561926 - Flags: review?(benjamin)
Attachment #561927 - Flags: review?(benjamin)
(Assignee)

Comment 10

6 years ago
Created attachment 562230 [details] [diff] [review]
Patch 2. Optimize for serial load/write

Added Telemetry probe.
Attachment #561703 - Attachment is obsolete: true
Attachment #562230 - Flags: review+
(Assignee)

Comment 11

6 years ago
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=d7569eaa6ced
Attachment #561927 - Flags: review?(benjamin) → review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
http://hg.mozilla.org/integration/mozilla-inbound/rev/fd5e3cb57245
http://hg.mozilla.org/integration/mozilla-inbound/rev/eeeb4aa609f6
Keywords: checkin-needed
Target Milestone: --- → Firefox 10
https://hg.mozilla.org/mozilla-central/rev/fd5e3cb57245
https://hg.mozilla.org/mozilla-central/rev/eeeb4aa609f6
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Blocks: 693485
Depends on: 726503
Component: Phishing Protection → Phishing Protection
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.