Redirect handling in resource prefetch predictor

chrome/browser/predictors/resource_prefetch_predictor_tables.h

Index: chrome/browser/predictors/resource_prefetch_predictor_tables.h
diff --git a/chrome/browser/predictors/resource_prefetch_predictor_tables.h b/chrome/browser/predictors/resource_prefetch_predictor_tables.h
index 953c23f1c553437fb7354050c2bc7bb7b3a2c521..bc63d5c095e9e48b90178777c937b79bd589f9b6 100644
--- a/chrome/browser/predictors/resource_prefetch_predictor_tables.h
+++ b/chrome/browser/predictors/resource_prefetch_predictor_tables.h
@@ -28,6 +28,8 @@ class Statement;
 namespace predictors {
 
 using chrome_browser_predictors::ResourceData;
+using chrome_browser_predictors::RedirectProto;
+using RedirectStat = chrome_browser_predictors::RedirectProto::RedirectStat;
 
 // Interface for database tables used by the ResourcePrefetchPredictor.
 // All methods except the constructor and destructor need to be called on the DB
@@ -36,8 +38,10 @@ using chrome_browser_predictors::ResourceData;
 // Currently manages:
 //  - UrlResourceTable - resources per Urls.
 //  - UrlMetadataTable - misc data for Urls (like last visit time).
+//  - UrlRedirectTable - redirects per Urls.
 //  - HostResourceTable - resources per host.
 //  - HostMetadataTable - misc data for hosts.
+//  - HostRedirectTable - redirects per host.
 class ResourcePrefetchPredictorTables : public PredictorTableBase {
  public:
   // Used in the UrlResourceTable and HostResourceTable to store resources
@@ -72,10 +76,9 @@ class ResourcePrefetchPredictorTables : public PredictorTableBase {
     // Not stored.
     float score;
   };
-  typedef std::vector<ResourceRow> ResourceRows;
 
   // Sorts the resource rows by score, decreasing.
-  static void SortResourceRows(ResourceRows* rows);
+  static void SortResourceRows(std::vector<ResourceRow>* rows);
 
   // Aggregated data for a Url or Host. Although the data differs slightly, we
   // store them in the same structure, because most of the fields are common and
@@ -93,29 +96,89 @@ class ResourcePrefetchPredictorTables : public PredictorTableBase {
     std::string primary_key;  // is_host() ? main frame url : host.
 
     base::Time last_visit;
-    ResourceRows resources;
+    std::vector<ResourceRow> resources;
   };
   // Map from primary key to PrefetchData for the key.
   typedef std::map<std::string, PrefetchData> PrefetchDataMap;
 
+  // Used in the UrlRedirectTable and HostRedirectTable to store redirects

[mattcary, 2016/09/15 09:40:47]

For example of how to simplify everything, the only difference between a
RedirectRow and the proto seems to be the score? That hardly seems worth all the
marshelling for. The score could be in the proto, and cleared before storing, or
stored in a parallel map with the caches.

[alexilin, 2016/09/15 10:51:49]

I didn't consider that the use of proto-generated types is allowed in business
code. However, I see a few pros and cons.

Pros:
- We get rid of one layer of abstraction. Less conversions, less code.

Cons:
- We don't have control over automatically generated type. We couldn't add own
methods, fields which should not be stored directly.
- The type could be used in some higher level classes which shouldn't know much
storage structure and be dependent on protobuf library.
- Proto-generated types could contain only restricted list of types (I don't
talk about other user-defined proto messages, enums, etc.), for example
base::Time should be converted into uint64.
- We probably don't want to have a deal with proto-style containers.

Of course, we have a simple structure for now, but I wrote all of this because
we could expand RedirectRow structure in the future and face a some of these
problems.

[mattcary, 2016/09/15 11:44:27]

Yes, exactly. This is a pretty big pro. All the boilerplate conversion from the
DB to an internal structure is annoying and error-prone.
I don't think this is much of a problem. Generally one doesn't want to add much
that isn't stored anyway. Not being able to extend the class also isn't usually
a problem in practice. The idea is to use a proto instead of a struct---we
wouldn't want to add a lot of machinery to a struct, either.
Being dependent on the protobuf library isn't a problem---it's already linked
in---and anyway most use is going to be through the tables class. To put it a
different way, RedirectRow is very implementation dependent so we wouldn't want
it to leak too far out either.
This also turns out not to be a big deal. In fact, it is something of an
advantage, as anything will have to be serialized to store in the DB anyway, so
we wouldn't want to use a complex type anyway. 
I'm not sure what you mean here. Again, the issue is that we can't serialize
complex data structures anyway, so we'd want to think carefully about relying on
containers. We have to build an index to anything we pull out of the DB.
That's a good thing to think of generally, but in this case because RedirectRow
is something that is going to be serialized into the DB, we have essential
constraints on how we'd expand the type in the future.

To put it a different way, this sort of situation (data that is serialized and
also manipulated) is exactly what protos were created for, and so it's likely
that long-term usage will work out okay as well.

[alexilin, 2016/09/15 17:56:16]

Yeah, now I see that this is even good if we treat the struct as POD. You know
everything about this struct after you looked at its proto declaration. Keeps it
simple and clear for readers.
I've meant "repeated" fields in proto. For example, if we get rid of
RedirectData struct (this is wrapper over RedirectProto message) then users of
this class will have to use proto-style accessors to manipulate repeated data
instead of std::vector.
 
Great, thanks for the explanation! So, should I get rid of all similar classes?
(ResourceRow, ResourceData, RedirectData)

+  // required for the page or host.
+  struct RedirectRow {
+    RedirectRow();
+    RedirectRow(const RedirectRow& other);
+    RedirectRow(const std::string& url,
+                size_t number_of_hits,
+                size_t number_of_misses,
+                size_t consecutive_misses);
+    void UpdateScore();
+    bool operator==(const RedirectRow& rhs) const;
+    static void FromProto(const RedirectStat& proto, RedirectRow* row);
+    void ToProto(RedirectStat* proto) const;
+
+    std::string url;
+    size_t number_of_hits;
+    size_t number_of_misses;
+    size_t consecutive_misses;
+
+    // Not stored.
+    float score;
+  };
+
+  // Sorts the redirect rows by score, decreasing.
+  static void SortRedirectRows(std::vector<RedirectRow>* rows);
+
+  // Aggregated data about redirections for a Url or Host.
+  struct RedirectData {
+    RedirectData(PrefetchKeyType key_type,
+                 const std::string& primary_key = std::string());
+    RedirectData(const RedirectData& other);
+    ~RedirectData();
+    bool operator==(const RedirectData& rhs) const;
+    static void FromProto(const RedirectProto& proto, RedirectData* data);
+    void ToProto(RedirectProto* proto) const;
+
+    bool is_host() const { return key_type == PREFETCH_KEY_TYPE_HOST; }
+
+    PrefetchKeyType key_type;
+    std::string primary_key;
+    base::Time last_visit;
+    std::vector<RedirectRow> redirects;
+  };
+  // Map from primary key to RedirectData for the key.
+  typedef std::map<std::string, RedirectData> RedirectDataMap;
+
   // Returns data for all Urls and Hosts.
   virtual void GetAllData(PrefetchDataMap* url_data_map,
-                          PrefetchDataMap* host_data_map);
+                          PrefetchDataMap* host_data_map,
+                          RedirectDataMap* url_redirect_data_map,
+                          RedirectDataMap* host_redirect_data_map);
 
   // Updates data for a Url and a host. If either of the |url_data| or
-  // |host_data| has an empty primary key, it will be ignored.
-  // Note that the Urls and primary key in |url_data| and |host_data| should be
-  // less than |kMaxStringLength| in length.
+  // |host_data| or |url_redirect_data| or |host_redirect_data| has an empty
+  // primary key, it will be ignored.
+  // Note that the Urls and primary key in |url_data|, |host_data|,
+  // |url_redirect_data| and |host_redirect_data| should be less than
+  // |kMaxStringLength| in length.
   virtual void UpdateData(const PrefetchData& url_data,
-                          const PrefetchData& host_data);
+                          const PrefetchData& host_data,
+                          const RedirectData& url_redirect_data,
+                          const RedirectData& host_redirect_data);
+
+  // Delete data for the input |urls| and |hosts|.
+  virtual void DeleteResourceData(const std::vector<std::string>& urls,
+                                  const std::vector<std::string>& hosts);
+
+  // Wrapper over DeleteResourceData for convenience.
+  virtual void DeleteSingleResourceDataPoint(const std::string& key,
+                                             PrefetchKeyType key_type);
 
   // Delete data for the input |urls| and |hosts|.
-  virtual void DeleteData(const std::vector<std::string>& urls,
-                  const std::vector<std::string>& hosts);
+  virtual void DeleteRedirectData(const std::vector<std::string>& urls,
+                                  const std::vector<std::string>& hosts);
 
-  // Wrapper over DeleteData for convenience.
-  virtual void DeleteSingleDataPoint(const std::string& key,
-                                     PrefetchKeyType key_type);
+  // Wrapper over DeleteRedirectData for convenience.
+  virtual void DeleteSingleRedirectDataPoint(const std::string& key,
+                                             PrefetchKeyType key_type);
 
   // Deletes all data in all the tables.
   virtual void DeleteAllData();
@@ -136,16 +199,22 @@ class ResourcePrefetchPredictorTables : public PredictorTableBase {
 
   // Helper functions below help perform functions on the Url and host table
   // using the same code.
-  void GetAllDataHelper(PrefetchKeyType key_type,
-                        PrefetchDataMap* data_map,
-                        std::vector<std::string>* to_delete);
-  bool UpdateDataHelper(const PrefetchData& data);
-  void DeleteDataHelper(PrefetchKeyType key_type,
-                        const std::vector<std::string>& keys);
+  void GetAllResourceDataHelper(PrefetchKeyType key_type,
+                                PrefetchDataMap* data_map,
+                                std::vector<std::string>* to_delete);
+  void GetAllRedirectDataHelper(PrefetchKeyType key_type,
+                                RedirectDataMap* redirect_map);
+  bool UpdateResourceDataHelper(const PrefetchData& data);
+  bool UpdateRedirectDataHelper(const RedirectData& data);
+  void DeleteResourceDataHelper(PrefetchKeyType key_type,
+                                const std::vector<std::string>& keys);
+  void DeleteRedirectDataHelper(PrefetchKeyType key_type,
+                                const std::vector<std::string>& keys);
 
   // Returns true if the strings in the |data| are less than |kMaxStringLength|
   // in length.
   static bool StringsAreSmallerThanDBLimit(const PrefetchData& data);
+  static bool StringsAreSmallerThanDBLimit(const RedirectData& data);
 
   // PredictorTableBase methods.
   void CreateTableIfNonExistent() override;
@@ -153,7 +222,7 @@ class ResourcePrefetchPredictorTables : public PredictorTableBase {
 
   // Database version. Always increment it when any change is made to the data
   // schema (including the .proto).
-  static constexpr int kDatabaseVersion = 2;
+  static constexpr int kDatabaseVersion = 3;
 
   static bool DropTablesIfOutdated(sql::Connection* db);
   static int GetDatabaseVersion(sql::Connection* db);
@@ -165,11 +234,15 @@ class ResourcePrefetchPredictorTables : public PredictorTableBase {
   sql::Statement* GetUrlResourceUpdateStatement();
   sql::Statement* GetUrlMetadataDeleteStatement();
   sql::Statement* GetUrlMetadataUpdateStatement();
+  sql::Statement* GetUrlRedirectDeleteStatement();
+  sql::Statement* GetUrlRedirectUpdateStatement();
 
   sql::Statement* GetHostResourceDeleteStatement();
   sql::Statement* GetHostResourceUpdateStatement();
   sql::Statement* GetHostMetadataDeleteStatement();
   sql::Statement* GetHostMetadataUpdateStatement();
+  sql::Statement* GetHostRedirectDeleteStatement();
+  sql::Statement* GetHostRedirectUpdateStatement();
 
   DISALLOW_COPY_AND_ASSIGN(ResourcePrefetchPredictorTables);
 };