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

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 <vector>
+#include <set>
 
 #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"
 
 namespace base {
 
 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
 // memory tracing. The main entry point for clients is represented by
 // RequestDumpPoint(). The extension by Un(RegisterDumpProvider).
 class BASE_EXPORT MemoryDumpManager : public TraceLog::EnabledStateObserver {
  public:
   static const uint64 kInvalidTracingProcessId;
   static const char* const kTraceCategoryForTesting;
 
@@ skipping 50 matching lines @@
   // This will never return kInvalidTracingProcessId.
   static uint64 ChildProcessIdToTracingProcessId(int child_id);
 
   // Returns a unique id for the current process. The id can be retrieved only
   // by child processes and only when tracing is enabled. This is intended to
   // express cross-process sharing of memory dumps on the child-process side,
   // without having to know its own child process id.
   uint64 tracing_process_id() const { return tracing_process_id_; }
 
  private:
-  // Descriptor struct used to hold information about registered MDPs. It is
-  // deliberately copyable, in order to allow to be used as hash_map value.
-  struct MemoryDumpProviderInfo {
-    MemoryDumpProviderInfo(
-        const scoped_refptr<SingleThreadTaskRunner>& task_runner);
-    ~MemoryDumpProviderInfo();
-
-    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).
-  };
-
   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 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.
+
+    // 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;
+  };
+
+  using MemoryDumpProviderInfoSet = std::set<MemoryDumpProviderInfo>;
+
+  // 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();
+
+    // 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;
 
-  static void SetInstanceForTesting(MemoryDumpManager* instance);
-
   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
   // thread on which CreateProcessDump() was called.
   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) {
     DCHECK(tracing_process_id_ == kInvalidTracingProcessId ||
            id == kInvalidTracingProcessId || tracing_process_id_ == id);
     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_;
 
   MemoryDumpManagerDelegate* delegate_;  // Not owned.
 
   // Protects from concurrent accesses to the |dump_providers_*| and |delegate_|
   // to guard against disabling logging while dumping on another thread.
   Lock lock_;
 
@@ skipping 39 matching lines @@
   }
 
  private:
   DISALLOW_COPY_AND_ASSIGN(MemoryDumpManagerDelegate);
 };
 
 }  // namespace trace_event
 }  // namespace base
 
 #endif  // BASE_TRACE_EVENT_MEMORY_DUMP_MANAGER_H_