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

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[(dormant account)]

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

Comment 1

[(dormant account)]

For comparison, chrome's bloom file has 116fragments and leads 2x slower than firefox equiv.

Comment 2

[Gian-Carlo Pascutto [:gcp]]

>To read files fast one needs to specify readhead...

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

Comment 3

[(dormant account)]

(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 :(

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: 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.

Comment 5

[Gian-Carlo Pascutto [:gcp]]

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.

Comment 6

[Gian-Carlo Pascutto [:gcp]]

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...

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

(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.

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

(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?

Comment 9

[Gian-Carlo Pascutto [:gcp]]

>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.

Comment 10

[Vladan Djeric (:vladan)]

Comment on attachment 689922 [details] [diff] [review]
Adds readahead to savebrowsing .sbstore and .cache files

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ed78ee40e0e

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7ed78ee40e0e

Comment 12

[Manoj]

(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.

Comment 13

[Gian-Carlo Pascutto [:gcp]]

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

[Manoj]

Filed bug 825042.