503713 - safe browsing requests shouldn't be placed in disk cache

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Justin Dolske [:Dolske]]

Open up about:cache, and look through the disk cache entries. You'll see multiple entries from requests to the google safebrowsing server.

I see entries like

Key: http://safebrowsing-cache.google.com/safebrowsing/rd/goog-phish-shavar_a_#######

and

Key: id=4a57da87&uri=http://safebrowsing.clients.google.com/safebrowsing/downloads/......

AFAIK all this data just ends up in urlclassifier3.sqlite, so it's just wasting space sitting in the disk cache, and crowding out other things that could be using the cache.

Biesi suggests that maybe it "should set the INHIBIT_CACHING flag".

Comment 1

[Justin Dolske [:Dolske]]

I wonder if this also contributes to disk thrashing, since we're writing to two files (disk cache, SQL DB) for each entry.

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

We're sure it doesn't use the cache version? If so, this sounds like a good idea. Our cache isn't known to be the fastest thing in the world, I wonder if this would give us a significant perf boost.

If we can get an idea of perf gains, feel free to renominate.

Comment 3

[David Dahl :ddahl]

If url-classifier were using cached http requests, how would it look them up in the cache? I just want to rule that out before looking into "INHIBIT_CACHING". I assume we are not using any cache data in safebrowsing.

Comment 4

[Justin Dolske [:Dolske]]

It would be a little silly for it to go out of its way to use the HTTP cache, considering the data must be available in the on-disk SQL DB.

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #3)
> If url-classifier were using cached http requests, how would it look them up in
> the cache? I just want to rule that out before looking into "INHIBIT_CACHING".
> I assume we are not using any cache data in safebrowsing.

Had a look into this... from what I understand:

* The stream updater will only ever send unique requests (each partial update is unique), so no chance of a cache hit.
* The hash completer uses batch requests, which would (almost) always result in a cache miss. Also, I *think* it will only send requests with a partial hashes (to get the complete hash), then store the complete hash, so not have to request the same partial hash more than once.

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1

This patch adds INHIBIT_CACHING to the stream updater and the hash completer requests. I've also added LOAD_BYPASS_CACHE to avoid any cache lookup, since that will be guaranteed to always result in a miss anyway.

Comment 7

[David Dahl :ddahl]

Blair: I am not sure if dcamp is reading bugmail or reviewing safebrowsing bugs. Did you write a test case for this change?

Comment 8

[Samuel Sidler (old account; do not CC)]

He'll likely review if you email him personally, but Tony Chang also does reviews, I believe.

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #7)
> Did you write a test case for this change?

No, I didn't think it was necessary, since I'm only adding flags that don't actually affect how any of this code works. If tests are warranted, I'll need help from someone - I haven't the faintest idea on how to automatically test this.

(In reply to comment #8)
> but Tony Chang also does reviews, I believe.

Ok.

Comment 10

[Tony Chang (Google)]

Comment on attachment 405820 [details] [diff] [review]
Patch v1

Looks good.

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/14f4a91d2ffa

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Please be sure to nominate this for a+ assuming it passes all tests on mozilla-central

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 405820 [details] [diff] [review]
Patch v1

approval1.9.2 requests aren't currently being monitored, since we're nearing RC freeze and there are too many outstanding requests, so I'm clearing this request. Feel free to re-request approval if you are confident that it's worth drivers' time to consider whether this non-blocker needs to land for 1.9.2 at this stage.

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Ah snap - I thought this had landed on 1.9.2 already :( Anyway, doesn't look like this gave any significant pref boost on trunk after all.

Comment 15

[Reed Loden [:reed]]

Comment on attachment 405820 [details] [diff] [review]
Patch v1

If it means less churning on the disk i/o side, I think it's worth it. You wouldn't believe the number of complaints I hear about safe browsing causing high disk i/o.

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #15)
> (From update of attachment 405820 [details] [diff] [review])
> If it means less churning on the disk i/o side, I think it's worth it. You
> wouldn't believe the number of complaints I hear about safe browsing causing
> high disk i/o.

Fair enough - theoretically it should help there. Pity disk IO isn't measured & graphed (or is it?) so this could be quantified.

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 405820 [details] [diff] [review]
Patch v1

a192=beltzner, seems very safe to me and less disk churn helps save battery, amongst other things

Comment 18

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3585501111ca