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

chrome/browser/supervised_user/experimental/supervised_user_async_url_checker.cc

Index: chrome/browser/supervised_user/experimental/supervised_user_async_url_checker.cc
diff --git a/chrome/browser/supervised_user/experimental/supervised_user_async_url_checker.cc b/chrome/browser/supervised_user/experimental/supervised_user_async_url_checker.cc
index f2501b4516f99bb1c4e6be807ab02f3baf76f5df..2c379c2a1d7add3c88285a331d13ff5e51b27dcb 100644
--- a/chrome/browser/supervised_user/experimental/supervised_user_async_url_checker.cc
+++ b/chrome/browser/supervised_user/experimental/supervised_user_async_url_checker.cc
@@ -35,6 +35,7 @@ const char kDataContentType[] = "application/x-www-form-urlencoded";
 const char kDataFormat[] = "key=%s&urls=%s";
 
 const size_t kDefaultCacheSize = 1000;
+const size_t kDefaultCacheTimeoutSeconds = 3600;
 
 // Builds the POST data for SafeSearch API requests.
 std::string BuildRequestData(const std::string& api_key, const GURL& url) {
@@ -94,7 +95,7 @@ struct SupervisedUserAsyncURLChecker::Check {
   GURL url;
   scoped_ptr<net::URLFetcher> fetcher;
   std::vector<CheckCallback> callbacks;
-  base::Time start_time;
+  base::TimeTicks start_time;

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

Unrelated

 };
 
 SupervisedUserAsyncURLChecker::Check::Check(
@@ -104,33 +105,36 @@ SupervisedUserAsyncURLChecker::Check::Check(
     : url(url),
       fetcher(fetcher.Pass()),
       callbacks(1, callback),
-      start_time(base::Time::Now()) {
+      start_time(base::TimeTicks::Now()) {
 }
 
 SupervisedUserAsyncURLChecker::Check::~Check() {}
 
 SupervisedUserAsyncURLChecker::CheckResult::CheckResult(
-    SupervisedUserURLFilter::FilteringBehavior behavior, bool uncertain)
-    : behavior(behavior), uncertain(uncertain) {
-}
+    SupervisedUserURLFilter::FilteringBehavior behavior,
+    bool uncertain)
+    : behavior(behavior),
+      uncertain(uncertain),
+      timestamp(base::TimeTicks::Now()) {}
 
 SupervisedUserAsyncURLChecker::SupervisedUserAsyncURLChecker(
     URLRequestContextGetter* context)
-    : context_(context), cache_(kDefaultCacheSize) {
-}
+    : SupervisedUserAsyncURLChecker(context, kDefaultCacheSize) {}
 
 SupervisedUserAsyncURLChecker::SupervisedUserAsyncURLChecker(
     URLRequestContextGetter* context,
     size_t cache_size)
-    : context_(context), cache_(cache_size) {
-}
+    : context_(context),
+      cache_(cache_size),
+      cache_timeout_(
+          base::TimeDelta::FromSeconds(kDefaultCacheTimeoutSeconds)) {}
 
 SupervisedUserAsyncURLChecker::~SupervisedUserAsyncURLChecker() {}
 
 bool SupervisedUserAsyncURLChecker::CheckURL(const GURL& url,
                                              const CheckCallback& callback) {
-  // TODO(treib): Hack: For now, allow all Google URLs to save search QPS. If we
-  // ever remove this, we should find a way to allow at least the NTP.
+  // TODO(treib): Hack: For now, allow all Google URLs to save QPS. If we ever
+  // remove this, we should find a way to allow at least the NTP.
   if (google_util::IsGoogleDomainUrl(url,
                                      google_util::ALLOW_SUBDOMAIN,
                                      google_util::ALLOW_NON_STANDARD_PORTS)) {
@@ -149,11 +153,17 @@ bool SupervisedUserAsyncURLChecker::CheckURL(const GURL& url,
   auto cache_it = cache_.Get(url);
   if (cache_it != cache_.end()) {
     const CheckResult& result = cache_it->second;
-    DVLOG(1) << "Cache hit! " << url.spec() << " is "
-             << (result.behavior == SupervisedUserURLFilter::BLOCK ? "NOT" : "")
-             << " safe; certain: " << !result.uncertain;
-    callback.Run(url, result.behavior, result.uncertain);
-    return true;
+    base::TimeDelta age = base::TimeTicks::Now() - result.timestamp;
+    if (age < cache_timeout_) {
+      DVLOG(1) << "Cache hit! " << url.spec() << " is "
+               << (result.behavior == SupervisedUserURLFilter::BLOCK ? "NOT"
+                                                                     : "")
+               << " safe; certain: " << !result.uncertain;
+      callback.Run(url, result.behavior, result.uncertain);
+      return true;
+    }
+    DVLOG(1) << "Outdated cache entry for " << url.spec() << ", purging";
+    cache_.Erase(cache_it);

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

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?

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

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

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

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]

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

   }
 
   // See if we already have a check in progress for this URL.
@@ -201,7 +211,7 @@ void SupervisedUserAsyncURLChecker::OnURLFetchComplete(
       is_porn ? SupervisedUserURLFilter::BLOCK : SupervisedUserURLFilter::ALLOW;
 
   UMA_HISTOGRAM_TIMES("ManagedUsers.SafeSitesDelay",
-                      base::Time::Now() - check->start_time);
+                      base::TimeTicks::Now() - check->start_time);
 
   cache_.Put(check->url, CheckResult(behavior, uncertain));