Supervised user SafeSites: don't cache results for more than one hour

Supervised user SafeSites: don't cache results for more than one hour

BUG=475513
Committed: https://crrev.com/b4c09109409ff876fd0f725fa5c55cfe0a952e92
Cr-Commit-Position: refs/heads/master@{#355048}

[Marc Treib, 2015-10-15 15:28:23]

treib@chromium.org changed reviewers:
+ bauerb@chromium.org

[Marc Treib, 2015-10-15 15:28:24]

PTAL!

https://codereview.chromium.org/1403153004/diff/40001/chrome/browser/supervis...
File
chrome/browser/supervised_user/experimental/supervised_user_async_url_checker.cc
(right):

https://codereview.chromium.org/1403153004/diff/40001/chrome/browser/supervis...
chrome/browser/supervised_user/experimental/supervised_user_async_url_checker.cc:98:
base::TimeTicks start_time;
Unrelated

[Marc Treib, 2015-10-20 07:58:45]

Ping!

[Bernhard Bauer, 2015-10-20 08:10:03]

Sorry, overlooked this.

https://codereview.chromium.org/1403153004/diff/40001/chrome/browser/supervis...
File
chrome/browser/supervised_user/experimental/supervised_user_async_url_checker.cc
(right):

https://codereview.chromium.org/1403153004/diff/40001/chrome/browser/supervis...
chrome/browser/supervised_user/experimental/supervised_user_async_url_checker.cc:166:
cache_.Erase(cache_it);
Hm... just as a small possible optimization: We evict entries from the MRU cache
based on the last time they were _accessed_, but we also only consider cache
entries that were _written_ not longer than a certain period of time ago. This
means that there could potentially be values in the cache that we are not
interested in anymore, but which are still competing for space with fresher
entries. Would it make sense to do a full pass over the cache from time to time
and prune all the stale entries to reduce space pressure?

https://codereview.chromium.org/1403153004/diff/40001/chrome/browser/supervis...
File
chrome/browser/supervised_user/experimental/supervised_user_async_url_checker.h
(right):

https://codereview.chromium.org/1403153004/diff/40001/chrome/browser/supervis...
chrome/browser/supervised_user/experimental/supervised_user_async_url_checker.h:41:
const base::TimeDelta& cache_timeout() const { return cache_timeout_; }
Do you actually need this method?

https://codereview.chromium.org/1403153004/diff/40001/chrome/browser/supervis...
chrome/browser/supervised_user/experimental/supervised_user_async_url_checker.h:42:
void set_cache_timeout(const base::TimeDelta& timeout) {
Name this method SetCacheTimeoutForTesting?

[Marc Treib, 2015-10-20 08:53:55]

https://codereview.chromium.org/1403153004/diff/40001/chrome/browser/supervis...
File
chrome/browser/supervised_user/experimental/supervised_user_async_url_checker.cc
(right):

https://codereview.chromium.org/1403153004/diff/40001/chrome/browser/supervis...
chrome/browser/supervised_user/experimental/supervised_user_async_url_checker.cc:166:
cache_.Erase(cache_it);
On 2015/10/20 08:10:03, Bernhard Bauer wrote:
> Hm... just as a small possible optimization: We evict entries from the MRU
cache
> based on the last time they were _accessed_, but we also only consider cache
> entries that were _written_ not longer than a certain period of time ago. This
> means that there could potentially be values in the cache that we are not
> interested in anymore, but which are still competing for space with fresher
> entries. Would it make sense to do a full pass over the cache from time to
time
> and prune all the stale entries to reduce space pressure?

I thought about it, and decided it's probably not worth the effort/overhead.
Worst case, we'll evict some still-valid entries, while keeping expired but
more-recently-accessed ones. Oh well.
If you do think it's worth it: What would be a good trigger for the cleanup
pass? On every n-th request (when the cache is full)?

https://codereview.chromium.org/1403153004/diff/40001/chrome/browser/supervis...
File
chrome/browser/supervised_user/experimental/supervised_user_async_url_checker.h
(right):

https://codereview.chromium.org/1403153004/diff/40001/chrome/browser/supervis...
chrome/browser/supervised_user/experimental/supervised_user_async_url_checker.h:41:
const base::TimeDelta& cache_timeout() const { return cache_timeout_; }
On 2015/10/20 08:10:03, Bernhard Bauer wrote:
> Do you actually need this method?

No. Removed.

https://codereview.chromium.org/1403153004/diff/40001/chrome/browser/supervis...
chrome/browser/supervised_user/experimental/supervised_user_async_url_checker.h:42:
void set_cache_timeout(const base::TimeDelta& timeout) {
On 2015/10/20 08:10:03, Bernhard Bauer wrote:
> Name this method SetCacheTimeoutForTesting?

Done.

[Bernhard Bauer, 2015-10-20 10:37:15]

lgtm

https://codereview.chromium.org/1403153004/diff/40001/chrome/browser/supervis...
File
chrome/browser/supervised_user/experimental/supervised_user_async_url_checker.cc
(right):

https://codereview.chromium.org/1403153004/diff/40001/chrome/browser/supervis...
chrome/browser/supervised_user/experimental/supervised_user_async_url_checker.cc:166:
cache_.Erase(cache_it);
On 2015/10/20 08:53:55, Marc Treib wrote:
> On 2015/10/20 08:10:03, Bernhard Bauer wrote:
> > Hm... just as a small possible optimization: We evict entries from the MRU
> cache
> > based on the last time they were _accessed_, but we also only consider cache
> > entries that were _written_ not longer than a certain period of time ago.
This
> > means that there could potentially be values in the cache that we are not
> > interested in anymore, but which are still competing for space with fresher
> > entries. Would it make sense to do a full pass over the cache from time to
> time
> > and prune all the stale entries to reduce space pressure?
> 
> I thought about it, and decided it's probably not worth the effort/overhead.
> Worst case, we'll evict some still-valid entries, while keeping expired but
> more-recently-accessed ones. Oh well.
> If you do think it's worth it: What would be a good trigger for the cleanup
> pass? On every n-th request (when the cache is full)?

Yes, I was thinking something like on every n-th cache eviction (where n is the
number of entries in the cache), so that we have an upper bound on unnecessarily
evicted elements, and the total overhead is amortized to constant.

It might make sense to gather some stats before we make any productive changes
to the code anyway. Do we have something like that already to get our cache hit
rate etc.?

[Marc Treib, 2015-10-20 11:53:50]

https://codereview.chromium.org/1403153004/diff/40001/chrome/browser/supervis...
File
chrome/browser/supervised_user/experimental/supervised_user_async_url_checker.cc
(right):

https://codereview.chromium.org/1403153004/diff/40001/chrome/browser/supervis...
chrome/browser/supervised_user/experimental/supervised_user_async_url_checker.cc:166:
cache_.Erase(cache_it);
On 2015/10/20 10:37:15, Bernhard Bauer wrote:
> On 2015/10/20 08:53:55, Marc Treib wrote:
> > On 2015/10/20 08:10:03, Bernhard Bauer wrote:
> > > Hm... just as a small possible optimization: We evict entries from the MRU
> > cache
> > > based on the last time they were _accessed_, but we also only consider
cache
> > > entries that were _written_ not longer than a certain period of time ago.
> This
> > > means that there could potentially be values in the cache that we are not
> > > interested in anymore, but which are still competing for space with
fresher
> > > entries. Would it make sense to do a full pass over the cache from time to
> > time
> > > and prune all the stale entries to reduce space pressure?
> > 
> > I thought about it, and decided it's probably not worth the effort/overhead.
> > Worst case, we'll evict some still-valid entries, while keeping expired but
> > more-recently-accessed ones. Oh well.
> > If you do think it's worth it: What would be a good trigger for the cleanup
> > pass? On every n-th request (when the cache is full)?
> 
> Yes, I was thinking something like on every n-th cache eviction (where n is
the
> number of entries in the cache), so that we have an upper bound on
unnecessarily
> evicted elements, and the total overhead is amortized to constant.
> 
> It might make sense to gather some stats before we make any productive changes
> to the code anyway. Do we have something like that already to get our cache
hit
> rate etc.?

No, we don't have any stats on the cache in particular. I'll think about adding
some. (But we don't have enough users yet anyway for those to tell us
anything...)

[Marc Treib, 2015-10-20 11:55:46]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2015-10-20 11:56:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1403153004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1403153004/60001

[commit-bot: I haz the power, 2015-10-20 13:00:15]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-10-20 13:01:51]

Patchset 4 (id:??) landed as
https://crrev.com/b4c09109409ff876fd0f725fa5c55cfe0a952e92
Cr-Commit-Position: refs/heads/master@{#355048}