diff options
author | zhaoqin <zhaoqin@chromium.org> | 2015-09-30 11:04:29 -0700 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2015-09-30 18:05:20 +0000 |
commit | 745e18c523846de216c2f2253beb4e8047f77e57 (patch) | |
tree | a8c43a082280c59e1a2abe79015d358db1b5f175 /sql | |
parent | 493d992af78670f99743bb5458c8d7b6019dbb72 (diff) | |
download | chromium_src-745e18c523846de216c2f2253beb4e8047f77e57.zip chromium_src-745e18c523846de216c2f2253beb4e8047f77e57.tar.gz chromium_src-745e18c523846de216c2f2253beb4e8047f77e57.tar.bz2 |
Revert of [sql] Use memory-mapped I/O for sql::Connection. (patchset #3 id:40001 of https://codereview.chromium.org/1368533003/ )
Reason for revert:
data race reported by TSan on sql/connection.cc
BUG=537681
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.
>
> [Relands https://crrev.com/9a1948a4d6d4,
> reverted at https://crrev.com/55c3216b15ce ]
>
> BUG=489784, 533682
>
> Committed: https://crrev.com/5c2f4e50d96d8b2ae0ae4a5c7d319e6be3b33bca
> Cr-Commit-Position: refs/heads/master@{#351344}
TBR=rmcilroy@chromium.org,pavely@chromium.org,shess@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=489784, 533682
Review URL: https://codereview.chromium.org/1375583003
Cr-Commit-Position: refs/heads/master@{#351596}
Diffstat (limited to 'sql')
-rw-r--r-- | sql/connection.cc | 94 | ||||
-rw-r--r-- | sql/connection.h | 20 | ||||
-rw-r--r-- | sql/connection_unittest.cc | 80 | ||||
-rw-r--r-- | sql/statement.cc | 5 |
4 files changed, 0 insertions, 199 deletions
diff --git a/sql/connection.cc b/sql/connection.cc index 662127c..300e5e6 100644 --- a/sql/connection.cc +++ b/sql/connection.cc @@ -282,9 +282,6 @@ Connection::Connection() needs_rollback_(false), in_memory_(false), poisoned_(false), - mmap_disabled_(false), - mmap_enabled_(false), - total_changes_at_last_release_(0), stats_histogram_(NULL), commit_time_histogram_(NULL), autocommit_time_histogram_(NULL), @@ -469,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; @@ -846,9 +776,6 @@ bool Connection::CommitTransaction() { RecordCommitTime(delta); RecordOneEvent(EVENT_COMMIT); - // Release dirty cache pages after the transaction closes. - ReleaseCacheMemoryIfNeeded(false); - return ret; } @@ -939,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; } @@ -1309,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); @@ -1364,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; } diff --git a/sql/connection.h b/sql/connection.h index d456b6c..19592d9 100644 --- a/sql/connection.h +++ b/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. diff --git a/sql/connection_unittest.cc b/sql/connection_unittest.cc index 0038a1d..10a15a8 100644 --- a/sql/connection_unittest.cc +++ b/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,82 +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. -#if !defined(MOJO_APPTEST_IMPL) -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]); - } -} -#endif - } // namespace diff --git a/sql/statement.cc b/sql/statement.cc index 2a0faa7..09c95da 100644 --- a/sql/statement.cc +++ b/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; } |