Last Comment Bug 810101 - loading the two safebrowsing files is not as fast as it could be
: loading the two safebrowsing files is not as fast as it could be
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Safe Browsing (show other bugs)
: unspecified
: x86_64 Windows 7
: -- normal (vote)
: Firefox 20
Assigned To: Aaron Klotz [:aklotz]
:
Mentors:
Depends on:
Blocks: start-faster
  Show dependency treegraph
 
Reported: 2012-11-08 15:32 PST by (dormant account)
Modified: 2014-05-27 12:25 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Adds readahead to savebrowsing .sbstore and .cache files (1.79 KB, patch)
2012-12-07 14:15 PST, Aaron Klotz [:aklotz]
gpascutto: review+
vladan.bugzilla: checkin+
Details | Diff | Splinter Review

Description (dormant account) 2012-11-08 15:32:25 PST
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) 2012-11-08 16:18:50 PST
For comparison, chrome's bloom file has 116fragments and leads 2x slower than firefox equiv.
Comment 2 Gian-Carlo Pascutto [:gcp] 2012-11-10 00:59:39 PST
>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) 2012-11-10 20:30:11 PST
(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 Aaron Klotz [:aklotz] 2012-12-07 14:15:45 PST
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.
Comment 5 Gian-Carlo Pascutto [:gcp] 2012-12-10 06:36:40 PST
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] 2012-12-10 06:47:01 PST
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 Aaron Klotz [:aklotz] 2012-12-11 13:24:31 PST
(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 Aaron Klotz [:aklotz] 2012-12-11 13:27:49 PST
(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] 2012-12-11 14:22:45 PST
>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) 2012-12-12 15:06:30 PST
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] 2012-12-13 08:05:03 PST
https://hg.mozilla.org/mozilla-central/rev/7ed78ee40e0e
Comment 12 Manoj 2012-12-25 17:06:11 PST
(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] 2012-12-26 02:25:23 PST
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 2012-12-27 12:02:28 PST
Filed bug 825042.

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