sql: Feature flag for in-memory temporary storage.

This CL introduces SqlTempStoreMemory flag, which is intended to be
managed via Finch. When set, the flag causes sql::Database to run a
PRAGMA temp_store=MEMORY [1] query on every opened database.

This approach is intended to approximate the effect of building SQLite
with the SQLITE_TEMP_STORE=3 [2] macro, to measure the memory
consumption impact. Ideally, we'd test the macro directly, but //sql is
a core component of Chrome, so we can't load different versions of it
based on Finch.

[1] https://www.sqlite.org/pragma.html#pragma_temp_store
[2] https://www.sqlite.org/compile.html#temp_store

Bug: 875538
Change-Id: I537d90d763be1100503ed4bd2ada2ee19eb090bb
Reviewed-on: https://chromium-review.googlesource.com/1180530Reviewed-by: Mark Pearson <mpearson@chromium.org>
Reviewed-by: Chris Mumford <cmumford@chromium.org>
Commit-Queue: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584652}

chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc

@@ -178,9 +178,24 @@ class TestingOmniboxEditController : public ChromeOmniboxEditController {
 
 // OmniboxViewViewsTest -------------------------------------------------------
 
-class OmniboxViewViewsTest : public ChromeViewsTestBase {
+// Base class that ensures ScopedFeatureList is initialized first.
+class OmniboxViewViewsTestBase : public ChromeViewsTestBase {
  public:
-  OmniboxViewViewsTest();
+  explicit OmniboxViewViewsTestBase(
+      const std::vector<base::Feature>& enabled_features) {
+    scoped_feature_list_.InitWithFeatures(enabled_features, {});
+  }
+
+ protected:
+  base::test::ScopedFeatureList scoped_feature_list_;
+};
+
+class OmniboxViewViewsTest : public OmniboxViewViewsTestBase {
+ public:
+  explicit OmniboxViewViewsTest(
+      const std::vector<base::Feature>& enabled_features);
+
+  OmniboxViewViewsTest() : OmniboxViewViewsTest(std::vector<base::Feature>()) {}
 
   TestToolbarModel* toolbar_model() { return &toolbar_model_; }
   TestingOmniboxView* omnibox_view() const { return omnibox_view_; }
@@ -226,8 +241,10 @@ class OmniboxViewViewsTest : public ChromeViewsTestBase {
   DISALLOW_COPY_AND_ASSIGN(OmniboxViewViewsTest);
 };
 
-OmniboxViewViewsTest::OmniboxViewViewsTest()
-    : util_(&profile_),
+OmniboxViewViewsTest::OmniboxViewViewsTest(
+    const std::vector<base::Feature>& enabled_features)
+    : OmniboxViewViewsTestBase(enabled_features),
+      util_(&profile_),
       command_updater_(nullptr),
       omnibox_edit_controller_(&command_updater_, &toolbar_model_) {}
 
@@ -446,14 +463,16 @@ TEST_F(OmniboxViewViewsTest, RevertOnBlur) {
 }
 
 class OmniboxViewViewsSteadyStateElisionsTest : public OmniboxViewViewsTest {
+ public:
+  OmniboxViewViewsSteadyStateElisionsTest()
+      : OmniboxViewViewsTest(
+            {omnibox::kUIExperimentHideSteadyStateUrlSchemeAndSubdomains}) {}
+
  protected:
   const int kCharacterWidth = 10;
   const base::string16 kFullUrl = base::ASCIIToUTF16("https://www.example.com");
 
   void SetUp() override {
-    scoped_feature_list_.InitAndEnableFeature(
-        omnibox::kUIExperimentHideSteadyStateUrlSchemeAndSubdomains);
-
     OmniboxViewViewsTest::SetUp();
 
     // Advance 5 seconds from epoch so the time is not considered null.
@@ -524,7 +543,6 @@ class OmniboxViewViewsSteadyStateElisionsTest : public OmniboxViewViewsTest {
 
  private:
   test::ScopedMacViewsBrowserMode views_mode_{true};
-  base::test::ScopedFeatureList scoped_feature_list_;
   base::SimpleTestTickClock clock_;
 };
 

sql/BUILD.gn

@@ -21,6 +21,8 @@ component("sql") {
     "recovery.cc",
     "recovery.h",
     "sql_export.h",
+    "sql_features.cc",
+    "sql_features.h",
     "sql_memory_dump_provider.cc",
     "sql_memory_dump_provider.h",
     "statement.cc",

sql/database.cc

@@ -33,6 +33,7 @@
 #include "sql/database_memory_dump_provider.h"
 #include "sql/initialization.h"
 #include "sql/meta_table.h"
+#include "sql/sql_features.h"
 #include "sql/statement.h"
 #include "sql/vfs_wrapper.h"
 #include "third_party/sqlite/sqlite3.h"
@@ -1674,6 +1675,12 @@ bool Database::OpenInternal(const std::string& file_name,
     ignore_result(Execute("PRAGMA locking_mode=EXCLUSIVE"));
   }
 
+  if (base::FeatureList::IsEnabled(features::kSqlTempStoreMemory)) {
+    err = ExecuteAndReturnErrorCode("PRAGMA temp_store=MEMORY");
+    // This operates on in-memory configuration, so it should not fail.
+    DCHECK_EQ(err, SQLITE_OK) << "Failed switching to in-RAM temporary storage";
+  }
+
   // http://www.sqlite.org/pragma.html#pragma_journal_mode
   // DELETE (default) - delete -journal file to commit.
   // TRUNCATE - truncate -journal file to commit.
@@ -1681,7 +1688,7 @@ bool Database::OpenInternal(const std::string& file_name,
   // TRUNCATE should be faster than DELETE because it won't need directory
   // changes for each transaction.  PERSIST may break the spirit of using
   // secure_delete.
-  ignore_result(Execute("PRAGMA journal_mode = TRUNCATE"));
+  ignore_result(Execute("PRAGMA journal_mode=TRUNCATE"));
 
   const base::TimeDelta kBusyTimeout =
       base::TimeDelta::FromSeconds(kBusyTimeoutSeconds);
@@ -1728,7 +1735,7 @@ bool Database::OpenInternal(const std::string& file_name,
   // 64-bit platforms.
   size_t mmap_size = mmap_disabled_ ? 0 : GetAppropriateMmapSize();
   std::string mmap_sql =
-      base::StringPrintf("PRAGMA mmap_size = %" PRIuS, mmap_size);
+      base::StringPrintf("PRAGMA mmap_size=%" PRIuS, mmap_size);
   ignore_result(Execute(mmap_sql.c_str()));
 
   // Determine if memory-mapping has actually been enabled.  The Execute() above
@@ -1889,7 +1896,7 @@ bool Database::IntegrityCheckHelper(const char* pragma_sql,
   // allows SQLite to process through certain cases of corruption.
   // Failing to set this pragma probably means that the database is
   // beyond recovery.
-  static const char kWritableSchemaSql[] = "PRAGMA writable_schema = ON";
+  static const char kWritableSchemaSql[] = "PRAGMA writable_schema=ON";
   if (!Execute(kWritableSchemaSql))
     return false;
 
@@ -1909,7 +1916,7 @@ bool Database::IntegrityCheckHelper(const char* pragma_sql,
   }
 
   // Best effort to put things back as they were before.
-  static const char kNoWritableSchemaSql[] = "PRAGMA writable_schema = OFF";
+  static const char kNoWritableSchemaSql[] = "PRAGMA writable_schema=OFF";
   ignore_result(Execute(kNoWritableSchemaSql));
 
   return ret;

sql/database_unittest.cc

@@ -13,12 +13,14 @@
 #include "base/macros.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/test/metrics/histogram_tester.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/test/simple_test_tick_clock.h"
 #include "base/trace_event/process_memory_dump.h"
 #include "build/build_config.h"
 #include "sql/database.h"
 #include "sql/database_memory_dump_provider.h"
 #include "sql/meta_table.h"
+#include "sql/sql_features.h"
 #include "sql/statement.h"
 #include "sql/test/error_callback_support.h"
 #include "sql/test/scoped_error_expecter.h"
@@ -100,7 +102,6 @@ class ScopedCommitHook {
 
 namespace {
 
-using sql::test::ExecuteWithResults;
 using sql::test::ExecuteWithResult;
 
 // Helper to return the count of items in sqlite_master.  Return -1 in
@@ -1618,4 +1619,22 @@ TEST_F(SQLDatabaseTest, CompileError) {
 #endif  // !defined(OS_ANDROID) && !defined(OS_IOS) && !defined(OS_FUCHSIA)
 }
 
+// Verify that Raze() can handle an empty file.  SQLite should treat
+// this as an empty database.
+TEST_F(SQLDatabaseTest, SqlTempMemoryFeatureFlagDefault) {
+  EXPECT_EQ("0", ExecuteWithResult(&db(), "PRAGMA temp_store"))
+      << "temp_store should not be set by default";
+}
+
+TEST_F(SQLDatabaseTest, SqlTempMemoryFeatureFlagEnabled) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(features::kSqlTempStoreMemory);
+
+  db().Close();
+
+  ASSERT_TRUE(db().Open(db_path()));
+  EXPECT_EQ("2", ExecuteWithResult(&db(), "PRAGMA temp_store"))
+      << "temp_store should be set by the feature flag SqlTempStoreMemory";
+}
+
 }  // namespace sql

sql/sql_features.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "sql/sql_features.h"
+
+namespace sql {
+
+namespace features {
+
+// SQLite databases only use RAM for temporary storage.
+//
+// Enabling this feature matches the SQLITE_TEMP_STORE=3 build option, which is
+// used on Android.
+//
+// TODO(pwnall): After the memory impact of the config change is assessed, land
+//               https://crrev.com/c/1146493 and remove this flag.
+const base::Feature kSqlTempStoreMemory{"SqlTempStoreMemory",
+                                        base::FEATURE_DISABLED_BY_DEFAULT};
+
+}  // namespace features
+
+}  // namespace sql

sql/sql_features.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef SQL_SQL_FEATURES_H_
+#define SQL_SQL_FEATURES_H_
+
+#include "base/feature_list.h"
+#include "sql/sql_export.h"
+
+namespace sql {
+
+namespace features {
+
+SQL_EXPORT extern const base::Feature kSqlTempStoreMemory;
+
+}  // namespace features
+
+}  // namespace sql
+
+#endif  // SQL_SQL_FEATURES_H_