Skip to content

Commit

Permalink
sql: Add Finch trial for eliminating sql::Database::Preload().
Browse files Browse the repository at this point in the history
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}
  • Loading branch information
pwnall authored and Commit Bot committed May 22, 2019
1 parent 8abfe92 commit 3ffd9a3
Show file tree
Hide file tree
Showing 4 changed files with 36 additions and 2 deletions.
4 changes: 4 additions & 0 deletions sql/database.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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;
Expand Down
12 changes: 11 additions & 1 deletion sql/sql_features.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
2 changes: 1 addition & 1 deletion sql/sql_features.h
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
20 changes: 20 additions & 0 deletions testing/variations/fieldtrial_testing_config.json
Original file line number Diff line number Diff line change
Expand Up @@ -4873,6 +4873,26 @@
]
}
],
"SqlSkipPreload": [
{
"platforms": [
"android",
"chromeos",
"linux",
"mac",
"ios",
"windows"
],
"experiments": [
{
"name": "Enabled",
"enable_features": [
"SqlSkipPreload"
]
}
]
}
],
"StabilityDebugging": [
{
"platforms": [
Expand Down

0 comments on commit 3ffd9a3

Please sign in to comment.