diff options
author | ssid <ssid@chromium.org> | 2016-01-13 06:21:57 -0800 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2016-01-13 14:23:13 +0000 |
commit | 3be5b1ecdf66f6eaaa0ba98f9b36cc2d93bf54d9 (patch) | |
tree | 20326a0c236d72a2e28b007f1782116cf301b07c /sql | |
parent | 3a6f49f649c3c413ca10fc60ec85f39532ac9e45 (diff) | |
download | chromium_src-3be5b1ecdf66f6eaaa0ba98f9b36cc2d93bf54d9.zip chromium_src-3be5b1ecdf66f6eaaa0ba98f9b36cc2d93bf54d9.tar.gz chromium_src-3be5b1ecdf66f6eaaa0ba98f9b36cc2d93bf54d9.tar.bz2 |
[tracing] Add separate dump provider for sql connection
The sql connection memory dump is not thread safe since the connections
can get deleted while a dump is happening. To make this thread safe,
this CL introduces a dump provider class owned by the connection. This
class holds a lock when dumping and deleting the database. Also, to
workaround thread safe dump provider registration, it uses the
UnregisterAndDeleteDumpProviderAsync api added in crrev.com/1430073002.
BUG=466141
Review URL: https://codereview.chromium.org/1434993002
Cr-Commit-Position: refs/heads/master@{#369161}
Diffstat (limited to 'sql')
-rw-r--r-- | sql/BUILD.gn | 2 | ||||
-rw-r--r-- | sql/connection.cc | 61 | ||||
-rw-r--r-- | sql/connection.h | 15 | ||||
-rw-r--r-- | sql/connection_memory_dump_provider.cc | 74 | ||||
-rw-r--r-- | sql/connection_memory_dump_provider.h | 41 | ||||
-rw-r--r-- | sql/connection_unittest.cc | 3 | ||||
-rw-r--r-- | sql/sql.gyp | 2 | ||||
-rw-r--r-- | sql/sql_memory_dump_provider.h | 6 |
8 files changed, 149 insertions, 55 deletions
diff --git a/sql/BUILD.gn b/sql/BUILD.gn index 58097b7..b79eca8 100644 --- a/sql/BUILD.gn +++ b/sql/BUILD.gn @@ -8,6 +8,8 @@ component("sql") { sources = [ "connection.cc", "connection.h", + "connection_memory_dump_provider.cc", + "connection_memory_dump_provider.h", "error_delegate_util.cc", "error_delegate_util.h", "init_status.h", diff --git a/sql/connection.cc b/sql/connection.cc index 9daa7d1..003005a 100644 --- a/sql/connection.cc +++ b/sql/connection.cc @@ -27,7 +27,7 @@ #include "base/strings/utf_string_conversions.h" #include "base/synchronization/lock.h" #include "base/trace_event/memory_dump_manager.h" -#include "base/trace_event/process_memory_dump.h" +#include "sql/connection_memory_dump_provider.h" #include "sql/meta_table.h" #include "sql/statement.h" #include "third_party/sqlite/sqlite3.h" @@ -254,44 +254,6 @@ bool Connection::ShouldIgnoreSqliteCompileError(int error) { basic_error == SQLITE_CORRUPT; } -bool Connection::OnMemoryDump(const base::trace_event::MemoryDumpArgs& args, - base::trace_event::ProcessMemoryDump* pmd) { - if (args.level_of_detail == - base::trace_event::MemoryDumpLevelOfDetail::LIGHT || - !db_) { - return true; - } - - // The high water mark is not tracked for the following usages. - int cache_size, dummy_int; - sqlite3_db_status(db_, SQLITE_DBSTATUS_CACHE_USED, &cache_size, &dummy_int, - 0 /* resetFlag */); - int schema_size; - sqlite3_db_status(db_, SQLITE_DBSTATUS_SCHEMA_USED, &schema_size, &dummy_int, - 0 /* resetFlag */); - int statement_size; - sqlite3_db_status(db_, SQLITE_DBSTATUS_STMT_USED, &statement_size, &dummy_int, - 0 /* resetFlag */); - - std::string name = base::StringPrintf( - "sqlite/%s_connection/%p", - histogram_tag_.empty() ? "Unknown" : histogram_tag_.c_str(), this); - base::trace_event::MemoryAllocatorDump* dump = pmd->CreateAllocatorDump(name); - dump->AddScalar(base::trace_event::MemoryAllocatorDump::kNameSize, - base::trace_event::MemoryAllocatorDump::kUnitsBytes, - cache_size + schema_size + statement_size); - dump->AddScalar("cache_size", - base::trace_event::MemoryAllocatorDump::kUnitsBytes, - cache_size); - dump->AddScalar("schema_size", - base::trace_event::MemoryAllocatorDump::kUnitsBytes, - schema_size); - dump->AddScalar("statement_size", - base::trace_event::MemoryAllocatorDump::kUnitsBytes, - statement_size); - return true; -} - void Connection::ReportDiagnosticInfo(int extended_error, Statement* stmt) { AssertIOAllowed(); @@ -386,13 +348,9 @@ Connection::Connection() update_time_histogram_(NULL), query_time_histogram_(NULL), clock_(new TimeSource()) { - base::trace_event::MemoryDumpManager::GetInstance()->RegisterDumpProvider( - this, "sql::Connection", nullptr); } Connection::~Connection() { - base::trace_event::MemoryDumpManager::GetInstance()->UnregisterDumpProvider( - this); Close(); } @@ -511,6 +469,17 @@ void Connection::CloseInternal(bool forced) { // of the function. http://crbug.com/136655. AssertIOAllowed(); + // Reseting acquires a lock to ensure no dump is happening on the database + // at the same time. Unregister takes ownership of provider and it is safe + // since the db is reset. memory_dump_provider_ could be null if db_ was + // poisoned. + if (memory_dump_provider_) { + memory_dump_provider_->ResetDatabase(); + base::trace_event::MemoryDumpManager::GetInstance() + ->UnregisterAndDeleteDumpProviderSoon( + std::move(memory_dump_provider_)); + } + int rc = sqlite3_close(db_); if (rc != SQLITE_OK) { UMA_HISTOGRAM_SPARSE_SLOWLY("Sqlite.CloseFailure", rc); @@ -1848,6 +1817,12 @@ bool Connection::OpenInternal(const std::string& file_name, mmap_enabled_ = true; } + DCHECK(!memory_dump_provider_); + memory_dump_provider_.reset( + new ConnectionMemoryDumpProvider(db_, histogram_tag_)); + base::trace_event::MemoryDumpManager::GetInstance()->RegisterDumpProvider( + memory_dump_provider_.get(), "sql::Connection", nullptr); + return true; } diff --git a/sql/connection.h b/sql/connection.h index e121b9f..b35e2fa 100644 --- a/sql/connection.h +++ b/sql/connection.h @@ -20,7 +20,6 @@ #include "base/memory/scoped_ptr.h" #include "base/threading/thread_restrictions.h" #include "base/time/time.h" -#include "base/trace_event/memory_dump_provider.h" #include "sql/sql_export.h" struct sqlite3; @@ -33,6 +32,7 @@ class HistogramBase; namespace sql { +class ConnectionMemoryDumpProvider; class Recovery; class Statement; @@ -105,7 +105,7 @@ class SQL_EXPORT TimeSource { DISALLOW_COPY_AND_ASSIGN(TimeSource); }; -class SQL_EXPORT Connection : public base::trace_event::MemoryDumpProvider { +class SQL_EXPORT Connection { private: class StatementRef; // Forward declaration, see real one below. @@ -113,7 +113,7 @@ class SQL_EXPORT Connection : public base::trace_event::MemoryDumpProvider { // The database is opened by calling Open[InMemory](). Any uncommitted // transactions will be rolled back when this object is deleted. Connection(); - ~Connection() override; + ~Connection(); // Pre-init configuration ---------------------------------------------------- @@ -484,11 +484,6 @@ class SQL_EXPORT Connection : public base::trace_event::MemoryDumpProvider { // with the syntax of a SQL statement, or problems with the database schema. static bool ShouldIgnoreSqliteCompileError(int error); - // base::trace_event::MemoryDumpProvider implementation. - bool OnMemoryDump( - const base::trace_event::MemoryDumpArgs& args, - base::trace_event::ProcessMemoryDump* process_memory_dump) override; - // Collect various diagnostic information and post a crash dump to aid // debugging. Dump rate per database is limited to prevent overwhelming the // crash server. @@ -511,6 +506,7 @@ class SQL_EXPORT Connection : public base::trace_event::MemoryDumpProvider { FRIEND_TEST_ALL_PREFIXES(SQLConnectionTest, CollectDiagnosticInfo); FRIEND_TEST_ALL_PREFIXES(SQLConnectionTest, GetAppropriateMmapSize); + FRIEND_TEST_ALL_PREFIXES(SQLConnectionTest, OnMemoryDump); FRIEND_TEST_ALL_PREFIXES(SQLConnectionTest, RegisterIntentToUpload); // Internal initialize function used by both Init and InitInMemory. The file @@ -795,6 +791,9 @@ class SQL_EXPORT Connection : public base::trace_event::MemoryDumpProvider { // changes. scoped_ptr<TimeSource> clock_; + // Stores the dump provider object when db is open. + scoped_ptr<ConnectionMemoryDumpProvider> memory_dump_provider_; + DISALLOW_COPY_AND_ASSIGN(Connection); }; diff --git a/sql/connection_memory_dump_provider.cc b/sql/connection_memory_dump_provider.cc new file mode 100644 index 0000000..0284bd0 --- /dev/null +++ b/sql/connection_memory_dump_provider.cc @@ -0,0 +1,74 @@ +// Copyright 2015 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "sql/connection_memory_dump_provider.h" + +#include "base/strings/stringprintf.h" +#include "base/trace_event/process_memory_dump.h" +#include "third_party/sqlite/sqlite3.h" + +namespace sql { + +ConnectionMemoryDumpProvider::ConnectionMemoryDumpProvider( + sqlite3* db, + const std::string& name) + : db_(db), connection_name_(name) {} + +ConnectionMemoryDumpProvider::~ConnectionMemoryDumpProvider() {} + +void ConnectionMemoryDumpProvider::ResetDatabase() { + base::AutoLock lock(lock_); + db_ = nullptr; +} + +bool ConnectionMemoryDumpProvider::OnMemoryDump( + const base::trace_event::MemoryDumpArgs& args, + base::trace_event::ProcessMemoryDump* pmd) { + if (args.level_of_detail == base::trace_event::MemoryDumpLevelOfDetail::LIGHT) + return true; + + int cache_size = 0; + int schema_size = 0; + int statement_size = 0; + { + // Lock is acquired here so that db_ is not reset in ResetDatabase when + // collecting stats. + base::AutoLock lock(lock_); + if (!db_) { + return false; + } + + // The high water mark is not tracked for the following usages. + int dummy_int; + int status = sqlite3_db_status(db_, SQLITE_DBSTATUS_CACHE_USED, &cache_size, + &dummy_int, 0 /* resetFlag */); + DCHECK_EQ(SQLITE_OK, status); + status = sqlite3_db_status(db_, SQLITE_DBSTATUS_SCHEMA_USED, &schema_size, + &dummy_int, 0 /* resetFlag */); + DCHECK_EQ(SQLITE_OK, status); + status = sqlite3_db_status(db_, SQLITE_DBSTATUS_STMT_USED, &statement_size, + &dummy_int, 0 /* resetFlag */); + DCHECK_EQ(SQLITE_OK, status); + } + + std::string name = base::StringPrintf( + "sqlite/%s_connection/%p", + connection_name_.empty() ? "Unknown" : connection_name_.c_str(), this); + base::trace_event::MemoryAllocatorDump* dump = pmd->CreateAllocatorDump(name); + dump->AddScalar(base::trace_event::MemoryAllocatorDump::kNameSize, + base::trace_event::MemoryAllocatorDump::kUnitsBytes, + cache_size + schema_size + statement_size); + dump->AddScalar("cache_size", + base::trace_event::MemoryAllocatorDump::kUnitsBytes, + cache_size); + dump->AddScalar("schema_size", + base::trace_event::MemoryAllocatorDump::kUnitsBytes, + schema_size); + dump->AddScalar("statement_size", + base::trace_event::MemoryAllocatorDump::kUnitsBytes, + statement_size); + return true; +} + +} // namespace sql diff --git a/sql/connection_memory_dump_provider.h b/sql/connection_memory_dump_provider.h new file mode 100644 index 0000000..bcad0f8 --- /dev/null +++ b/sql/connection_memory_dump_provider.h @@ -0,0 +1,41 @@ +// Copyright 2015 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef SQL_CONNECTION_MEMORY_DUMP_PROVIDER_H +#define SQL_CONNECTION_MEMORY_DUMP_PROVIDER_H + +#include <string> + +#include "base/macros.h" +#include "base/synchronization/lock.h" +#include "base/trace_event/memory_dump_provider.h" + +struct sqlite3; + +namespace sql { + +class ConnectionMemoryDumpProvider + : public base::trace_event::MemoryDumpProvider { + public: + ConnectionMemoryDumpProvider(sqlite3* db, const std::string& name); + ~ConnectionMemoryDumpProvider() override; + + void ResetDatabase(); + + // base::trace_event::MemoryDumpProvider implementation. + bool OnMemoryDump( + const base::trace_event::MemoryDumpArgs& args, + base::trace_event::ProcessMemoryDump* process_memory_dump) override; + + private: + sqlite3* db_; // not owned. + base::Lock lock_; + std::string connection_name_; + + DISALLOW_COPY_AND_ASSIGN(ConnectionMemoryDumpProvider); +}; + +} // namespace sql + +#endif // SQL_CONNECTION_MEMORY_DUMP_PROVIDER_H diff --git a/sql/connection_unittest.cc b/sql/connection_unittest.cc index d25f7c9..c6f80d8 100644 --- a/sql/connection_unittest.cc +++ b/sql/connection_unittest.cc @@ -15,6 +15,7 @@ #include "base/test/histogram_tester.h" #include "base/trace_event/process_memory_dump.h" #include "sql/connection.h" +#include "sql/connection_memory_dump_provider.h" #include "sql/correct_sql_test_base.h" #include "sql/meta_table.h" #include "sql/statement.h" @@ -1308,7 +1309,7 @@ TEST_F(SQLConnectionTest, OnMemoryDump) { base::trace_event::ProcessMemoryDump pmd(nullptr); base::trace_event::MemoryDumpArgs args = { base::trace_event::MemoryDumpLevelOfDetail::DETAILED}; - ASSERT_TRUE(db().OnMemoryDump(args, &pmd)); + ASSERT_TRUE(db().memory_dump_provider_->OnMemoryDump(args, &pmd)); EXPECT_GE(pmd.allocator_dumps().size(), 1u); } diff --git a/sql/sql.gyp b/sql/sql.gyp index 362597e..2e02427 100644 --- a/sql/sql.gyp +++ b/sql/sql.gyp @@ -22,6 +22,8 @@ 'sources': [ 'connection.cc', 'connection.h', + 'connection_memory_dump_provider.cc', + 'connection_memory_dump_provider.h', 'error_delegate_util.cc', 'error_delegate_util.h', 'init_status.h', diff --git a/sql/sql_memory_dump_provider.h b/sql/sql_memory_dump_provider.h index 01d7f04..3d894e2 100644 --- a/sql/sql_memory_dump_provider.h +++ b/sql/sql_memory_dump_provider.h @@ -2,8 +2,8 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#ifndef SQL_PROCESS_MEMORY_DUMP_PROVIDER_H -#define SQL_PROCESS_MEMORY_DUMP_PROVIDER_H +#ifndef SQL_SQL_MEMORY_DUMP_PROVIDER_H +#define SQL_SQL_MEMORY_DUMP_PROVIDER_H #include "base/macros.h" #include "base/memory/singleton.h" @@ -34,4 +34,4 @@ class SQL_EXPORT SqlMemoryDumpProvider } // namespace sql -#endif // SQL_PROCESS_MEMORY_DUMP_PROVIDER_H +#endif // SQL_SQL_MEMORY_DUMP_PROVIDER_H |