memory-infra: Finish moving to Mojo (2nd attempt)

services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc

 // Copyright 2017 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 "services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.h"
 
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/lazy_instance.h"
+#include "base/single_thread_task_runner.h"
+#include "base/synchronization/lock.h"
 #include "base/trace_event/memory_dump_request_args.h"
 #include "mojo/public/cpp/bindings/interface_request.h"
 #include "services/resource_coordinator/public/cpp/memory/coordinator.h"
+#include "services/resource_coordinator/public/interfaces/memory/constants.mojom.h"
 #include "services/resource_coordinator/public/interfaces/memory/memory_instrumentation.mojom.h"
 #include "services/service_manager/public/cpp/interface_provider.h"
 
 namespace memory_instrumentation {
 
-MemoryDumpManagerDelegateImpl::MemoryDumpManagerDelegateImpl(
-    service_manager::InterfaceProvider* interface_provider)
-    : is_coordinator_(false), binding_(this) {
+namespace {
+
+base::LazyInstance<MemoryDumpManagerDelegateImpl>::Leaky
+    g_memory_dump_manager_delegate = LAZY_INSTANCE_INITIALIZER;
+
+}  // namespace
+
+// static
+MemoryDumpManagerDelegateImpl* MemoryDumpManagerDelegateImpl::GetInstance() {
+  return g_memory_dump_manager_delegate.Pointer();
+}
+
+MemoryDumpManagerDelegateImpl::MemoryDumpManagerDelegateImpl()
+    : initialized_(false),
+      binding_(this),
+      task_runner_(nullptr),
+      pending_memory_dump_guid_(0) {}
+
+MemoryDumpManagerDelegateImpl::~MemoryDumpManagerDelegateImpl() {}
+
+void MemoryDumpManagerDelegateImpl::InitializeWithInterfaceProvider(
+    service_manager::InterfaceProvider* interface_provider) {
+  {
+    base::AutoLock lock(initialized_lock_);

[Primiano Tucci (use gerrit), 2017/03/08 12:05:43]

Hmm this is a bit of an odd pattern and honestly smells a bit like an unclear
initialization sequence.
Honestly I don't think here you really need to separate instance creation and
initialization.
I think all this would be more readable and easier to follow if you did
something like this:

- Have a static Method called MemoryDumpManagerImpl::CreateInstance(const
MemoryDumpManagerImpl::Config&) and just forward the config to the real ctor
where MemoryDumpManagerImpl::Config is a simple struct that takes all the arg
required for both cases.

- Use a plain static local which is thread safe in C++11, so you can also get
rid of the LazyInstance above, i.e.
MemoryDumpManagerImpl* MemoryDumpManagerImpl::Create(const
MemoryDumpManagerImpl::Config&) {
   static MemoryDumpManagerImpl* instance = new MemoryDumpManagerImpl(config);
   DCHECK(instance.config == config)
   return instance; 
}
this will make it structurally impossible to initialize the impl twice and will
remove the confusion that arises when you think about "what happens if you
GetInstance() before initialize()?" because now you have only CreateInstance and
no GetInstance (nor initialize) anymore :)

 

[chiniforooshan, 2017/03/08 19:03:43]

I learned about static locals today. Thanks!

One reason for this complexity was that when we initialize the delegate from the
coordinator process, we have to initialize it on the UI thread (the same thread
that binds to remote requests). That's why InitializeWithCoordinator does a
PostTask. Of course, we cannot PostTask in the constructor. But, I think since
your suggestion makes the code a lot cleaner, it's reasonable to move that
requirement to whoever calls the Create method; we have DCHECKs for this in the
Coordinator.

Done.

+    DCHECK(!initialized_);
+    initialized_ = true;
+  }
+
   interface_provider->GetInterface(mojo::MakeRequest(&coordinator_));
   coordinator_->RegisterProcessLocalDumpManager(
       binding_.CreateInterfacePtrAndBind());
+  base::trace_event::MemoryDumpManager::GetInstance()->Initialize(this);
 }
 
-MemoryDumpManagerDelegateImpl::MemoryDumpManagerDelegateImpl(
-    Coordinator* coordinator)
-    : is_coordinator_(true), binding_(this) {
+void MemoryDumpManagerDelegateImpl::InitializeWithCoordinator(
+    Coordinator* coordinator,
+    scoped_refptr<base::SingleThreadTaskRunner> task_runner) {
+  DCHECK(task_runner);
+  if (!task_runner->RunsTasksOnCurrentThread()) {
+    task_runner->PostTask(
+        FROM_HERE,
+        base::Bind(&MemoryDumpManagerDelegateImpl::InitializeWithCoordinator,
+                   base::Unretained(this), base::Unretained(coordinator),
+                   task_runner));
+    return;
+  }
+
+  {
+    base::AutoLock lock(initialized_lock_);
+    DCHECK(!initialized_);
+    initialized_ = true;
+  }
+
+  task_runner_ = task_runner;
   coordinator->BindCoordinatorRequest(mojo::MakeRequest(&coordinator_));
   coordinator_->RegisterProcessLocalDumpManager(
       binding_.CreateInterfacePtrAndBind());
+  base::trace_event::MemoryDumpManager::GetInstance()->set_tracing_process_id(
+      mojom::kServiceTracingProcessId);

[ssid, 2017/03/08 18:37:13]

How does this fix the problem of browser process not getting the right TPID in
some cases?
Why do we assign the same TPID for all processes? Shouldn't this be setting
invalid process id instead?

[chiniforooshan, 2017/03/08 22:39:03]

Note that InitializeWithCoordinator is only called from the browser process.
Children connect remotely and so call InitializeWithInterfaceProvider. So,
mojom::kServiceTracingProcessId is set only if we are in the browser process
(method names are changed after I applied Primiano's comments. But, the concept
is the same, we call set_tracing_process_id only if we are the coordinator.

For children, the initial value of TPID is set in
//base/trace_event/memory_dump_manager, and that's where
kInvalidTracingProcessId is defined.

I moved set_tracing_process_id here so that it's more clear that we initialize
TPID on the browser when Coordinator is created.

About the fix: My understanding of
https://cs.chromium.org/chromium/src/content/app/content_main_runner.cc?dr=C&...
was that BrowserMain does not run on Android. So, instead of setting TPID in
BrowserMain and creating Coordinator lazily (first CL), now I create the
Coordinator in content::BrowserMainRunnerImpl::Initialize (which is always run,
I think) and Coordinator sets TPID when its created.

[ssid, 2017/03/09 00:23:15]

Ah sorry. I thought they both call the same function, got mixed up with the
names.

+  base::trace_event::MemoryDumpManager::GetInstance()->Initialize(this);
 }
 
-MemoryDumpManagerDelegateImpl::~MemoryDumpManagerDelegateImpl() {}
-
 bool MemoryDumpManagerDelegateImpl::IsCoordinator() const {
-  return is_coordinator_;
+  return task_runner_ != nullptr;
 }
 
 void MemoryDumpManagerDelegateImpl::RequestProcessMemoryDump(
     const base::trace_event::MemoryDumpRequestArgs& args,
     const RequestProcessMemoryDumpCallback& callback) {
   MemoryDumpManagerDelegate::CreateProcessDump(args, callback);
 }
 
 void MemoryDumpManagerDelegateImpl::RequestGlobalMemoryDump(
     const base::trace_event::MemoryDumpRequestArgs& args,
     const base::trace_event::MemoryDumpCallback& callback) {
-  coordinator_->RequestGlobalMemoryDump(args, callback);
+  // Note: This condition is here to match the old behavior. If the delegate is

[Primiano Tucci (use gerrit), 2017/03/08 12:05:43]

Ah, TIL. Not sure this would ever intended. I think once this sticks we might
want to drop this special case and throttle everything (unless I missed some
piece of the discussion and this behavior is really needed) .
Can you just add a todo in this cl as a reminder to simplify this?

[ssid, 2017/03/08 18:37:13]

Some unittests are assuming that the child does ont queue memory dumps. Fixing
them would cause much larger CL. There is already a TODO below to fix the
unittests in next CL.

[chiniforooshan, 2017/03/08 19:03:43]

Done.

+  // in the browser process, we do not drop parallel requests in the delegate
+  // and so they will be queued by the Coordinator service (see
+  // CoordinatorImpl::RequestGlobalMemoryDump). If the delegate is in a child
+  // process, parallel requests will be cancelled.
+  //
+  // TODO(chiniforooshan): After transitioning to the mojo-based service is
+  // completed, we should enable queueing parallel global memory dump requests
+  // by delegates on all processes.
+  if (task_runner_) {

[Primiano Tucci (use gerrit), 2017/03/08 12:05:43]

I think that IsCoordinator()  here woul make this if statement more readable

[ssid, 2017/03/08 18:37:13]

Sorry about this. I asked to remove is_coordinator here because i was expecting
there should be no difference between the child and corrdinator's delegates. But
apparently we need the task runner. In this case, I agree with primiano about
having the coordinator back.
Sorry about the back and forth.

[chiniforooshan, 2017/03/08 19:03:43]

Done.

+    DCHECK(task_runner_);

[Primiano Tucci (use gerrit), 2017/03/08 12:05:43]

uh? how can this fail given the if above?

[chiniforooshan, 2017/03/08 19:03:43]

Done.

+    task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(&mojom::Coordinator::RequestGlobalMemoryDump,
+                   base::Unretained(coordinator_.get()), args, callback));
+    return;
+  }
+
+  {
+    base::AutoLock lock(pending_memory_dump_guid_lock_);
+    if (pending_memory_dump_guid_) {
+      callback.Run(args.dump_guid, false);
+      return;
+    }
+    pending_memory_dump_guid_ = args.dump_guid;
+  }
+  DCHECK(!task_runner_);
+  auto callback_proxy =
+      base::Bind(&MemoryDumpManagerDelegateImpl::MemoryDumpCallbackProxy,
+                 base::Unretained(this), callback);
+  coordinator_->RequestGlobalMemoryDump(args, callback_proxy);
+}
+
+void MemoryDumpManagerDelegateImpl::MemoryDumpCallbackProxy(
+    const base::trace_event::MemoryDumpCallback& callback,
+    uint64_t dump_guid,
+    bool success) {
+  DCHECK_NE(0U, pending_memory_dump_guid_);
+  pending_memory_dump_guid_ = 0;
+  callback.Run(dump_guid, success);
+}
+
+void MemoryDumpManagerDelegateImpl::SetAsNonCoordinatorForTesting() {
+  task_runner_ = nullptr;
 }
 
 }  // namespace memory_instrumentation