predictors: Refactor resource_prefetch_predictor_tables.

chrome/browser/predictors/resource_prefetch_predictor.cc

Index: chrome/browser/predictors/resource_prefetch_predictor.cc
diff --git a/chrome/browser/predictors/resource_prefetch_predictor.cc b/chrome/browser/predictors/resource_prefetch_predictor.cc
index bc8013823e45f315042e702e7e876b2c08fe3aae..796094eb130c08b92ef04f55b77408c59e54693c 100644
--- a/chrome/browser/predictors/resource_prefetch_predictor.cc
+++ b/chrome/browser/predictors/resource_prefetch_predictor.cc
@@ -527,7 +527,7 @@ bool ResourcePrefetchPredictor::GetPrefetchData(
 void ResourcePrefetchPredictor::PopulatePrefetcherRequest(
     const PrefetchData& data,
     std::vector<GURL>* urls) {
-  for (const ResourceData& resource : data.resources) {
+  for (const ResourceData& resource : data.resources()) {
     float confidence =
         static_cast<float>(resource.number_of_hits()) /
         (resource.number_of_hits() + resource.number_of_misses());
@@ -715,13 +715,13 @@ void ResourcePrefetchPredictor::RemoveOldestEntryInPrefetchDataMap(
   if (data_map->empty())
     return;
 
-  base::Time oldest_time;
+  uint64_t oldest_time = UINT64_MAX;

[pasko, 2016/10/04 16:12:16]

nit: max_possible_time

("oldest" suggests epoch to me ..)

[alexilin, 2016/10/04 17:25:46]

It's not a constant. And the value UINT64_MAX actually won't ever used (look at
the conditions below). But I was forced to initialize this variable to some
value by one of the compilers. Actually, value 0 is also suitable but it doesn't
look consistent int this context.
We are trying to find the entry with the minimum last visit time. Minimum time
-> oldest. I could rename it to 'minimum_time' if you wish.

[pasko, 2016/10/05 11:49:32]

ah, pardon, my bad, I missed the part that this is only an initial value

   std::string key_to_delete;
-  for (PrefetchDataMap::iterator it = data_map->begin();
-       it != data_map->end(); ++it) {
-    if (key_to_delete.empty() || it->second.last_visit < oldest_time) {
-      key_to_delete = it->first;
-      oldest_time = it->second.last_visit;
+  for (const auto& kv : *data_map) {
+    const PrefetchData& data = kv.second;
+    if (key_to_delete.empty() || data.last_visit_time() < oldest_time) {
+      key_to_delete = data.primary_key();
+      oldest_time = data.last_visit_time();
     }
   }
 
@@ -813,32 +813,33 @@ void ResourcePrefetchPredictor::LearnNavigation(
     if (data_map->size() >= max_data_map_size)
       RemoveOldestEntryInPrefetchDataMap(key_type, data_map);
 
-    cache_entry = data_map->insert(std::make_pair(
-        key, PrefetchData(key_type, key))).first;
-    cache_entry->second.last_visit = base::Time::Now();
+    cache_entry = data_map->insert(std::make_pair(key, PrefetchData())).first;
+    cache_entry->second.set_primary_key(key);

[Benoit L, 2016/10/04 15:43:57]

nit: Seems like the code would be a bit easier to read by introducing:

PrefetchData& prefetch_data = cache_entry->second;

wdyt?

[pasko, 2016/10/04 16:12:16]

+1

[alexilin, 2016/10/04 17:25:46]

Yeah, I was thinking about it but it seemed that we would have to introduce this
in 3 different scopes. And I didn't like it then.
But I tried your suggestion and now it looks better, thanks.

+    cache_entry->second.set_last_visit_time(
+        base::Time::Now().ToInternalValue());
     size_t new_resources_size = new_resources.size();
     std::set<GURL> resources_seen;
     for (size_t i = 0; i < new_resources_size; ++i) {
-      if (resources_seen.find(new_resources[i].resource_url) !=
-          resources_seen.end()) {
+      const URLRequestSummary& summary = new_resources[i];
+      if (resources_seen.find(summary.resource_url) != resources_seen.end())
         continue;
-      }
-      ResourceData resource_to_add;
-      resource_to_add.set_resource_url(new_resources[i].resource_url.spec());
-      resource_to_add.set_resource_type(static_cast<ResourceData::ResourceType>(
-          new_resources[i].resource_type));
-      resource_to_add.set_number_of_hits(1);
-      resource_to_add.set_average_position(i + 1);
-      resource_to_add.set_priority(
-          static_cast<ResourceData::Priority>(new_resources[i].priority));
-      resource_to_add.set_has_validators(new_resources[i].has_validators);
-      resource_to_add.set_always_revalidate(new_resources[i].always_revalidate);
-      cache_entry->second.resources.push_back(resource_to_add);
-      resources_seen.insert(new_resources[i].resource_url);
+
+      ResourceData* resource_to_add = cache_entry->second.add_resources();
+      resource_to_add->set_resource_url(summary.resource_url.spec());
+      resource_to_add->set_resource_type(
+          static_cast<ResourceData::ResourceType>(summary.resource_type));
+      resource_to_add->set_number_of_hits(1);
+      resource_to_add->set_average_position(i + 1);
+      resource_to_add->set_priority(
+          static_cast<ResourceData::Priority>(summary.priority));
+      resource_to_add->set_has_validators(summary.has_validators);
+      resource_to_add->set_always_revalidate(summary.always_revalidate);
+
+      resources_seen.insert(summary.resource_url);
     }
   } else {
-    std::vector<ResourceData>& old_resources = cache_entry->second.resources;
-    cache_entry->second.last_visit = base::Time::Now();
+    cache_entry->second.set_last_visit_time(

[Benoit L, 2016/10/04 15:43:57]

nit: Same here.

[alexilin, 2016/10/04 17:25:46]

Done.

+        base::Time::Now().ToInternalValue());
 
     // Build indices over the data.
     std::map<GURL, int> new_index, old_index;
@@ -849,9 +850,10 @@ void ResourcePrefetchPredictor::LearnNavigation(
       if (new_index.find(summary.resource_url) == new_index.end())
         new_index[summary.resource_url] = i;
     }
-    int old_resources_size = static_cast<int>(old_resources.size());
+    int old_resources_size =
+        static_cast<int>(cache_entry->second.resources_size());
     for (int i = 0; i < old_resources_size; ++i) {
-      const ResourceData& resource = old_resources[i];
+      const ResourceData& resource = cache_entry->second.resources(i);
       GURL resource_url(resource.resource_url());

[Benoit L, 2016/10/04 15:43:57]

tiny nit: the variable resource is used exactly once, and doesn't really improve
readability to me.

[alexilin, 2016/10/04 17:25:46]

Twice, actually.
Second time as a key in old_index map.
But it could be replaced by map::insert function and checking its return value.
1 lookup instead of 2 (in DEBUG mode). PROFIT!

       DCHECK(old_index.find(resource_url) == old_index.end());
       old_index[resource_url] = i;
@@ -859,33 +861,35 @@ void ResourcePrefetchPredictor::LearnNavigation(
 
     // Go through the old urls and update their hit/miss counts.
     for (int i = 0; i < old_resources_size; ++i) {
-      ResourceData& old_resource = old_resources[i];
-      GURL resource_url(old_resource.resource_url());
+      ResourceData* old_resource = cache_entry->second.mutable_resources(i);
+      GURL resource_url(old_resource->resource_url());
       if (new_index.find(resource_url) == new_index.end()) {
-        old_resource.set_number_of_misses(old_resource.number_of_misses() + 1);
-        old_resource.set_consecutive_misses(old_resource.consecutive_misses() +
-                                            1);
+        old_resource->set_number_of_misses(old_resource->number_of_misses() +
+                                           1);
+        old_resource->set_consecutive_misses(
+            old_resource->consecutive_misses() + 1);
       } else {
         const URLRequestSummary& new_summary =
             new_resources[new_index[resource_url]];
 
         // Update the resource type since it could have changed.
-        if (new_summary.resource_type != content::RESOURCE_TYPE_LAST_TYPE)
-          old_resource.set_resource_type(
+        if (new_summary.resource_type != content::RESOURCE_TYPE_LAST_TYPE) {
+          old_resource->set_resource_type(
               static_cast<ResourceData::ResourceType>(
                   new_summary.resource_type));
+        }
 
-        old_resource.set_priority(
+        old_resource->set_priority(
             static_cast<ResourceData::Priority>(new_summary.priority));
 
         int position = new_index[resource_url] + 1;
         int total =
-            old_resource.number_of_hits() + old_resource.number_of_misses();
-        old_resource.set_average_position(
-            ((old_resource.average_position() * total) + position) /
+            old_resource->number_of_hits() + old_resource->number_of_misses();
+        old_resource->set_average_position(
+            ((old_resource->average_position() * total) + position) /
             (total + 1));
-        old_resource.set_number_of_hits(old_resource.number_of_hits() + 1);
-        old_resource.set_consecutive_misses(0);
+        old_resource->set_number_of_hits(old_resource->number_of_hits() + 1);
+        old_resource->set_consecutive_misses(0);
       }
     }
 
@@ -896,17 +900,17 @@ void ResourcePrefetchPredictor::LearnNavigation(
         continue;
 
       // Only need to add new stuff.
-      ResourceData resource_to_add;
-      resource_to_add.set_resource_url(summary.resource_url.spec());
-      resource_to_add.set_resource_type(
+      ResourceData* resource_to_add = cache_entry->second.add_resources();
+      resource_to_add->set_resource_url(summary.resource_url.spec());
+      resource_to_add->set_resource_type(
           static_cast<ResourceData::ResourceType>(summary.resource_type));
-      resource_to_add.set_number_of_hits(1);
-      resource_to_add.set_average_position(i + 1);
-      resource_to_add.set_priority(
+      resource_to_add->set_number_of_hits(1);
+      resource_to_add->set_average_position(i + 1);
+      resource_to_add->set_priority(
           static_cast<ResourceData::Priority>(summary.priority));
-      resource_to_add.set_has_validators(new_resources[i].has_validators);
-      resource_to_add.set_always_revalidate(new_resources[i].always_revalidate);
-      old_resources.push_back(resource_to_add);
+      resource_to_add->set_has_validators(new_resources[i].has_validators);
+      resource_to_add->set_always_revalidate(
+          new_resources[i].always_revalidate);
 
       // To ensure we dont add the same url twice.
       old_index[summary.resource_url] = 0;
@@ -914,20 +918,19 @@ void ResourcePrefetchPredictor::LearnNavigation(
   }
 
   // Trim and sort the resources after the update.
-  std::vector<ResourceData>& resources = cache_entry->second.resources;
-  for (auto it = resources.begin(); it != resources.end();) {
-    if (it->consecutive_misses() >= config_.max_consecutive_misses)
-      it = resources.erase(it);
-    else
-      ++it;
+  ResourcePrefetchPredictorTables::TrimResources(
+      &(cache_entry->second), config_.max_consecutive_misses);
+  ResourcePrefetchPredictorTables::SortResources(&(cache_entry->second));
+  if (cache_entry->second.resources_size() >
+      static_cast<int>(config_.max_resources_per_entry)) {
+    cache_entry->second.mutable_resources()->DeleteSubrange(
+        config_.max_resources_per_entry,
+        cache_entry->second.resources_size() - config_.max_resources_per_entry);
   }
-  ResourcePrefetchPredictorTables::SortResources(&resources);
-  if (resources.size() > config_.max_resources_per_entry)
-    resources.resize(config_.max_resources_per_entry);
 
   // If the row has no resources, remove it from the cache and delete the
   // entry in the database. Else update the database.
-  if (resources.empty()) {
+  if (cache_entry->second.resources_size() == 0) {
     data_map->erase(key);
     BrowserThread::PostTask(
         BrowserThread::DB, FROM_HERE,
@@ -935,11 +938,9 @@ void ResourcePrefetchPredictor::LearnNavigation(
             &ResourcePrefetchPredictorTables::DeleteSingleResourceDataPoint,
             tables_, key, key_type));
   } else {
-    bool is_host = key_type == PREFETCH_KEY_TYPE_HOST;
-    PrefetchData empty_data(
-        !is_host ? PREFETCH_KEY_TYPE_HOST : PREFETCH_KEY_TYPE_URL,
-        std::string());
+    PrefetchData empty_data;
     RedirectData empty_redirect_data;
+    bool is_host = key_type == PREFETCH_KEY_TYPE_HOST;
     const PrefetchData& host_data = is_host ? cache_entry->second : empty_data;
     const PrefetchData& url_data = is_host ? empty_data : cache_entry->second;
     BrowserThread::PostTask(
@@ -1001,21 +1002,12 @@ void ResourcePrefetchPredictor::LearnRedirect(const std::string& key,
     }
   }
 
-  // Trim and sort redirects after update.
-  std::vector<RedirectStat> redirects;
-  redirects.reserve(cache_entry->second.redirect_endpoints_size());
-  for (const RedirectStat& redirect :
-       cache_entry->second.redirect_endpoints()) {
-    if (redirect.consecutive_misses() < config_.max_consecutive_misses)
-      redirects.push_back(redirect);
-  }
-  ResourcePrefetchPredictorTables::SortRedirects(&redirects);
-
-  cache_entry->second.clear_redirect_endpoints();
-  for (const RedirectStat& redirect : redirects)
-    cache_entry->second.add_redirect_endpoints()->CopyFrom(redirect);
+  // Trim and sort the redirects after the update.
+  ResourcePrefetchPredictorTables::TrimRedirects(
+      &(cache_entry->second), config_.max_consecutive_misses);
+  ResourcePrefetchPredictorTables::SortRedirects(&(cache_entry->second));
 
-  if (redirects.empty()) {
+  if (cache_entry->second.redirect_endpoints_size() == 0) {
     redirect_map->erase(cache_entry);
     BrowserThread::PostTask(
         BrowserThread::DB, FROM_HERE,
@@ -1023,10 +1015,9 @@ void ResourcePrefetchPredictor::LearnRedirect(const std::string& key,
             &ResourcePrefetchPredictorTables::DeleteSingleRedirectDataPoint,
             tables_, key, key_type));
   } else {
-    bool is_host = key_type == PREFETCH_KEY_TYPE_HOST;
     RedirectData empty_redirect_data;
-    PrefetchData empty_url_data(PREFETCH_KEY_TYPE_URL, std::string());
-    PrefetchData empty_host_data(PREFETCH_KEY_TYPE_HOST, std::string());
+    PrefetchData empty_data;
+    bool is_host = key_type == PREFETCH_KEY_TYPE_HOST;
     const RedirectData& host_redirect_data =
         is_host ? cache_entry->second : empty_redirect_data;
     const RedirectData& url_redirect_data =
@@ -1034,7 +1025,7 @@ void ResourcePrefetchPredictor::LearnRedirect(const std::string& key,
     BrowserThread::PostTask(
         BrowserThread::DB, FROM_HERE,
         base::Bind(&ResourcePrefetchPredictorTables::UpdateData, tables_,
-                   empty_url_data, empty_host_data, url_redirect_data,
+                   empty_data, empty_data, url_redirect_data,
                    host_redirect_data));
   }
 }