[memory-infra] Remove MemoryDumpManagerDelegate

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 <stdint.h>
 
 #include <map>
 #include <memory>
 #include <unordered_set>
 #include <vector>
 
 #include "base/atomicops.h"
 #include "base/containers/hash_tables.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/singleton.h"
 #include "base/synchronization/lock.h"
 #include "base/trace_event/memory_allocator_dump.h"
 #include "base/trace_event/memory_dump_provider_info.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"
 
-// Forward declare |MemoryDumpManagerDelegateImplTest| so that we can make it a
+// Forward declare |ProcessLocalDumpManagerImplTest| so that we can make it a
 // friend of |MemoryDumpManager| and give it access to |SetInstanceForTesting|.
 namespace memory_instrumentation {
 
-class MemoryDumpManagerDelegateImplTest;
+class ProcessLocalDumpManagerImplTest;
 
 }  // namespace memory_instrumentation
 
 namespace base {
 
 class SingleThreadTaskRunner;
 class Thread;
 
 namespace trace_event {
 
-class MemoryDumpManagerDelegate;
 class MemoryDumpProvider;
 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 char* const kTraceCategory;
   static const char* const kLogPrefix;
 
   // This value is returned as the tracing id of the child processes by
   // GetTracingProcessId() when tracing is not enabled.
   static const uint64_t kInvalidTracingProcessId;
 
   static MemoryDumpManager* GetInstance();
 
   // Invoked once per process to listen to trace begin / end events.
   // Initialization can happen after (Un)RegisterMemoryDumpProvider() calls
   // and the MemoryDumpManager guarantees to support this.
   // On the other side, the MemoryDumpManager will not be fully operational
   // (i.e. will NACK any RequestGlobalMemoryDump()) until initialized.
   // Arguments:
-  //  delegate: inversion-of-control interface for embedder-specific behaviors
-  //      (multiprocess handshaking). See the lifetime and thread-safety
-  //      requirements in the |MemoryDumpManagerDelegate| docstring.
-  void Initialize(std::unique_ptr<MemoryDumpManagerDelegate> delegate);
+  //  request_dump_callback: Callback to invoke a global dump. Global dump
+  //      involves embedder-specific behaviors like multiprocess handshaking.
+  //  is_coordinator: True when current process coodinates the periodic dump
+  //      triggering.
+  using RequestGlobalDumpCallback =

[Primiano Tucci (use gerrit), 2017/04/19 15:44:34]

small nit: type declarations should go up, after public:
see https://google.github.io/styleguide/cppguide.html#Declaration_Order

we have a bunch of things called "callback" around these days. Can we pick a
more specific name for this?
I'd probably not call it "callback" at all (there is precedent for this *) as
this is semantically not quite a callback but more an inverted function call.
probably I'd call it RequestGlobalDumpFunction.

Also can you add a "TODO(primiano): in the long term clients should just reach
into the memory service. For the moment however that requires extra dependencies
and we use this as a way to reach the service from this base class."

*
https://cs.chromium.org/search/?q=using%5C+%5Cw%2Bfunction%5C+%3D%5C+.*Callba...

[ssid, 2017/04/20 01:18:21]

I don't think this should be moved. Let's say in future we have a new allocator
in base that does a GC and we want to request global dumps when GC is done. This
would become impossible because it can't reach the service. Similarly lot of
areas in codebase cannot depend on service, like sql/ and gin/. It will be
better if we keep this api in base.
Yeah I was expecting this comment. Just felt function was not used in chromium.
Fixed.

[Primiano Tucci (use gerrit), 2017/04/20 20:30:05]

Hmm good point. ACK

+      RepeatingCallback<void(const MemoryDumpRequestArgs& args,
+                             const GlobalMemoryDumpCallback& callback)>;
+  void Initialize(RequestGlobalDumpCallback request_dump_callback,
+                  bool is_coordinator);
 
   // (Un)Registers a MemoryDumpProvider instance.
   // Args:
   //  - mdp: the MemoryDumpProvider instance to be registered. MemoryDumpManager
   //      does NOT take memory ownership of |mdp|, which is expected to either
   //      be a singleton or unregister itself.
   //  - name: a friendly name (duplicates allowed). Used for debugging and
   //      run-time profiling of memory-infra internals. Must be a long-lived
   //      C string.
   //  - task_runner: either a SingleThreadTaskRunner or SequencedTaskRunner. All
@@ skipping 32 matching lines @@
   // processes have dumped) and its success (true iff all the dumps were
   // successful).
   void RequestGlobalDump(MemoryDumpType dump_type,
                          MemoryDumpLevelOfDetail level_of_detail,
                          const GlobalMemoryDumpCallback& callback);
 
   // Same as above (still asynchronous), but without callback.
   void RequestGlobalDump(MemoryDumpType dump_type,
                          MemoryDumpLevelOfDetail level_of_detail);
 
+  // NOTE: Use RequestGlobalDump() to create memory dumps. Creates a memory dump

[Primiano Tucci (use gerrit), 2017/04/19 15:44:34]

Since you have the "This method should only be used..." I'd remote the NOTE
here.

[ssid, 2017/04/20 01:18:21]

I'd like to keep this note because it is really easy for someone to call this
function by mistake since it's named CreateProcessDump()

[Primiano Tucci (use gerrit), 2017/04/20 20:30:05]

Acknowledged.

+  // 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. This method should only be used by the
+  // embedder while creating a global memory dump.
+  void CreateProcessDump(const MemoryDumpRequestArgs& args,
+                         const ProcessMemoryDumpCallback& callback);
+
   // TraceLog::EnabledStateObserver implementation.
   void OnTraceLogEnabled() override;
   void OnTraceLogDisabled() override;
 
   // Enable heap profiling if kEnableHeapProfiling is specified.
   void EnableHeapProfilingIfNeeded();
 
   // Returns true if the dump mode is allowed for current tracing session.
   bool IsDumpModeAllowed(MemoryDumpLevelOfDetail dump_mode);
 
@@ skipping 26 matching lines @@
   };
 
   // When set to true, calling |RegisterMemoryDumpProvider| is a no-op.
   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>;
+  // todo fix

[fmeawad, 2017/04/18 14:48:11]

Don't forget this TODO

[ssid, 2017/04/20 01:18:21]

Done.

   friend class MemoryDumpManagerDelegate;
   friend class MemoryDumpManagerTest;
-  friend class memory_instrumentation::MemoryDumpManagerDelegateImplTest;
+  friend class memory_instrumentation::ProcessLocalDumpManagerImplTest;
 
   // Holds the state of a process memory dump that needs to be carried over
   // across task runners in order to fulfil an asynchronous CreateProcessDump()
   // request. At any time exactly one task runner owns a
   // ProcessMemoryDumpAsyncState.
   struct ProcessMemoryDumpAsyncState {
     ProcessMemoryDumpAsyncState(
         MemoryDumpRequestArgs req_args,
         const MemoryDumpProviderInfo::OrderedSet& dump_providers,
         scoped_refptr<MemoryDumpSessionState> session_state,
@@ skipping 49 matching lines @@
   static const char* const kSystemAllocatorPoolName;
 
   MemoryDumpManager();
   ~MemoryDumpManager() override;
 
   static void SetInstanceForTesting(MemoryDumpManager* instance);
   static uint32_t GetDumpsSumKb(const std::string&, const ProcessMemoryDump*);
   static void FinalizeDumpAndAddToTrace(
       std::unique_ptr<ProcessMemoryDumpAsyncState> pmd_async_state);
 
-  // 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 ProcessMemoryDumpCallback& callback);
-
   // Calls InvokeOnMemoryDump() for the next MDP on the task runner specified by
   // the MDP while registration. On failure to do so, skips and continues to
   // next MDP.
   void SetupNextMemoryDump(
       std::unique_ptr<ProcessMemoryDumpAsyncState> pmd_async_state);
 
   // Invokes OnMemoryDump() of the next MDP and calls SetupNextMemoryDump() at
   // the end to continue the ProcessMemoryDump. Should be called on the MDP task
   // runner.
   void InvokeOnMemoryDump(ProcessMemoryDumpAsyncState* owned_pmd_async_state);
@@ skipping 19 matching lines @@
   MemoryDumpProviderInfo::OrderedSet dump_providers_;
 
   // Shared among all the PMDs to keep state scoped to the tracing session.
   scoped_refptr<MemoryDumpSessionState> session_state_;
 
   // The list of names of dump providers that are blacklisted from strict thread
   // affinity check on unregistration.
   std::unordered_set<StringPiece, StringPieceHash>
       strict_thread_check_blacklist_;
 
-  std::unique_ptr<MemoryDumpManagerDelegate> delegate_;
+  // Callback provided by the embedder to handle global dump requests.
+  RequestGlobalDumpCallback request_dump_callback_;

[Primiano Tucci (use gerrit), 2017/04/19 15:44:34]

s/callback/function/

[ssid, 2017/04/20 01:18:21]

Done.

 
-  // Protects from concurrent accesses to the |dump_providers_*| and |delegate_|
-  // to guard against disabling logging while dumping on another thread.
+  // True when current process coordinates the periodic dump triggering.
+  bool is_coordinator_;
+
+  // Protects from concurrent accesses to the |dump_providers_*| and
+  // |is_coordinator_| to guard against disabling logging while dumping on

[Primiano Tucci (use gerrit), 2017/04/19 15:44:34]

I think over time more stuff got serialized aroudn this lock (the dump_thread_
iirc) so I'd probably just say: "Protects form concurrent access to the local
state, e.g. to guard against ..."

[ssid, 2017/04/20 01:18:20]

Done.

+  // another thread.
   Lock lock_;
 
   // Optimization to avoid attempting any memory dump (i.e. to not walk an empty
   // dump_providers_enabled_ list) when tracing is not enabled.
   subtle::AtomicWord memory_tracing_enabled_;
 
   // Thread used for MemoryDumpProviders which don't specify a task runner
   // affinity.
   std::unique_ptr<Thread> dump_thread_;
 
   // The unique id of the child process. This is created only for tracing and is
   // expected to be valid only when tracing is enabled.
   uint64_t tracing_process_id_;
 
   // When true, calling |RegisterMemoryDumpProvider| is a no-op.
   bool dumper_registrations_ignored_for_testing_;
 
   // Whether new memory dump providers should be told to enable heap profiling.
   bool heap_profiling_enabled_;
 
   DISALLOW_COPY_AND_ASSIGN(MemoryDumpManager);
 };
 
-// The delegate is supposed to be long lived (read: a Singleton) and thread
-// safe (i.e. should expect calls from any thread and handle thread hopping).
-class BASE_EXPORT MemoryDumpManagerDelegate {
- public:
-  MemoryDumpManagerDelegate() {}
-  virtual ~MemoryDumpManagerDelegate() {}
-
-  virtual void RequestGlobalMemoryDump(
-      const MemoryDumpRequestArgs& args,
-      const GlobalMemoryDumpCallback& callback) = 0;
-
-  virtual bool IsCoordinator() const = 0;
-
- protected:
-  void CreateProcessDump(const MemoryDumpRequestArgs& args,
-                         const ProcessMemoryDumpCallback& callback) {
-    MemoryDumpManager::GetInstance()->CreateProcessDump(args, callback);
-  }
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(MemoryDumpManagerDelegate);
-};
-
 }  // namespace trace_event
 }  // namespace base
 
 #endif  // BASE_TRACE_EVENT_MEMORY_DUMP_MANAGER_H_