Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1314)

Unified Diff: base/trace_event/memory_dump_manager.cc

Issue 2836933002: memory-infra: Never kill memory-infra background thread (Closed)
Patch Set: Created 3 years, 8 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: base/trace_event/memory_dump_manager.cc
diff --git a/base/trace_event/memory_dump_manager.cc b/base/trace_event/memory_dump_manager.cc
index 10e5d1b13f6c6e22756b73e5e4da0c73dd1533a8..d449d78cd5c8c132666c82b4f02eb5a7b12a6228 100644
--- a/base/trace_event/memory_dump_manager.cc
+++ b/base/trace_event/memory_dump_manager.cc
@@ -489,6 +489,24 @@ bool MemoryDumpManager::IsDumpProviderRegisteredForTesting(
return false;
}
+scoped_refptr<base::SingleThreadTaskRunner> MemoryDumpManager::GetTaskRunner() {
+ lock_.AssertAcquired();
+
+ if (dump_thread_)
+ return dump_thread_->task_runner();
+
+ // Spin-up the thread used to invoke unbound dump providers.
+ std::unique_ptr<Thread> dump_thread(new Thread("MemoryInfra"));
+ if (!dump_thread->Start()) {
Primiano Tucci (use gerrit) 2017/04/24 15:30:58 I'd just violently CHECK(started) here. Trying to
hjd 2017/04/24 16:27:59 Done.
+ LOG(ERROR) << "Failed to start the memory-infra thread for tracing";
+ NOTREACHED();
+ }
+
+ dump_thread_ = std::move(dump_thread);
+
+ return dump_thread_->task_runner();
+}
+
void MemoryDumpManager::CreateProcessDump(
const MemoryDumpRequestArgs& args,
const ProcessMemoryDumpCallback& callback) {
@@ -512,12 +530,8 @@ void MemoryDumpManager::CreateProcessDump(
{
AutoLock lock(lock_);
- // |dump_thread_| can be nullptr is tracing was disabled before reaching
- // here. SetupNextMemoryDump() is robust enough to tolerate it and will
- // NACK the dump.
pmd_async_state.reset(new ProcessMemoryDumpAsyncState(
- args, dump_providers_, session_state_, callback,
- dump_thread_ ? dump_thread_->task_runner() : nullptr));
+ args, dump_providers_, session_state_, callback, GetTaskRunner()));
// Safety check to prevent reaching here without calling RequestGlobalDump,
// with disallowed modes. If |session_state_| is null then tracing is
@@ -551,22 +565,22 @@ void MemoryDumpManager::SetupNextMemoryDump(
// (for discounting trace memory overhead) while holding the |lock_|.
TraceLog::GetInstance()->InitializeThreadLocalEventBufferIfSupported();
- // |dump_thread_| might be destroyed before getting this point.
- // It means that tracing was disabled right before starting this dump.
- // Anyway either tracing is stopped or this was the last hop, create a trace
+ // MDM might have been disabled before getting to this point.
+ // Anyway either MDM is disabled or this was the last hop, create a trace
// event, add it to the trace and finalize process dump invoking the callback.
- if (!pmd_async_state->dump_thread_task_runner.get()) {
+ if (!subtle::NoBarrier_Load(&memory_tracing_enabled_)) {
Primiano Tucci (use gerrit) 2017/04/24 15:30:58 I think this extra check is superfluous. The callb
hjd 2017/04/24 16:27:59 Removing this causes a test (MemoryDumpManagerTest
ssid 2017/04/25 03:21:47 I agree with primiano. We could get to a state whe
if (pmd_async_state->pending_dump_providers.empty()) {
- VLOG(1) << kLogPrefix << " failed because dump thread was destroyed"
+ VLOG(1) << kLogPrefix << " failed because MemoryDumpManager was disabled"
<< " before finalizing the dump";
} else {
- VLOG(1) << kLogPrefix << " failed because dump thread was destroyed"
+ VLOG(1) << kLogPrefix << " failed because MemoryDumpManager was disabled"
<< " before dumping "
<< pmd_async_state->pending_dump_providers.back().get()->name;
}
pmd_async_state->dump_successful = false;
pmd_async_state->pending_dump_providers.clear();
}
+
if (pmd_async_state->pending_dump_providers.empty())
return FinalizeDumpAndAddToTrace(std::move(pmd_async_state));
@@ -585,7 +599,7 @@ void MemoryDumpManager::SetupNextMemoryDump(
}
// If the dump provider did not specify a task runner affinity, dump on
- // |dump_thread_| which is already checked above for presence.
+ // |dump_thread_|.
SequencedTaskRunner* task_runner = mdpinfo->task_runner.get();
if (!task_runner) {
DCHECK(mdpinfo->options.dumps_on_single_thread_task_runner);
@@ -786,12 +800,6 @@ void MemoryDumpManager::FinalizeDumpAndAddToTrace(
void MemoryDumpManager::Enable(
const TraceConfig::MemoryDumpConfig& memory_dump_config) {
- // Spin-up the thread used to invoke unbound dump providers.
- std::unique_ptr<Thread> dump_thread(new Thread("MemoryInfra"));
- if (!dump_thread->Start()) {
- LOG(ERROR) << "Failed to start the memory-infra thread for tracing";
- return;
- }
scoped_refptr<MemoryDumpSessionState> session_state =
new MemoryDumpSessionState;
@@ -826,9 +834,6 @@ void MemoryDumpManager::Enable(
DCHECK(delegate_); // At this point we must have a delegate.
session_state_ = session_state;
- DCHECK(!dump_thread_);
- dump_thread_ = std::move(dump_thread);
-
subtle::NoBarrier_Store(&memory_tracing_enabled_, 1);
MemoryDumpScheduler::Config periodic_config;
@@ -851,7 +856,7 @@ void MemoryDumpManager::Enable(
MemoryPeakDetector::GetInstance()->Setup(
BindRepeating(&MemoryDumpManager::GetDumpProvidersForPolling,
Unretained(this)),
- dump_thread_->task_runner(),
+ GetTaskRunner(),
BindRepeating(&OnPeakDetected, trigger.level_of_detail));
MemoryPeakDetector::Config peak_config;
@@ -864,7 +869,7 @@ void MemoryDumpManager::Enable(
// When peak detection is enabled, trigger a dump straight away as it
// gives a good reference point for analyzing the trace.
if (delegate_->IsCoordinator()) {
- dump_thread_->task_runner()->PostTask(
+ GetTaskRunner()->PostTask(
FROM_HERE, BindRepeating(&OnPeakDetected, trigger.level_of_detail));
}
}
@@ -872,8 +877,7 @@ void MemoryDumpManager::Enable(
// Only coordinator process triggers periodic global memory dumps.
if (delegate_->IsCoordinator() && !periodic_config.triggers.empty()) {
- MemoryDumpScheduler::GetInstance()->Start(periodic_config,
- dump_thread_->task_runner());
+ MemoryDumpScheduler::GetInstance()->Start(periodic_config, GetTaskRunner());
}
}
@@ -884,19 +888,12 @@ void MemoryDumpManager::Disable() {
if (!subtle::NoBarrier_Load(&memory_tracing_enabled_))
return;
subtle::NoBarrier_Store(&memory_tracing_enabled_, 0);
- std::unique_ptr<Thread> dump_thread;
{
AutoLock lock(lock_);
MemoryDumpScheduler::GetInstance()->Stop();
MemoryPeakDetector::GetInstance()->TearDown();
- dump_thread = std::move(dump_thread_);
session_state_ = nullptr;
}
-
- // Thread stops are blocking and must be performed outside of the |lock_|
- // or will deadlock (e.g., if SetupNextMemoryDump() tries to acquire it).
- if (dump_thread)
- dump_thread->Stop();
Primiano Tucci (use gerrit) 2017/04/24 15:30:58 \o/ This, as a side effect, should fix crbug.com/7
hjd 2017/04/24 16:27:59 Done.
ssid 2017/04/25 03:21:47 Yay
}
bool MemoryDumpManager::IsDumpModeAllowed(MemoryDumpLevelOfDetail dump_mode) {
« base/trace_event/memory_dump_manager.h ('K') | « base/trace_event/memory_dump_manager.h ('k') | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698