loading the two safebrowsing files is not as fast as it could be

RESOLVED FIXED in Firefox 20

Status

()

Toolkit
Safe Browsing
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: (dormant account), Assigned: aklotz)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 20
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
on my test system xul.dll is read from disk faster than the phishing files even though it involves 6mb more data reading(800ms vs 140ms).
malware file is also fragmented into 6 pieces vs 1 for phish file. To read files fast one needs to specify readhead...see http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/standalone/nsGlueLinkingWin.cpp#43 for an example
(Reporter)

Comment 1

5 years ago
For comparison, chrome's bloom file has 116fragments and leads 2x slower than firefox equiv.
(Reporter)

Updated

5 years ago
Blocks: 810156
>To read files fast one needs to specify readhead...

Readahead and fallocate were already added for those files in bug 686691.
(Reporter)

Comment 3

5 years ago
(In reply to Gian-Carlo Pascutto (:gcp) from comment #2)
> >To read files fast one needs to specify readhead...
> 
> Readahead and fallocate were already added for those files in bug 686691.

I wonder why it's not working then :(
Assignee: nobody → aklotz
Created attachment 689922 [details] [diff] [review]
Adds readahead to savebrowsing .sbstore and .cache files

We were using readahead for .pset files but not .sbstore or .cache files. This patch shaved 50ms off on my box.
Attachment #689922 - Flags: review?(gpascutto)
Comment on attachment 689922 [details] [diff] [review]
Adds readahead to savebrowsing .sbstore and .cache files

We shouldn't read the HashStore (sbtore) on startup, but when we do during an update it's a serial read, so this should be good.
Attachment #689922 - Flags: review?(gpascutto) → review+
Actually, the cache file should be minimal. Like < 512 bytes typically, so readahead won't do much there.

The issue is that we read the chunk info from the sbstore files to determine if they contain valid data (in RegenActiveTables). It's not clear to me if that is really needed - the data is needed just before an update check, but I can't think of a case where they would be present but without chunk info and where we don't want to update them.

RegenActiveTables being active during startup isn't very good in itself, as it's scanning directories...
(In reply to Gian-Carlo Pascutto (:gcp) from comment #5)
> Comment on attachment 689922 [details] [diff] [review]
> Adds readahead to savebrowsing .sbstore and .cache files
> 
> We shouldn't read the HashStore (sbtore) on startup, but when we do during
> an update it's a serial read, so this should be good.

A HashStore read is being kicked off when there is a lookup in nsHttpChannel::BeginConnect. Even though the read is being done off the main thread, the channel is suspended until the lookup completes.
(In reply to Gian-Carlo Pascutto (:gcp) from comment #6)
> Actually, the cache file should be minimal. Like < 512 bytes typically, so
> readahead won't do much there.
> 
I agree that it won't do much and I can remove it if you prefer. OTOH given that the change does no harm, leaving it in means that we wouldn't need to go in and add it later should the file format change and the file grows larger. Thoughts?
Flags: needinfo?(gpascutto)
>A HashStore read is being kicked off when there is a lookup in 
>nsHttpChannel::BeginConnect. Even though the read is being done off the main 
>thread, the channel is suspended until the lookup completes.

This is unexpected.

>I agree that it won't do much and I can remove it if you prefer. OTOH given that 
>the change does no harm, leaving it in means that we wouldn't need to go in and 
>add it later should the file format change and the file grows larger. Thoughts?

I'd keep it, as you mentioned it does no harm.
Flags: needinfo?(gpascutto)
Attachment #689922 - Flags: checkin?(vdjeric)
Comment on attachment 689922 [details] [diff] [review]
Adds readahead to savebrowsing .sbstore and .cache files

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ed78ee40e0e
Attachment #689922 - Flags: checkin?(vdjeric) → checkin+
https://hg.mozilla.org/mozilla-central/rev/7ed78ee40e0e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20

Comment 12

5 years ago
(In reply to Gian-Carlo Pascutto (:gcp) from comment #9)
> >A HashStore read is being kicked off when there is a lookup in 
> >nsHttpChannel::BeginConnect. Even though the read is being done off the main 
> >thread, the channel is suspended until the lookup completes.
> 
> This is unexpected.
> 
If it makes sense to file a bug to investigate the cause of this, I will file it. Please let me know.
Yes please, with as much info when/where it originates.

Maybe it turns out to be not entirely unexpected but it would be good to document where the current code is suboptimal.

Comment 14

5 years ago
Filed bug 825042.
Component: Phishing Protection → Phishing Protection
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.