From 3ffd9a37315ca248ec018cf86f0371b3c595a749 Mon Sep 17 00:00:00 2001
From: Victor Costan <pwnall@chromium.org>
Date: Wed, 22 May 2019 00:46:54 +0000
Subject: [PATCH] sql: Add Finch trial for eliminating
 sql::Database::Preload().

The logic in sql::Database::Preload() is a pessimization for modern OS
schedulers, as it forces the kernel to read the first few pages of a
newly opened database. https://crbug.com/243949 suggests that this
method was added without performance testing, just like the more complex
SQLite patch that it replaces.

This CL sets up a Finch trial that will either prove that we can get rid
of the Preload() hack, or give us conclusive data about its usefulness.

Bug: 243949
Change-Id: Icfee9aaa3d125af207974536e8e5f5e64a9609ba
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1438031
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Chris Mumford <cmumford@google.com>
Commit-Queue: Steven Holte <holte@chromium.org>
Auto-Submit: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#662006}
---
 sql/database.cc                               |  4 ++++
 sql/sql_features.cc                           | 12 ++++++++++-
 sql/sql_features.h                            |  2 +-
 .../variations/fieldtrial_testing_config.json | 20 +++++++++++++++++++
 4 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/sql/database.cc b/sql/database.cc
index 802923ce412d75..3ef9c18dc3838a 100644
--- a/sql/database.cc
+++ b/sql/database.cc
@@ -31,6 +31,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"
@@ -346,6 +347,9 @@ void Database::Preload() {
   base::Optional<base::ScopedBlockingCall> scoped_blocking_call;
   InitScopedBlockingCall(&scoped_blocking_call);
 
+  if (base::FeatureList::IsEnabled(features::kSqlSkipPreload))
+    return;
+
   if (!db_) {
     DCHECK(poisoned_) << "Cannot preload null db";
     return;
diff --git a/sql/sql_features.cc b/sql/sql_features.cc
index 93d84e125533b2..833c43e00b0085 100644
--- a/sql/sql_features.cc
+++ b/sql/sql_features.cc
@@ -8,7 +8,17 @@ namespace sql {
 
 namespace features {
 
-// Flags for SQLite configuration experiments will be defined here.
+// Skip the logic for preloading databases.
+//
+// Enabling this feature turns sql::Database::Preload() into a noop.
+// https://crbug.com/243949 suggests that sql::Database::Preload() was added
+// without any proper benchmarking, and the logic is a pessimization for modern
+// OS schedulers.
+//
+// TODO(pwnall): After the performance impact of the change is assessed, remove
+//               sql::Database::Preload() and this flag.
+const base::Feature kSqlSkipPreload{"SqlSkipPreload",
+                                    base::FEATURE_DISABLED_BY_DEFAULT};
 
 }  // namespace features
 
diff --git a/sql/sql_features.h b/sql/sql_features.h
index 4acfc54fb32382..eee57706634ade 100644
--- a/sql/sql_features.h
+++ b/sql/sql_features.h
@@ -12,7 +12,7 @@ namespace sql {
 
 namespace features {
 
-// Flags for SQLite configuration experiments will be declared here.
+COMPONENT_EXPORT(SQL) extern const base::Feature kSqlSkipPreload;
 
 }  // namespace features
 
diff --git a/testing/variations/fieldtrial_testing_config.json b/testing/variations/fieldtrial_testing_config.json
index d2a9d41fc8848b..dc3501070ff50c 100644
--- a/testing/variations/fieldtrial_testing_config.json
+++ b/testing/variations/fieldtrial_testing_config.json
@@ -4873,6 +4873,26 @@
             ]
         }
     ],
+    "SqlSkipPreload": [
+        {
+            "platforms": [
+                "android",
+                "chromeos",
+                "linux",
+                "mac",
+                "ios",
+                "windows"
+            ],
+            "experiments": [
+                {
+                    "name": "Enabled",
+                    "enable_features": [
+                        "SqlSkipPreload"
+                    ]
+                }
+            ]
+        }
+    ],
     "StabilityDebugging": [
         {
             "platforms": [