memory-infra: Remove is_enabled_ from MDM

base/trace_event/memory_dump_manager.cc

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 387212c85fa98ee616908ae7d5515acdd2b83335..39cca628194518c9e16cfd8ff15dcfa81e4a4f7c 100644
--- a/base/trace_event/memory_dump_manager.cc
+++ b/base/trace_event/memory_dump_manager.cc
@@ -182,7 +182,6 @@ void MemoryDumpManager::SetInstanceForTesting(MemoryDumpManager* instance) {
 
 MemoryDumpManager::MemoryDumpManager()
     : is_coordinator_(false),
-      is_enabled_(0),
       tracing_process_id_(kInvalidTracingProcessId),
       dumper_registrations_ignored_for_testing_(false),
       heap_profiling_enabled_(false) {
@@ -394,7 +393,7 @@ void MemoryDumpManager::UnregisterDumpProviderInternal(
     DCHECK(!(*mdp_iter)->owned_dump_provider);
     (*mdp_iter)->owned_dump_provider = std::move(owned_mdp);
   } else if (strict_thread_check_blacklist_.count((*mdp_iter)->name) == 0 ||
-             subtle::NoBarrier_Load(&is_enabled_)) {
+             tracing_observer_->IsMemoryInfraTracingEnabled()) {

[Primiano Tucci (use gerrit), 2017/04/28 15:58:17]

ssid is ripping out this code for you in
https://codereview.chromium.org/2844373002/ which I just LGTM'd
Which I guess is what caused the most awkwardness

[ssid, 2017/04/28 18:35:04]

Sorry i did not review this cl because of the mentioned cl. Please rebase.

     // If dump provider's name is on |strict_thread_check_blacklist_|, then the
     // DCHECK is fired only when tracing is enabled. Otherwise the DCHECK is
     // fired even when tracing is not enabled (stricter).
@@ -435,12 +434,9 @@ void MemoryDumpManager::RequestGlobalDump(
     MemoryDumpType dump_type,
     MemoryDumpLevelOfDetail level_of_detail,
     const GlobalMemoryDumpCallback& callback) {
-  // Bail out immediately if tracing is not enabled at all or if the dump mode
-  // is not allowed.
-  if (!UNLIKELY(subtle::NoBarrier_Load(&is_enabled_)) ||
-      !IsDumpModeAllowed(level_of_detail)) {
-    VLOG(1) << kLogPrefix << " failed because " << kTraceCategory
-            << " tracing category is not enabled or the requested dump mode is "
+  if (!IsDumpModeAllowed(level_of_detail)) {

[Primiano Tucci (use gerrit), 2017/04/28 15:58:17]

I think we should have this check only in the ::Finalize method, before calling
the TracingObserver->MaybeAddToTrace()

which kinda feels like a dejavu because I though we did that already? (or maybe
I start confusing CL I reviewed vs CLs that exist only in my mind)

[ssid, 2017/04/28 18:35:04]

No this used to exist here and not even trigger wrong dumps. Makes perfect sense
to just move this to just before MaybeAddToTrace call.
Also we need a failure condition there to run the callback if dump mode is not
allowed? I think the case in Finalize() should be:

If (!IsDumpModeAllowed() && dump_type != SUMMARY_ONLY) {
  callback.Run(false /* failure */);
} else {
  MaybeAddToTrace();
}


Do we still need to optimize the case where some component r4equests a detailed
dump when only background dumps are allowed in the tracing session, then we just
don't trigger the dumps instead early out with failure? I think it'd be a good
idea to have a check here too.

So, with that logic the option would be:

in RequestGlobalDump():
If (!IsDumpModeAllowed() && dump_type != SUMMARY_ONLY) {
 callback.Run(false)
 return;
}

in Finalize():
CHECK(IsDumpModeAllowed() || dump_type == SUMMARY_ONLY);


I am fine with either case. I think having the optimization is better.

[hjd, 2017/05/04 11:17:18]

Done.

+    VLOG(1) << kLogPrefix
+            << " failed because  the requested dump mode is "
                "not allowed by trace config.";
     if (!callback.is_null())
       callback.Run(0u /* guid */, false /* success */);
@@ -565,22 +561,6 @@ void MemoryDumpManager::SetupNextMemoryDump(
   // (for discounting trace memory overhead) while holding the |lock_|.
   TraceLog::GetInstance()->InitializeThreadLocalEventBufferIfSupported();
 
-  // 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 (!subtle::NoBarrier_Load(&is_enabled_)) {
-    if (pmd_async_state->pending_dump_providers.empty()) {
-      VLOG(1) << kLogPrefix << " failed because MemoryDumpManager was disabled"
-              << " before finalizing the dump";
-    } else {
-      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));
 
@@ -835,8 +815,6 @@ void MemoryDumpManager::Enable(
   DCHECK(!request_dump_function_.is_null());
   session_state_ = session_state;
 
-  subtle::NoBarrier_Store(&is_enabled_, 1);
-
   MemoryDumpScheduler::Config periodic_config;
   bool peak_detector_configured = false;
   for (const auto& trigger : memory_dump_config.triggers) {
@@ -887,15 +865,11 @@ void MemoryDumpManager::Disable() {
   // There might be a memory dump in progress while this happens. Therefore,
   // ensure that the MDM state which depends on the tracing enabled / disabled
   // state is always accessed by the dumping methods holding the |lock_|.
-  if (!subtle::NoBarrier_Load(&is_enabled_))
-    return;
-  subtle::NoBarrier_Store(&is_enabled_, 0);

[ssid, 2017/04/28 18:35:04]

Yay we don't need this anymore :D

[hjd, 2017/05/04 11:17:18]

:D

-  {
-    AutoLock lock(lock_);
-    MemoryDumpScheduler::GetInstance()->Stop();
-    MemoryPeakDetector::GetInstance()->TearDown();
-    session_state_ = nullptr;
-  }
+  AutoLock lock(lock_);
+
+  MemoryDumpScheduler::GetInstance()->Stop();
+  MemoryPeakDetector::GetInstance()->TearDown();
+  session_state_ = nullptr;
 }
 
 bool MemoryDumpManager::IsDumpModeAllowed(MemoryDumpLevelOfDetail dump_mode) {

[ssid, 2017/04/28 18:35:04]

Maybe rename to DoesSessionAllowDumpMode?
Since without having a session any dump mode is allowed.

[hjd, 2017/05/04 11:17:18]

Done.