[tracing] Simplify and improve thread-hopping logic of memory-infra

base/trace_event/memory_dump_manager.h

Index: base/trace_event/memory_dump_manager.h
diff --git a/base/trace_event/memory_dump_manager.h b/base/trace_event/memory_dump_manager.h
index 0718d32e9525ad3527a4e914f2033b2f9d8ec623..e4de04e3257b445dd004c14e99a65f7b36b17921 100644
--- a/base/trace_event/memory_dump_manager.h
+++ b/base/trace_event/memory_dump_manager.h
@@ -5,7 +5,7 @@
 #ifndef BASE_TRACE_EVENT_MEMORY_DUMP_MANAGER_H_
 #define BASE_TRACE_EVENT_MEMORY_DUMP_MANAGER_H_
 
-#include <vector>
+#include <set>
 
 #include "base/atomicops.h"
 #include "base/containers/hash_tables.h"
@@ -14,6 +14,7 @@
 #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"
 
 namespace base {
@@ -22,13 +23,8 @@ class SingleThreadTaskRunner;
 
 namespace trace_event {
 
-namespace {
-class ProcessMemoryDumpHolder;
-}
-
 class MemoryDumpManagerDelegate;
 class MemoryDumpProvider;
-class ProcessMemoryDump;
 class MemoryDumpSessionState;
 
 // This is the interface exposed to the rest of the codebase to deal with
@@ -99,32 +95,80 @@ class BASE_EXPORT MemoryDumpManager : public TraceLog::EnabledStateObserver {
   uint64 tracing_process_id() const { return tracing_process_id_; }
 
  private:
+  friend struct DefaultDeleter<MemoryDumpManager>;  // For the testing instance.
+  friend struct DefaultSingletonTraits<MemoryDumpManager>;
+  friend class MemoryDumpManagerDelegate;
+  friend class MemoryDumpManagerTest;
+  FRIEND_TEST_ALL_PREFIXES(MemoryDumpManagerTest, DisableFailingDumpers);
+
   // Descriptor struct used to hold information about registered MDPs. It is
-  // deliberately copyable, in order to allow to be used as hash_map value.
+  // deliberately copyable, in order to allow to be used as std::set value.

[petrcermak, 2015/07/03 11:04:16]

nit: I think it should be either "to allow IT to be used", or "to BE allowED to
be used"

[Primiano Tucci (use gerrit), 2015/07/03 15:05:45]

Done.

   struct MemoryDumpProviderInfo {
     MemoryDumpProviderInfo(
+        MemoryDumpProvider* dump_provider,
         const scoped_refptr<SingleThreadTaskRunner>& task_runner);
     ~MemoryDumpProviderInfo();
 
+    // Define a total order based on the affinity thread (i.e. |task_runner|),

[petrcermak, 2015/07/03 11:04:16]

Don't you mean "thread affinity" (swap word order)? In that case, there should
be no preceding "the" and the "(i.e. |task_runner|)" should probably be in the
middle of the statement "thread (i.e. |task_runner|) affinity", which is quite
unreadable, or most probably "thread affinity (i.e. |task_runner| affinity)".

[Primiano Tucci (use gerrit), 2015/07/03 15:05:45]

^__^. Sold for 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;
     scoped_refptr<SingleThreadTaskRunner> task_runner;  // Optional.
-    int consecutive_failures;  // Number of times the provider failed (to
-                               // disable the MDPs).
-    bool disabled;  // For fail-safe logic (auto-disable failing MDPs).
+
+    // 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;
   };
 
-  friend struct DefaultDeleter<MemoryDumpManager>;  // For the testing instance.
-  friend struct DefaultSingletonTraits<MemoryDumpManager>;
-  friend class MemoryDumpManagerDelegate;
-  friend class MemoryDumpManagerTest;
-  FRIEND_TEST_ALL_PREFIXES(MemoryDumpManagerTest, DisableFailingDumpers);
+  using MemoryDumpProviderInfoSet = std::set<MemoryDumpProviderInfo>;
 
-  static const int kMaxConsecutiveFailuresCount;
+  // Holds the state of a process memory dump that needs to be carry over across

[petrcermak, 2015/07/03 11:04:17]

s/carry/carried/

[Primiano Tucci (use gerrit), 2015/07/03 15:05:45]

Done.

+  // threads in order to fullfill an asynchronous CreateProcessDump() request.

[petrcermak, 2015/07/03 11:04:17]

s/fullfill/fulfil/ (I find the correct spelling strange to be honest but it is
what it is).

[Primiano Tucci (use gerrit), 2015/07/03 15:05:45]

Done.

+  // At any time, exactly one thread owns a ProcessMemoryDumpAsyncState.
+  struct ProcessMemoryDumpAsyncState {
+    ProcessMemoryDumpAsyncState(
+        MemoryDumpRequestArgs req_args,
+        MemoryDumpProviderInfoSet::iterator next_dump_provider,
+        const scoped_refptr<MemoryDumpSessionState>& session_state,
+        MemoryDumpCallback callback);
+    ~ProcessMemoryDumpAsyncState();
 
-  static void SetInstanceForTesting(MemoryDumpManager* instance);
+    // The ProcessMemoryDump container, where each dump provider will dump its
+    // own MemoryAllocatorDump(s) upon the OnMemoryDump() call.
+    ProcessMemoryDump process_memory_dump;
+
+    // The arguments passed to the initial CreateProcessDump() request.
+    const MemoryDumpRequestArgs req_args;
+
+    // An iterator to the next dump provider that should be invoked.

[petrcermak, 2015/07/03 11:04:16]

nit: Most of the fields are described as "The ...", so I'd suggest doing the
same here and for callback.

[Primiano Tucci (use gerrit), 2015/07/03 15:05:45]

Done.

+    MemoryDumpProviderInfoSet::iterator next_dump_provider;
+
+    // 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.
+    const scoped_refptr<SingleThreadTaskRunner> task_runner;
+
+   private:
+    DISALLOW_COPY_AND_ASSIGN(ProcessMemoryDumpAsyncState);
+  };
+
+  static const int kMaxConsecutiveFailuresCount;
 
   MemoryDumpManager();
   virtual ~MemoryDumpManager();
 
+  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 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
@@ -132,11 +176,10 @@ class BASE_EXPORT MemoryDumpManager : public TraceLog::EnabledStateObserver {
   void CreateProcessDump(const MemoryDumpRequestArgs& args,
                          const MemoryDumpCallback& callback);
 
-  bool InvokeDumpProviderLocked(MemoryDumpProvider* mdp,
-                                ProcessMemoryDump* pmd);
+  // Continues the ProcessMemoryDump started by CreateProcessDump(), hopping
+  // across threads as needed as specified by MDPs in RegisterDumpProvider().
   void ContinueAsyncProcessDump(
-      MemoryDumpProvider* mdp,
-      scoped_refptr<ProcessMemoryDumpHolder> pmd_holder);
+      scoped_ptr<ProcessMemoryDumpAsyncState> pmd_async_state);
 
   // Pass kInvalidTracingProcessId to invalidate the id.
   void set_tracing_process_id(uint64 id) {
@@ -145,7 +188,13 @@ class BASE_EXPORT MemoryDumpManager : public TraceLog::EnabledStateObserver {
     tracing_process_id_ = id;
   }
 
-  hash_map<MemoryDumpProvider*, MemoryDumpProviderInfo> dump_providers_;
+  // An ordererd set of registered MemoryDumpProviderInfo(s), sorted by thread
+  // affinity (MDPs belonging to the same thread are adjacent).
+  MemoryDumpProviderInfoSet dump_providers_;
+
+  // Flag used to signal that some provider was removed from |dump_providers_|
+  // and therefore the current memory dump (if any) should be aborted.
+  bool did_unregister_dump_provider_;
 
   // Shared among all the PMDs to keep state scoped to the tracing session.
   scoped_refptr<MemoryDumpSessionState> session_state_;