predictors: Extract sql key-value tables into separate class.

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 0808fbad23ee3eb9668965ff898e1356e9215b9c..386661f1941c89afa2a7c5dc92913524f786d92e 100644
--- a/chrome/browser/predictors/resource_prefetch_predictor_tables.h
+++ b/chrome/browser/predictors/resource_prefetch_predictor_tables.h
@@ -13,15 +13,12 @@
 
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
+#include "chrome/browser/predictors/glowplug_key_value_table.h"
 #include "chrome/browser/predictors/predictor_table_base.h"
 #include "chrome/browser/predictors/resource_prefetch_common.h"
 #include "chrome/browser/predictors/resource_prefetch_predictor.pb.h"
 #include "components/precache/core/proto/precache.pb.h"
 
-namespace sql {
-class Statement;
-}
-
 namespace predictors {
 
 // From resource_prefetch_predictor.proto.
@@ -32,10 +29,13 @@ using RedirectStat = RedirectData_RedirectStat;
 // thread.
 //
 // Currently manages:
-//  - UrlResourceTable - resources per Urls.
-//  - UrlRedirectTable - redirects per Urls.
-//  - HostResourceTable - resources per host.
-//  - HostRedirectTable - redirects per host.
+//  - UrlResourceTable - key: url, value: PrefetchData
+//  - UrlRedirectTable - key: url, value: RedirectData
+//  - HostResourceTable - key: host, value: PrefetchData
+//  - HostRedirectTable - key: host, value: RedirectData
+//  - ManifestTable - key: host with stripped "www." prefix,
+//                    value: precache::PrecacheManifest
+//  - OriginTable - key: host, value: OriginData
 class ResourcePrefetchPredictorTables : public PredictorTableBase {
  public:
   typedef std::map<std::string, PrefetchData> PrefetchDataMap;
@@ -130,11 +130,6 @@ class ResourcePrefetchPredictorTables : public PredictorTableBase {
   ~ResourcePrefetchPredictorTables() override;
 
  private:
-  // Represents the type of information that is stored in prefetch database.
-  enum class PrefetchDataType { RESOURCE, REDIRECT, MANIFEST, ORIGIN };
-
-  enum class TableOperationType { INSERT, REMOVE };
-
   friend class PredictorDatabaseInternal;
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTablesTest,
                            DatabaseVersionIsSet);
@@ -145,23 +140,6 @@ class ResourcePrefetchPredictorTables : public PredictorTableBase {
   // schema (including the .proto).
   static constexpr int kDatabaseVersion = 7;
 
-  // Helper functions below help perform functions on the Url and host table
-  // using the same code.
-  void GetAllResourceDataHelper(PrefetchKeyType key_type,
-                                PrefetchDataMap* data_map);
-  void GetAllRedirectDataHelper(PrefetchKeyType key_type,
-                                RedirectDataMap* redirect_map);
-  void GetAllManifestDataHelper(ManifestDataMap* manifest_map);
-  void GetAllOriginDataHelper(OriginDataMap* manifest_map);
-
-  void UpdateDataHelper(PrefetchKeyType key_type,
-                        PrefetchDataType data_type,
-                        const std::string& key,
-                        const google::protobuf::MessageLite& data);
-  void DeleteDataHelper(PrefetchKeyType key_type,
-                        PrefetchDataType data_type,
-                        const std::vector<std::string>& keys);
-
   // PredictorTableBase:
   void CreateTableIfNonExistent() override;
   void LogDatabaseStats() override;
@@ -170,14 +148,13 @@ class ResourcePrefetchPredictorTables : public PredictorTableBase {
   static int GetDatabaseVersion(sql::Connection* db);
   static bool SetDatabaseVersion(sql::Connection* db, int version);
 
-  // Helper to return cached Statements.
-  std::unique_ptr<sql::Statement> GetTableUpdateStatement(

[Benoit L, 2017/04/27 15:49:03]

We lose the cached statements, does it matter?

[alexilin, 2017/04/28 14:54:52]

I wrote a little benchmark that creates sql::Statement 10000 times in a loop:
* Using GetCachedStatement: 0.154616 s
* Using GetUniqueStatement: 0.776918 s
* Cache the result of GetUniqueStatement manually: 0.10126 s

So the most efficient way would be to cache the statements inside the class.
Don't know whether this behavior supported by sql library.
I mean, that's how GetCachedStatement function works, it just saves result of
GetUniqueStatement in a map. But implementation details could be changed.

Using of GetCachedStatement requires passing some unique identifier inside of
GlowplugKeyValueTable class, that doesn't look great.

WDYT?

[Benoit L, 2017/04/28 15:12:34]

Re-using a unique statement is OK I believe, as it's explicitely OK to bind a
statement to different values.
Cached statement looks heavyweight indeed, what about a lazily initialized
statement?

[alexilin, 2017/04/28 16:27:26]

It turned out that we can't store cached statements inside of the class because
the StatementRef is defined in the private section of sql::Connection.

-      PrefetchKeyType key_type,
-      PrefetchDataType data_type,
-      TableOperationType op_type);
-
-  static const char* GetTableName(PrefetchKeyType key_type,
-                                  PrefetchDataType data_type);
+  std::unique_ptr<GlowplugKeyValueTable<PrefetchData>> url_resource_table_;
+  std::unique_ptr<GlowplugKeyValueTable<RedirectData>> url_redirect_table_;
+  std::unique_ptr<GlowplugKeyValueTable<PrefetchData>> host_resource_table_;
+  std::unique_ptr<GlowplugKeyValueTable<RedirectData>> host_redirect_table_;
+  std::unique_ptr<GlowplugKeyValueTable<precache::PrecacheManifest>>
+      manifest_table_;
+  std::unique_ptr<GlowplugKeyValueTable<OriginData>> origin_table_;
 
   DISALLOW_COPY_AND_ASSIGN(ResourcePrefetchPredictorTables);
 };