Revert of [sql] Use memory-mapped I/O for sql::Connection. (patchset c…

…hromium#8 id:140001 of https://codereview.chromium.org/1349863003/ )

Reason for revert:
mmap_enabled_ isn't initialized, causing MSAN failures:

https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_MSAN/5882/layout-test-results/virtual/sharedarraybuffer/fast/workers/constructor-proto-crash-log.txt

STDERR: ==3138==WARNING: MemorySanitizer: use-of-uninitialized-value
STDERR:     #0 0x7fc8068d3a65 in ReleaseCacheMemoryIfNeeded sql/connection.cc:513:7
STDERR:     chromium#1 0x7fc8068d3a65 in sql::Connection::ExecuteAndReturnErrorCode(char const*) sql/connection.cc:943:0
STDERR:     chromium#2 0x7fc8068ca454 in sql::Connection::OpenInternal(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, sql::Connection::Retry) sql/connection.cc:1275:9
STDERR:     chromium#3 0x7fc8068c845f in sql::Connection::Open(base::FilePath const&) sql/connection.cc:367:10
STDERR:     chromium#4 0x7fc806b1b868 in storage::QuotaDatabase::LazyOpen(bool) storage/browser/quota/quota_database.cc:488:14

Original issue's description:
> [sql] Use memory-mapped I/O for sql::Connection.
>
> sql::Connection::Open*() uses PRAGMA mmap_size to enable SQLite's
> memory-mapped I/O.  Additionally instrument to flush dirty pages from
> the page cache after writes.
>
> BUG=489784,533682
>
> Committed: https://crrev.com/9a1948a4d6d445d5c8e209bdcd1cd050af72060b
> Cr-Commit-Position: refs/heads/master@{#350362}

R=shess@chromium.org
TBR=pavely@chromium.org, pvalenzuela@chromium.org, rmcilroy@chromium.org, shess@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=489784,533682

Review URL: https://codereview.chromium.org/1365783002 .

Cr-Commit-Position: refs/heads/master@{#350386}

sql/connection.cc

@@ -466,73 +466,6 @@ void Connection::Preload() {
   }
 }
 
-// SQLite keeps unused pages associated with a connection in a cache.  It asks
-// the cache for pages by an id, and if the page is present and the database is
-// unchanged, it considers the content of the page valid and doesn't read it
-// from disk.  When memory-mapped I/O is enabled, on read SQLite uses page
-// structures created from the memory map data before consulting the cache.  On
-// write SQLite creates a new in-memory page structure, copies the data from the
-// memory map, and later writes it, releasing the updated page back to the
-// cache.
-//
-// This means that in memory-mapped mode, the contents of the cached pages are
-// not re-used for reads, but they are re-used for writes if the re-written page
-// is still in the cache. The implementation of sqlite3_db_release_memory() as
-// of SQLite 3.8.7.4 frees all pages from pcaches associated with the
-// connection, so it should free these pages.
-//
-// Unfortunately, the zero page is also freed.  That page is never accessed
-// using memory-mapped I/O, and the cached copy can be re-used after verifying
-// the file change counter on disk.  Also, fresh pages from cache receive some
-// pager-level initialization before they can be used.  Since the information
-// involved will immediately be accessed in various ways, it is unclear if the
-// additional overhead is material, or just moving processor cache effects
-// around.
-//
-// TODO(shess): It would be better to release the pages immediately when they
-// are no longer needed.  This would basically happen after SQLite commits a
-// transaction.  I had implemented a pcache wrapper to do this, but it involved
-// layering violations, and it had to be setup before any other sqlite call,
-// which was brittle.  Also, for large files it would actually make sense to
-// maintain the existing pcache behavior for blocks past the memory-mapped
-// segment.  I think drh would accept a reasonable implementation of the overall
-// concept for upstreaming to SQLite core.
-//
-// TODO(shess): Another possibility would be to set the cache size small, which
-// would keep the zero page around, plus some pre-initialized pages, and SQLite
-// can manage things.  The downside is that updates larger than the cache would
-// spill to the journal.  That could be compensated by setting cache_spill to
-// false.  The downside then is that it allows open-ended use of memory for
-// large transactions.
-//
-// TODO(shess): The TrimMemory() trick of bouncing the cache size would also
-// work.  There could be two prepared statements, one for cache_size=1 one for
-// cache_size=goal.
-void Connection::ReleaseCacheMemoryIfNeeded(bool implicit_change_performed) {
-  // If memory-mapping is not enabled, the page cache helps performance.
-  if (!mmap_enabled_)
-    return;
-
-  // On caller request, force the change comparison to fail.  Done before the
-  // transaction-nesting test so that the signal can carry to transaction
-  // commit.
-  if (implicit_change_performed)
-    --total_changes_at_last_release_;
-
-  // Cached pages may be re-used within the same transaction.
-  if (transaction_nesting())
-    return;
-
-  // If no changes have been made, skip flushing.  This allows the first page of
-  // the database to remain in cache across multiple reads.
-  const int total_changes = sqlite3_total_changes(db_);
-  if (total_changes == total_changes_at_last_release_)
-    return;
-
-  total_changes_at_last_release_ = total_changes;
-  sqlite3_db_release_memory(db_);
-}
-
 void Connection::TrimMemory(bool aggressively) {
   if (!db_)
     return;
@@ -843,9 +776,6 @@ bool Connection::CommitTransaction() {
   RecordCommitTime(delta);
   RecordOneEvent(EVENT_COMMIT);
 
-  // Release dirty cache pages after the transaction closes.
-  ReleaseCacheMemoryIfNeeded(false);
-
   return ret;
 }
 
@@ -936,12 +866,6 @@ int Connection::ExecuteAndReturnErrorCode(const char* sql) {
     const base::TimeDelta delta = Now() - before;
     RecordTimeAndChanges(delta, read_only);
   }
-
-  // Most calls to Execute() modify the database.  The main exceptions would be
-  // calls such as CREATE TABLE IF NOT EXISTS which could modify the database
-  // but sometimes don't.
-  ReleaseCacheMemoryIfNeeded(true);
-
   return rc;
 }
 
@@ -1306,18 +1230,6 @@ bool Connection::OpenInternal(const std::string& file_name,
   // secure_delete.
   ignore_result(Execute("PRAGMA journal_mode = TRUNCATE"));
 
-  // Enable memory-mapped access.  This value will be capped by
-  // SQLITE_MAX_MMAP_SIZE, which could be different between 32-bit and 64-bit
-  // platforms.
-  mmap_enabled_ = false;
-  if (!mmap_disabled_)
-    ignore_result(Execute("PRAGMA mmap_size = 268435456"));  // 256MB.
-  {
-    Statement s(GetUniqueStatement("PRAGMA mmap_size"));
-    if (s.Step() && s.ColumnInt64(0) > 0)
-      mmap_enabled_ = true;
-  }
-
   const base::TimeDelta kBusyTimeout =
     base::TimeDelta::FromSeconds(kBusyTimeoutSeconds);
 
@@ -1361,9 +1273,6 @@ void Connection::DoRollback() {
   RecordUpdateTime(delta);
   RecordOneEvent(EVENT_ROLLBACK);
 
-  // The cache may have been accumulating dirty pages for commit.
-  ReleaseCacheMemoryIfNeeded(false);
-
   needs_rollback_ = false;
 }
 

sql/connection.h

@@ -146,9 +146,6 @@ class SQL_EXPORT Connection {
   // other platforms.
   void set_restrict_to_user() { restrict_to_user_ = true; }
 
-  // Call to opt out of memory-mapped file I/O.
-  void set_mmap_disabled() { mmap_disabled_ = true; }
-
   // Set an error-handling callback.  On errors, the error number (and
   // statement, if available) will be passed to the callback.
   //
@@ -641,12 +638,6 @@ class SQL_EXPORT Connection {
     return clock_->Now();
   }
 
-  // Release page-cache memory if memory-mapped I/O is enabled and the database
-  // was changed.  Passing true for |implicit_change_performed| allows
-  // overriding the change detection for cases like DDL (CREATE, DROP, etc),
-  // which do not participate in the total-rows-changed tracking.
-  void ReleaseCacheMemoryIfNeeded(bool implicit_change_performed);
-
   // The actual sqlite database. Will be NULL before Init has been called or if
   // Init resulted in an error.
   sqlite3* db_;
@@ -688,17 +679,6 @@ class SQL_EXPORT Connection {
   // databases.
   bool poisoned_;
 
-  // |true| if SQLite memory-mapped I/O is not desired for this connection.
-  bool mmap_disabled_;
-
-  // |true| if SQLite memory-mapped I/O was enabled for this connection.
-  // Used by ReleaseCacheMemoryIfNeeded().
-  bool mmap_enabled_;
-
-  // Used by ReleaseCacheMemoryIfNeeded() to track if new changes have happened
-  // since memory was last released.
-  int total_changes_at_last_release_;
-
   ErrorCallback error_callback_;
 
   // Tag for auxiliary histograms.

sql/connection_unittest.cc

@@ -4,12 +4,10 @@
 
 #include "base/bind.h"
 #include "base/files/file_util.h"
-#include "base/files/memory_mapped_file.h"
 #include "base/files/scoped_file.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/logging.h"
 #include "base/metrics/statistics_recorder.h"
-#include "base/strings/stringprintf.h"
 #include "base/test/histogram_tester.h"
 #include "sql/connection.h"
 #include "sql/correct_sql_test_base.h"
@@ -1300,80 +1298,4 @@ TEST_F(SQLConnectionTest, TimeUpdateTransaction) {
   EXPECT_EQ(0, samples->sum());
 }
 
-// Make sure that OS file writes to a mmap'ed file are reflected in the memory
-// mapping of a memory-mapped file.  Normally SQLite writes to memory-mapped
-// files using memcpy(), which should stay consistent.  Our SQLite is slightly
-// patched to mmap read only, then write using OS file writes.  If the
-// memory-mapped version doesn't reflect the OS file writes, SQLite's
-// memory-mapped I/O should be disabled on this platform.
-TEST_F(SQLConnectionTest, MmapTest) {
-  // Skip the test for platforms which don't enable memory-mapped I/O in SQLite,
-  // or which don't even support the pragma.  The former seems to apply to iOS,
-  // the latter to older iOS.
-  // TODO(shess): Disable test on iOS?  Disable on USE_SYSTEM_SQLITE?
-  {
-    sql::Statement s(db().GetUniqueStatement("PRAGMA mmap_size"));
-    if (!s.Step() || !s.ColumnInt64(0))
-      return;
-  }
-
-  // The test re-uses the database file to make sure it's representative of a
-  // SQLite file, but will be storing incompatible data.
-  db().Close();
-
-  const uint32 kFlags =
-      base::File::FLAG_OPEN|base::File::FLAG_READ|base::File::FLAG_WRITE;
-  char buf[4096];
-
-  // Create a file with a block of '0', a block of '1', and a block of '2'.
-  {
-    base::File f(db_path(), kFlags);
-    ASSERT_TRUE(f.IsValid());
-    memset(buf, '0', sizeof(buf));
-    ASSERT_EQ(f.Write(0*sizeof(buf), buf, sizeof(buf)), (int)sizeof(buf));
-
-    memset(buf, '1', sizeof(buf));
-    ASSERT_EQ(f.Write(1*sizeof(buf), buf, sizeof(buf)), (int)sizeof(buf));
-
-    memset(buf, '2', sizeof(buf));
-    ASSERT_EQ(f.Write(2*sizeof(buf), buf, sizeof(buf)), (int)sizeof(buf));
-  }
-
-  // mmap the file and verify that everything looks right.
-  {
-    base::MemoryMappedFile m;
-    ASSERT_TRUE(m.Initialize(db_path()));
-
-    memset(buf, '0', sizeof(buf));
-    ASSERT_EQ(0, memcmp(buf, m.data() + 0*sizeof(buf), sizeof(buf)));
-
-    memset(buf, '1', sizeof(buf));
-    ASSERT_EQ(0, memcmp(buf, m.data() + 1*sizeof(buf), sizeof(buf)));
-
-    memset(buf, '2', sizeof(buf));
-    ASSERT_EQ(0, memcmp(buf, m.data() + 2*sizeof(buf), sizeof(buf)));
-
-    // Scribble some '3' into the first page of the file, and verify that it
-    // looks the same in the memory mapping.
-    {
-      base::File f(db_path(), kFlags);
-      ASSERT_TRUE(f.IsValid());
-      memset(buf, '3', sizeof(buf));
-      ASSERT_EQ(f.Write(0*sizeof(buf), buf, sizeof(buf)), (int)sizeof(buf));
-    }
-    ASSERT_EQ(0, memcmp(buf, m.data() + 0*sizeof(buf), sizeof(buf)));
-
-    // Repeat with a single '4' in case page-sized blocks are different.
-    const size_t kOffset = 1*sizeof(buf) + 123;
-    ASSERT_NE('4', m.data()[kOffset]);
-    {
-      base::File f(db_path(), kFlags);
-      ASSERT_TRUE(f.IsValid());
-      buf[0] = '4';
-      ASSERT_EQ(f.Write(kOffset, buf, 1), 1);
-    }
-    ASSERT_EQ('4', m.data()[kOffset]);
-  }
-}
-
 }  // namespace

sql/statement.cc

@@ -110,11 +110,6 @@ void Statement::Reset(bool clear_bound_vars) {
       ref_->connection()->RecordOneEvent(Connection::EVENT_STATEMENT_SUCCESS);
   }
 
-  // Potentially release dirty cache pages if an autocommit statement made
-  // changes.
-  if (ref_->connection())
-    ref_->connection()->ReleaseCacheMemoryIfNeeded(false);
-
   succeeded_ = false;
   stepped_ = false;
 }

sync/syncable/directory_backing_store.cc

@@ -1690,10 +1690,6 @@ void DirectoryBackingStore::ResetAndCreateConnection() {
   db_->set_exclusive_locking();
   db_->set_cache_size(32);
   db_->set_page_size(database_page_size_);
-
-  // TODO(shess): Sync corruption tests interact poorly with mmap, disable for
-  // now.  http://crbug.com/533682
-  db_->set_mmap_disabled();
   if (!catastrophic_error_handler_.is_null())
     SetCatastrophicErrorHandler(catastrophic_error_handler_);
 }

third_party/sqlite/BUILD.gn

@@ -67,13 +67,6 @@ if (!use_system_sqlite) {
       # prefer to control distribution to worker threads.
       "SQLITE_MAX_WORKER_THREADS=0",
 
-      # Allow 256MB mmap footprint per connection.  Should not be too open-ended
-      # as that could cause memory fragmentation.  50MB encompasses the 99th
-      # percentile of Chrome databases in the wild.
-      # TODO(shess): A 64-bit-specific value could be 1G or more.
-      # TODO(shess): Figure out if exceeding this is costly.
-      "SQLITE_MAX_MMAP_SIZE=268435456",
-
       # Use a read-only memory map when mmap'ed I/O is enabled to prevent memory
       # stompers from directly corrupting the database.
       # TODO(shess): Upstream the ability to use this define.

third_party/sqlite/sqlite.gyp

@@ -28,12 +28,6 @@
       # appropriately.  Chromium doesn't configure SQLite for that, and would
       # prefer to control distribution to worker threads.
       'SQLITE_MAX_WORKER_THREADS=0',
-      # Allow 256MB mmap footprint per connection.  Should not be too open-ended
-      # as that could cause memory fragmentation.  50MB encompasses the 99th
-      # percentile of Chrome databases in the wild.
-      # TODO(shess): A 64-bit-specific value could be 1G or more.
-      # TODO(shess): Figure out if exceeding this is costly.
-      'SQLITE_MAX_MMAP_SIZE=268435456',
       # Use a read-only memory map when mmap'ed I/O is enabled to prevent memory
       # stompers from directly corrupting the database.
       # TODO(shess): Upstream the ability to use this define.