Move Database::Preload(...) to Database::Options

This CL is adding a field to Database options to control whether
or not the database should be preload.

The preload must happen before the db.Open(...) to ensure that
it's not conflicting with file exclusive mode.

Bug: 40904059
Change-Id: I3e2b2390127f6d240c1327f18c10e290aaab9ae8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6235625
Commit-Queue: Etienne Bergeron <etienneb@chromium.org>
Reviewed-by: Anthony Vallée-Dubois <anthonyvd@chromium.org>
Reviewed-by: Dylan Cutler <dylancutler@google.com>
Cr-Commit-Position: refs/heads/main@{#1418731}

net/extras/sqlite/sqlite_persistent_store_backend_base.cc

@@ -96,7 +96,8 @@ bool SQLitePersistentStoreBackendBase::InitializeDatabase() {
   db_ = std::make_unique<sql::Database>(
       sql::DatabaseOptions()
           .set_exclusive_locking(false)
-          .set_exclusive_database_file_lock(enable_exclusive_access_),
+          .set_exclusive_database_file_lock(enable_exclusive_access_)
+          .set_preload(true),
       histogram_tag_);
 
   // base::Unretained is safe because |this| owns (and therefore outlives) the
@@ -105,24 +106,6 @@ bool SQLitePersistentStoreBackendBase::InitializeDatabase() {
       &SQLitePersistentStoreBackendBase::DatabaseErrorCallback,
       base::Unretained(this)));
 
-  bool has_been_preloaded = false;
-  // It is not possible to preload a database opened with exclusive access,
-  // because the file cannot be opened again to preload it. In this case,
-  // preload before opening the database.
-  if (enable_exclusive_access_) {
-    has_been_preloaded = true;
-
-    // Can only attempt to preload before Open if the file exists.
-    if (base::PathExists(path_)) {
-      // See comments in Database::Preload for explanation of these values.
-      constexpr int kPreReadSize = 128 * 1024 * 1024;  // 128 MB
-      // TODO(crbug.com/40904059): Consider moving preload behind a database
-      // option.
-      base::PreReadFile(path_, /*is_executable=*/false, /*sequential=*/false,
-                        kPreReadSize);
-    }
-  }
-
   if (!db_->Open(path_)) {
     DLOG(ERROR) << "Unable to open " << histogram_tag_.value << " DB.";
     RecordOpenDBProblem();
@@ -130,11 +113,6 @@ bool SQLitePersistentStoreBackendBase::InitializeDatabase() {
     return false;
   }
 
-  // Only attempt a preload if the database hasn't already been preloaded above.
-  if (!has_been_preloaded) {
-    db_->Preload();
-  }
-
   if (!MigrateDatabaseSchema() || !CreateDatabaseSchema()) {
     DLOG(ERROR) << "Unable to update or initialize " << histogram_tag_.value
                 << " DB tables.";

sql/database.cc

@@ -219,6 +219,10 @@ void RecordOpenDatabaseFailureReason(const std::string& histogram_tag,
 }  // namespace
 
 DatabaseOptions::DatabaseOptions() = default;
+DatabaseOptions::DatabaseOptions(const DatabaseOptions&) = default;
+DatabaseOptions::DatabaseOptions(DatabaseOptions&&) = default;
+DatabaseOptions& DatabaseOptions::operator=(const DatabaseOptions&) = default;
+DatabaseOptions& DatabaseOptions::operator=(DatabaseOptions&&) = default;
 DatabaseOptions::~DatabaseOptions() = default;
 
 // static
@@ -385,6 +389,12 @@ bool Database::Open(const base::FilePath& path) {
   DCHECK_NE(path_string, kSqliteOpenInMemoryPath)
       << "Path conflicts with SQLite magic identifier";
 
+  // Preload the database before opening it to ensure it's working with the
+  // exclusive mode.
+  if (options_.preload_) {
+    PreloadInternal(path);
+  }
+
   {
     ScopedOpenErrorReporter reporter(this,
                                      "Sql.Database.Open.FirstAttempt.Error");
@@ -519,18 +529,7 @@ void Database::Preload() {
   CHECK(!options_.exclusive_database_file_lock_)
       << "Cannot preload an exclusively locked database.";
 
-  std::optional<base::ScopedBlockingCall> scoped_blocking_call;
-  InitScopedBlockingCall(FROM_HERE, &scoped_blocking_call);
-
-  // Maximum number of bytes that will be prefetched from the database.
-  //
-  // This limit is very aggressive. The main trade-off involved is that having
-  // SQLite block on reading from disk has a high impact on Chrome startup cost
-  // for the databases that are on the critical path to startup. So, the limit
-  // must exceed the expected sizes of databases on the critical path.
-  constexpr int kPreReadSize = 128 * 1024 * 1024;  // 128 MB
-  base::PreReadFile(DbPath(), /*is_executable=*/false, /*sequential=*/false,
-                    kPreReadSize);
+  PreloadInternal(DbPath());
 }
 
 // SQLite keeps unused pages associated with a database in a cache.  It asks
@@ -2274,6 +2273,29 @@ bool Database::OpenInternal(const std::string& db_file_path) {
   return true;
 }
 
+void Database::PreloadInternal(const base::FilePath& path) {
+  TRACE_EVENT0("sql", "Database::PreloadInternal");
+
+  // TODO(crbug.com/40904059): Consider moving this to a DCHECK after fixing
+  // or migrating callsites that call Preload(...) on in-memory databases.
+  if (!in_memory_) {
+    return;
+  }
+
+  base::ScopedBlockingCall scoped_blocking_call(FROM_HERE,
+                                                base::BlockingType::MAY_BLOCK);
+
+  // Maximum number of bytes that will be prefetched from the database.
+  //
+  // This limit is very aggressive. The main trade-off involved is that having
+  // SQLite block on reading from disk has a high impact on Chrome startup cost
+  // for the databases that are on the critical path to startup. So, the limit
+  // must exceed the expected sizes of databases on the critical path.
+  static constexpr int kPreReadSize = 128 * 1024 * 1024;  // 128 MB
+  base::PreReadFile(path, /*is_executable=*/false, /*sequential=*/false,
+                    kPreReadSize);
+}
+
 void Database::ConfigureSqliteDatabaseObject() {
   // The use of SQLite's non-standard string quoting is not allowed in Chrome.
   //

sql/database.h

@@ -74,6 +74,10 @@ struct COMPONENT_EXPORT(SQL) DatabaseOptions {
   static constexpr int kDefaultPageSize = 4096;
 
   DatabaseOptions();
+  DatabaseOptions(const DatabaseOptions&);
+  DatabaseOptions(DatabaseOptions&&);
+  DatabaseOptions& operator=(const DatabaseOptions&);
+  DatabaseOptions& operator=(DatabaseOptions&&);
   ~DatabaseOptions();
 
   // If true, the database can only be opened by one process at a time.
@@ -142,6 +146,19 @@ struct COMPONENT_EXPORT(SQL) DatabaseOptions {
     return *this;
   }
 
+  // If true, enables preloading the database before opening it.
+  //
+  // Hints the file system that the database will be accessed soon.
+  //
+  // This method should be called on databases that are on the critical path to
+  // Chrome startup. Informing the filesystem about our expected access pattern
+  // early on reduces the likelihood that we'll be blocked on disk I/O. This has
+  // a high impact on startup time.
+  DatabaseOptions& set_preload(bool preload) {
+    preload_ = preload;
+    return *this;
+  }
+
   // If true, transaction commit waits for data to reach persistent media.
   //
   // This is currently only meaningful on macOS. All other operating systems
@@ -245,6 +262,7 @@ struct COMPONENT_EXPORT(SQL) DatabaseOptions {
   bool flush_to_media_ = false;
   int page_size_ = kDefaultPageSize;
   int cache_size_ = 0;
+  bool preload_ = false;
   bool mmap_alt_status_discouraged_ = false;
   bool enable_views_discouraged_ = false;
   const char* vfs_name_discouraged_ = nullptr;
@@ -829,6 +847,9 @@ class COMPONENT_EXPORT(SQL) Database {
   // opened in-memory.
   bool OpenInternal(const std::string& file_name);
 
+  // Requests the operating system to preload the pages on disk into memory.
+  void PreloadInternal(const base::FilePath& path);
+
   // Configures the underlying sqlite3* object via sqlite3_db_config().
   //
   // To minimize the number of possible SQLite code paths executed in Chrome,

sql/database_unittest.cc

@@ -2532,6 +2532,14 @@ TEST_P(SQLDatabaseTest, OpenWithRecoveryHandlesCorruption) {
   }
 }
 
+TEST_P(SQLDatabaseTest, OpenWithPreload) {
+  db_->Close();
+
+  DatabaseOptions options = GetDBOptions().set_preload(true);
+  db_ = std::make_unique<Database>(options, test::kTestTag);
+  ASSERT_TRUE(db_->Open(db_path_));
+}
+
 TEST_P(SQLDatabaseTest, ExecuteFailsAfterCorruptSizeInHeader) {
   ASSERT_TRUE(
       db_->Execute("CREATE TABLE rows(i INTEGER PRIMARY KEY NOT NULL)"));