Use persistent memory for receiving metrics from sub-processes.

content/browser/browser_child_process_host_impl.cc

Index: content/browser/browser_child_process_host_impl.cc
diff --git a/content/browser/browser_child_process_host_impl.cc b/content/browser/browser_child_process_host_impl.cc
index afc52015320fecafb9abc463abf4a979e56945f4..c5f61aba6b38953d91fcdbaa7818bf06ea3a8059 100644
--- a/content/browser/browser_child_process_host_impl.cc
+++ b/content/browser/browser_child_process_host_impl.cc
@@ -15,6 +15,8 @@
 #include "base/macros.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram.h"
+#include "base/metrics/persistent_histogram_allocator.h"
+#include "base/metrics/persistent_memory_allocator.h"
 #include "base/stl_util.h"
 #include "base/strings/string_util.h"
 #include "base/synchronization/waitable_event.h"
@@ -84,15 +86,15 @@ BrowserChildProcessHost* BrowserChildProcessHost::Create(
     content::ProcessType process_type,
     BrowserChildProcessHostDelegate* delegate) {
   return new BrowserChildProcessHostImpl(
-      process_type, delegate, mojo::edk::GenerateRandomToken());
+      process_type, delegate, mojo::edk::GenerateRandomToken(), 0, nullptr);
 }
 
 BrowserChildProcessHost* BrowserChildProcessHost::Create(
     content::ProcessType process_type,
     BrowserChildProcessHostDelegate* delegate,
     const std::string& mojo_child_token) {
-  return new BrowserChildProcessHostImpl(
-      process_type, delegate, mojo_child_token);
+  return new BrowserChildProcessHostImpl(process_type, delegate,
+                                         mojo_child_token, 0, nullptr);
 }
 
 BrowserChildProcessHost* BrowserChildProcessHost::FromID(int child_process_id) {
@@ -135,7 +137,9 @@ void BrowserChildProcessHostImpl::RemoveObserver(
 BrowserChildProcessHostImpl::BrowserChildProcessHostImpl(
     content::ProcessType process_type,
     BrowserChildProcessHostDelegate* delegate,
-    const std::string& mojo_child_token)
+    const std::string& mojo_child_token,
+    size_t shared_metrics_memory_size,
+    base::StringPiece shared_metrics_name)
     : data_(process_type),
       delegate_(delegate),
       mojo_child_token_(mojo_child_token),
@@ -168,6 +172,16 @@ BrowserChildProcessHostImpl::BrowserChildProcessHostImpl(
   GetContentClient()->browser()->BrowserChildProcessHostCreated(this);
 
   power_monitor_message_broadcaster_.Init();
+
+  // Create a persistent memory segment for subprocess histograms only if
+  // they're active in the browser.
+  if (shared_metrics_memory_size > 0 && base::GlobalHistogramAllocator::Get()) {
+    std::unique_ptr<base::SharedMemory> shm(new base::SharedMemory());
+    shm->CreateAndMapAnonymous(shared_metrics_memory_size);
+    metrics_allocator_.reset(new base::SharedPersistentMemoryAllocator(
+        std::move(shm), static_cast<uint64_t>(data_.id), shared_metrics_name,
+        /*readonly=*/false));

[Alexei Svitkine (slow), 2016/08/12 23:14:36]

I think it would be better to just have the the BrowserChildProcessHostImpl ctor
take the base::SharedPersistentMemoryAllocator object, as opposed to 2 params.

In that case, move this bit of logic to a helper function that can be invoked
from the place where the ctor is being called.

[bcwhite, 2016/08/15 19:43:10]

I don't think that would be good.  That would force the callers to know about
persistent classes and the like which seems unnecessary when the only real
things it has to provide is "how much" and "what's it called".

[Alexei Svitkine (slow), 2016/08/18 07:21:19]

The callers wouldn't need to know the persistent classes if they use a helper
function. The current implementation makes this class know about all the details
of how to create a SharedPersistentMemoryAllocator, whereas this seems like a
detail that this class shouldn't concern itself with.

How about making a helper function that takes "how much" and "what it's called"
and returns the allocator - and that function can be called at the callsites or
nullptr be passed. The function can live somewhere in persistent metrics code,
so only that code needs to know the details of how to set it up.

[bcwhite, 2016/08/18 15:21:55]

This class knows about creating shared memory because this class is what passes
that shared memory to the newly created process.  Creating an allocator on top
of it already does use a helper-function (actually a helper-class) named
SharedPersistentMemoryAllocator.
This class needs to know about an allocator (so it can give it away) and how
it's based on shared memory (so it can pass that memory to the child process). 
There doesn't seem to be any benefit in spreading that knowledge and their
inter-relationship to additional modules.  This is also the only place that
knows the process ID (line 150) which is used as an identifier inside the memory
allocator -- nothing passing in an allocator as a parameter could get that
because it hasn't yet been determined.

[Alexei Svitkine (slow), 2016/08/18 15:57:28]

Let me step back a bit. The concrete things I'm not a fan of with the current
proposed implementation:


  1. The block of code in BrowserChildProcessHostImpl cares a lot about the
persistent memory details. I'd rather it be a helper function that just takes
id, metrics memory name and size and return it.
  2. The magic numbers introduced in callers of the ctor - i.e. the code in
gpu_process_host.cc now has magic constants (name and size in it).

Both of those make the existing code that currently doesn't need to care very
much about the persistent memory stuff be deeply involved with it. I'd rather
not have us dilute the focus of those files.

Could we perhaps just introduce a "child_process_persistent_memory.cc" file that
has a CreatePersistentMemory(content::ProcessType process_type) function? Then,
that function could have the full logic - including switching on the process
type to get the name/size?

If you add such a function, then maybe you don't even need any change to the
ctor signature - since it can be called directly from the ctor impl.

[bcwhite, 2016/08/18 17:42:17]

It cares about the shared memory details but that's because it is what is
sharing the memory.  Since this class is doing the sharing and doing the holding
and the passing to whatever "takes" it, it seems wrong to move the creation of
it somewhere else.

Creating the allocator on top of it is not significant enough to be separated.
I thought it would be better to keep constants related to a specific process
type with that specific process type class.  However, if you'd prefer it all in
one place, I can write that static helper function inside this class -- no need
for a new file.

[Alexei Svitkine (slow), 2016/08/19 15:10:51]

I see your point. I still don't like that this class needs to know about the
GlobalHistogramAllocator object not being null. And it actually seems error
prone to require callers to know this - what happens if someone forgets to make
that check somehow?

Perhaps it would make more sense to have a static factory function to create a
SharedPersistentMemoryAllocator? This could live in the base metrics code and
just take the id, name and size as params.

Then, this block would be much simpler:

metrics_allocator_.reset(base::SharedPersistentMemoryAllocator::Create(shared_metrics_name,
shared_metrics_memory_size, static_cast<uint64_t>(data_.id));

And that create function will do all the details such as checking size and
presence of the global allocator and creating SharedMemory instance. This way,
this class needs to know much less details of the persistent memory
implementation.
Thinking about this more - do we really need the name as a param? I think we can
just use  GetProcessTypeNameInEnglish() on the process type and use that. (And
maybe preprocess that to remove spaces.)

Then, that simplifies the ctor API some.

As to keeping the size param with the callers, I guess that part is OK. Maybe
instead of inlining it, which makes it very cryptic, pull it out into a constant
at the top of each file with a relevant comment.

[bcwhite, 2016/08/19 15:55:21]

Eventually that will be removed, but it's used here and elsewhere instead of
checking for the existence of the field-trial.  I'll add a TODO to remove it.
I suppose.  Could those names ever change?  It would break the metrics reporting
if they did but the histograms.xml file wasn't updated to match.
Heh.  I came to your way of thinking that embedding all that in a single place
would keep the API simple and be easier to maintain.  :-)

[bcwhite, 2016/08/19 15:55:21]

New separate method for creating the allocator that determines parameters based
on process type rather than them being passed in.

+  }
 }
 
 BrowserChildProcessHostImpl::~BrowserChildProcessHostImpl() {
@@ -268,6 +282,11 @@ const base::Process& BrowserChildProcessHostImpl::GetProcess() const {
   return child_process_->GetProcess();
 }
 
+std::unique_ptr<base::SharedPersistentMemoryAllocator>
+BrowserChildProcessHostImpl::TakeMetricsAllocator() {
+  return std::move(metrics_allocator_);
+}
+
 void BrowserChildProcessHostImpl::SetName(const base::string16& name) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   data_.name = name;
@@ -335,6 +354,7 @@ void BrowserChildProcessHostImpl::OnChannelConnected(int32_t peer_pid) {
   delegate_->OnChannelConnected(peer_pid);
 
   if (IsProcessLaunched()) {
+    ShareMetricsAllocatorToProcess();
     BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
                             base::Bind(&NotifyProcessLaunchedAndConnected,
                                        data_));
@@ -436,6 +456,16 @@ bool BrowserChildProcessHostImpl::Send(IPC::Message* message) {
   return child_process_host_->Send(message);
 }
 
+void BrowserChildProcessHostImpl::ShareMetricsAllocatorToProcess() {
+  if (metrics_allocator_) {
+    base::SharedMemoryHandle shm_handle;
+    metrics_allocator_->shared_memory()->ShareToProcess(data_.handle,
+                                                        &shm_handle);
+    Send(new ChildProcessMsg_SetHistogramMemory(
+        shm_handle, metrics_allocator_->shared_memory()->mapped_size()));
+  }
+}
+
 void BrowserChildProcessHostImpl::OnProcessLaunchFailed(int error_code) {
   delegate_->OnProcessLaunchFailed(error_code);
   notify_child_disconnected_ = false;
@@ -462,6 +492,7 @@ void BrowserChildProcessHostImpl::OnProcessLaunched() {
   delegate_->OnProcessLaunched();
 
   if (is_channel_connected_) {
+    ShareMetricsAllocatorToProcess();
     BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
                             base::Bind(&NotifyProcessLaunchedAndConnected,
                                        data_));