memory-infra: Remove is_enabled_ from MDM

base/trace_event/memory_dump_manager.cc

 // 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 "base/trace_event/memory_dump_manager.h"
 
 #include <inttypes.h>
 #include <stdio.h>
 
 #include <algorithm>
@@ skipping 164 matching lines @@
                    LeakySingletonTraits<MemoryDumpManager>>::get();
 }
 
 // static
 void MemoryDumpManager::SetInstanceForTesting(MemoryDumpManager* instance) {
   g_instance_for_testing = instance;
 }
 
 MemoryDumpManager::MemoryDumpManager()
     : is_coordinator_(false),
-      is_enabled_(0),
       tracing_process_id_(kInvalidTracingProcessId),
       dumper_registrations_ignored_for_testing_(false),
       heap_profiling_enabled_(false) {
   g_next_guid.GetNext();  // Make sure that first guid is not zero.
 
   // At this point the command line may not be initialized but we try to
   // enable the heap profiler to capture allocations as soon as possible.
   EnableHeapProfilingIfNeeded();
 
   strict_thread_check_blacklist_.insert(std::begin(kStrictThreadCheckBlacklist),
@@ skipping 191 matching lines @@
 
   if (take_mdp_ownership_and_delete_async) {
     // The MDP will be deleted whenever the MDPInfo struct will, that is either:
     // - At the end of this function, if no dump is in progress.
     // - Either in SetupNextMemoryDump() or InvokeOnMemoryDump() when MDPInfo is
     //   removed from |pending_dump_providers|.
     // - When the provider is removed from other clients (MemoryPeakDetector).
     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).
     // TODO(ssid): Remove this condition after removing all the dump providers
     // in the blacklist and the buildbots are no longer flakily hitting the
     // DCHECK, crbug.com/643438.
 
     // If you hit this DCHECK, your dump provider has a bug.
     // Unregistration of a MemoryDumpProvider is safe only if:
     // - The MDP has specified a sequenced task runner affinity AND the
@@ skipping 20 matching lines @@
   // to just skip it, without actually invoking the |mdp|, which might be
   // destroyed by the caller soon after this method returns.
   (*mdp_iter)->disabled = true;
   dump_providers_.erase(mdp_iter);
 }
 
 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 */);
     return;
   }
 
   const uint64_t guid =
       TraceLog::GetInstance()->MangleEventId(g_next_guid.GetNext());
 
   // Creates an async event to keep track of the global dump evolution.
@@ skipping 67 matching lines @@
   // unexpected dumps.
   if (TraceLog::GetInstance()
           ->GetCurrentTraceConfig()
           .IsArgumentFilterEnabled()) {
     CHECK_EQ(MemoryDumpLevelOfDetail::BACKGROUND, args.level_of_detail);
   }
 
   std::unique_ptr<ProcessMemoryDumpAsyncState> pmd_async_state;
   {
     AutoLock lock(lock_);
 

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

We should have a condition here that says:

if (level_of_detail === DETAILED && heap_profiling_enabled_ && !session_state_)
{
  callback.Run(false);
  return;
}

If we have heap profiling enabled and no session then PMD will later expect a
session state while creating heap dump.
So before creating PMDAS we need to check if session state is null.

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

Done.

     pmd_async_state.reset(new ProcessMemoryDumpAsyncState(
         args, dump_providers_, session_state_, callback,
         GetOrCreateBgTaskRunnerLocked()));
 
     // Safety check to prevent reaching here without calling RequestGlobalDump,
     // with disallowed modes. If |session_state_| is null then tracing is
     // disabled.
     CHECK(!session_state_ ||
           session_state_->IsDumpModeAllowed(args.level_of_detail));

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

If we are going with the first option of first creating dump and then deciding
about adding to trace this check should be removed since it will crash if
someone requests a detailed dump.

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

Acknowledged.

 
     // If enabled, holds back the peak detector resetting its estimation window.
     MemoryPeakDetector::GetInstance()->Throttle();
   }
 
   // Start the process dump. This involves task runner hops as specified by the
   // MemoryDumpProvider(s) in RegisterDumpProvider()).
   SetupNextMemoryDump(std::move(pmd_async_state));
 }
 
 // PostTask InvokeOnMemoryDump() to the dump provider's sequenced task runner. A
 // PostTask is always required for a generic SequencedTaskRunner to ensure that
 // no other task is running on it concurrently. SetupNextMemoryDump() and
 // InvokeOnMemoryDump() are called alternatively which linearizes the dump
 // provider's OnMemoryDump invocations.
 // At most one of either SetupNextMemoryDump() or InvokeOnMemoryDump() can be
 // active at any time for a given PMD, regardless of status of the |lock_|.
 // |lock_| is used in these functions purely to ensure consistency w.r.t.
 // (un)registrations of |dump_providers_|.
 void MemoryDumpManager::SetupNextMemoryDump(
     std::unique_ptr<ProcessMemoryDumpAsyncState> pmd_async_state) {
   HEAP_PROFILER_SCOPED_IGNORE;
   // Initalizes the ThreadLocalEventBuffer to guarantee that the TRACE_EVENTs
   // in the PostTask below don't end up registering their own dump providers
   // (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));
 
   // Read MemoryDumpProviderInfo thread safety considerations in
   // memory_dump_manager.h when accessing |mdpinfo| fields.
   MemoryDumpProviderInfo* mdpinfo =
       pmd_async_state->pending_dump_providers.back().get();
 
   // If we are in background tracing, we should invoke only the whitelisted
   // providers. Ignore other providers and continue.
@@ skipping 234 matching lines @@
         MakeUnique<SessionStateConvertableProxy<TypeNameDeduplicator>>(
             session_state, &MemoryDumpSessionState::type_name_deduplicator));
   }
 
   AutoLock lock(lock_);
 
   // At this point we must have the ability to request global dumps.
   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) {
     if (!session_state_->IsDumpModeAllowed(trigger.level_of_detail)) {
       NOTREACHED();
       continue;
     }
     if (trigger.trigger_type == MemoryDumpType::PERIODIC_INTERVAL) {
       if (periodic_config.triggers.empty()) {
         periodic_config.callback = BindRepeating(&OnPeriodicSchedulerTick);
@@ skipping 30 matching lines @@
   if (is_coordinator_ && !periodic_config.triggers.empty()) {
     MemoryDumpScheduler::GetInstance()->Start(periodic_config,
                                               GetOrCreateBgTaskRunnerLocked());
   }
 }
 
 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.

   AutoLock lock(lock_);
   if (!session_state_)
     return false;
   return session_state_->IsDumpModeAllowed(dump_mode);
 }
 
 MemoryDumpManager::ProcessMemoryDumpAsyncState::ProcessMemoryDumpAsyncState(
     MemoryDumpRequestArgs req_args,
     const MemoryDumpProviderInfo::OrderedSet& dump_providers,
     scoped_refptr<MemoryDumpSessionState> session_state,
@@ skipping 19 matching lines @@
   if (iter == process_dumps.end()) {
     std::unique_ptr<ProcessMemoryDump> new_pmd(
         new ProcessMemoryDump(session_state, dump_args));
     iter = process_dumps.insert(std::make_pair(pid, std::move(new_pmd))).first;
   }
   return iter->second.get();
 }
 
 }  // namespace trace_event
 }  // namespace base