[tracing] Simplify logic of MemoryDumpManager

base/trace_event/memory_dump_manager.h

 // 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 BASE_TRACE_EVENT_MEMORY_DUMP_MANAGER_H_
 #define BASE_TRACE_EVENT_MEMORY_DUMP_MANAGER_H_
 
 #include <map>
 #include <memory>
 #include <set>
+#include <vector>
 
 #include "base/atomicops.h"
 #include "base/containers/hash_tables.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/singleton.h"
 #include "base/synchronization/lock.h"
 #include "base/timer/timer.h"
 #include "base/trace_event/memory_dump_request_args.h"
 #include "base/trace_event/process_memory_dump.h"
 #include "base/trace_event/trace_event.h"
@@ skipping 102 matching lines @@
   void set_dumper_registrations_ignored_for_testing(bool ignored) {
     dumper_registrations_ignored_for_testing_ = ignored;
   }
 
  private:
   friend std::default_delete<MemoryDumpManager>;  // For the testing instance.
   friend struct DefaultSingletonTraits<MemoryDumpManager>;
   friend class MemoryDumpManagerDelegate;
   friend class MemoryDumpManagerTest;
 
-  // Descriptor struct used to hold information about registered MDPs. It is
-  // deliberately copyable, in order to allow it to be used as std::set value.
-  struct MemoryDumpProviderInfo {
+  // Descriptor used to hold information about registered MDPs.
+  // Some important considerations about lifetime of this object:
+  // - In nominal conditions, all the MemoryDumpProviderInfo instances live in
+  //   the |dump_providers_| collection (% unregistration while dumping).
+  // - Upon each dump they (actually their scoped_refptr-s) are copied into
+  //   the ProcessMemoryDumpAsyncState. This is to allow removal (see below).
+  // - When the MDP.OnMemoryDump() is invoked, the corresponding MDPInfo copy
+  //   inside ProcessMemoryDumpAsyncState is removed.
+  // - In nominal conditions, the MDPInfo is destroyed in the
+  //   UnregisterDumpProvider() call.
+  // - If UnregisterDumpProvider() is called while a dump is in progress, the
+  //   MDPInfo is destroyed in the epilogue of ContinueAsyncProcessDump(), when
+  //   the copy inside ProcessMemoryDumpAsyncState is erase()-d.

[ssid, 2015/12/18 15:18:46]

Maybe also get back the comment that says, the non-const elements can only be
accessed in the task_runner thread if it is not null, unless the thread does not
exist. Not really sure if needed.

[Primiano Tucci (use gerrit), 2015/12/18 16:54:46]

Done.

+  struct MemoryDumpProviderInfo
+      : public RefCountedThreadSafe<MemoryDumpProviderInfo> {
+    // Define a total order based on the thread (i.e. |task_runner|) affinity,
+    // so that all MDP belonging to the same thread are adjacent in the set.
+    struct Comparator {
+      bool operator()(const scoped_refptr<MemoryDumpProviderInfo>& a,
+                      const scoped_refptr<MemoryDumpProviderInfo>& b) const;
+    };
+    using OrderedSet =
+        std::set<scoped_refptr<MemoryDumpProviderInfo>, Comparator>;
+
     MemoryDumpProviderInfo(
         MemoryDumpProvider* dump_provider,
         const char* name,
         const scoped_refptr<SingleThreadTaskRunner>& task_runner,
         const MemoryDumpProvider::Options& options);
-    ~MemoryDumpProviderInfo();
-
-    // Define a total order based on the thread (i.e. |task_runner|) affinity,
-    // so that all MDP belonging to the same thread are adjacent in the set.
-    bool operator<(const MemoryDumpProviderInfo& other) const;
 
     MemoryDumpProvider* const dump_provider;
+
+    // Human readable name, for debugging and testing. Not necessarily unique.
     const char* const name;
 
     // The task_runner affinity. Can be nullptr, in which case the dump provider
     // will be invoked on |dump_thread_|.
-    scoped_refptr<SingleThreadTaskRunner> task_runner;
+    const scoped_refptr<SingleThreadTaskRunner> task_runner;
 
     // The |options| arg passed to RegisterDumpProvider().
     const MemoryDumpProvider::Options options;
 
-    // For fail-safe logic (auto-disable failing MDPs). These fields are mutable
-    // as can be safely changed without impacting the order within the set.
-    mutable int consecutive_failures;
-    mutable bool disabled;
+    // For fail-safe logic (auto-disable failing MDPs).
+    int consecutive_failures;
 
-    // When a dump provider unregisters, it is flagged as |unregistered| and it
-    // is removed only upon the next memory dump. This is to avoid altering the
-    // |dump_providers_| collection while a dump is in progress.
-    mutable bool unregistered;
+    // Flagged either by the auto-disable logic or during unregistration.
+    bool disabled;
+
+   private:
+    friend class base::RefCountedThreadSafe<MemoryDumpProviderInfo>;
+    ~MemoryDumpProviderInfo();
+
+    DISALLOW_COPY_AND_ASSIGN(MemoryDumpProviderInfo);
   };
 
-  using MemoryDumpProviderInfoSet = std::set<MemoryDumpProviderInfo>;
-
   // Holds the state of a process memory dump that needs to be carried over
   // across threads in order to fulfil an asynchronous CreateProcessDump()
   // request. At any time exactly one thread owns a ProcessMemoryDumpAsyncState.
   struct ProcessMemoryDumpAsyncState {
     ProcessMemoryDumpAsyncState(
         MemoryDumpRequestArgs req_args,
-        MemoryDumpProviderInfoSet::iterator next_dump_provider,
+        const MemoryDumpProviderInfo::OrderedSet& dump_providers,
         const scoped_refptr<MemoryDumpSessionState>& session_state,
         MemoryDumpCallback callback,
         const scoped_refptr<SingleThreadTaskRunner>& dump_thread_task_runner);
     ~ProcessMemoryDumpAsyncState();
 
     // Gets or creates the memory dump container for the given target process.
     ProcessMemoryDump* GetOrCreateMemoryDumpContainerForProcess(ProcessId pid);
 
     // A map of ProcessId -> ProcessMemoryDump, one for each target process
     // being dumped from the current process. Typically each process dumps only
     // for itself, unless dump providers specify a different |target_process| in
     // MemoryDumpProvider::Options.
     std::map<ProcessId, scoped_ptr<ProcessMemoryDump>> process_dumps;
 
     // The arguments passed to the initial CreateProcessDump() request.
     const MemoryDumpRequestArgs req_args;
 
-    // The |dump_providers_| iterator to the next dump provider that should be
-    // invoked (or dump_providers_.end() if at the end of the sequence).
-    MemoryDumpProviderInfoSet::iterator next_dump_provider;
+    // An ordered sequence of dump providers that have to be invoked to complete
+    // the dump. This is a copy of |dump_providers_| at the beginning of a dump
+    // and becomes empty at the end, when all dump providers have been invoked.
+    std::vector<scoped_refptr<MemoryDumpProviderInfo>> pending_dump_providers;
 
     // The trace-global session state.
     scoped_refptr<MemoryDumpSessionState> session_state;
 
     // Callback passed to the initial call to CreateProcessDump().
     MemoryDumpCallback callback;
 
     // The thread on which FinalizeDumpAndAddToTrace() (and hence |callback|)
     // should be invoked. This is the thread on which the initial
     // CreateProcessDump() request was called.
@@ skipping 12 matching lines @@
 
   static const int kMaxConsecutiveFailuresCount;
   static const char* const kSystemAllocatorPoolName;
 
   MemoryDumpManager();
   ~MemoryDumpManager() override;
 
   static void SetInstanceForTesting(MemoryDumpManager* instance);
   static void FinalizeDumpAndAddToTrace(
       scoped_ptr<ProcessMemoryDumpAsyncState> pmd_async_state);
-  static void AbortDumpLocked(MemoryDumpCallback callback,
-                              scoped_refptr<SingleThreadTaskRunner> task_runner,
-                              uint64_t dump_guid);
 
   // Internal, used only by MemoryDumpManagerDelegate.
   // Creates a memory dump for the current process and appends it to the trace.
   // |callback| will be invoked asynchronously upon completion on the same
   // thread on which CreateProcessDump() was called.
   void CreateProcessDump(const MemoryDumpRequestArgs& args,
                          const MemoryDumpCallback& callback);
 
   // Continues the ProcessMemoryDump started by CreateProcessDump(), hopping
   // across threads as needed as specified by MDPs in RegisterDumpProvider().
   void ContinueAsyncProcessDump(
-      scoped_ptr<ProcessMemoryDumpAsyncState> pmd_async_state);
+      ProcessMemoryDumpAsyncState* owned_pmd_async_state);
 
   // An ordererd set of registered MemoryDumpProviderInfo(s), sorted by thread
   // affinity (MDPs belonging to the same thread are adjacent).
-  MemoryDumpProviderInfoSet dump_providers_;
+  MemoryDumpProviderInfo::OrderedSet dump_providers_;
 
   // Shared among all the PMDs to keep state scoped to the tracing session.
   scoped_refptr<MemoryDumpSessionState> session_state_;
 
   MemoryDumpManagerDelegate* delegate_;  // Not owned.
 
   // When true, this instance is in charge of coordinating periodic dumps.
   bool is_coordinator_;
 
   // Protects from concurrent accesses to the |dump_providers_*| and |delegate_|
@@ skipping 44 matching lines @@
   }
 
  private:
   DISALLOW_COPY_AND_ASSIGN(MemoryDumpManagerDelegate);
 };
 
 }  // namespace trace_event
 }  // namespace base
 
 #endif  // BASE_TRACE_EVENT_MEMORY_DUMP_MANAGER_H_