Enable spare RenderProcessHost to be preinitialized.

content/browser/renderer_host/render_process_host_impl.cc

Index: content/browser/renderer_host/render_process_host_impl.cc
diff --git a/content/browser/renderer_host/render_process_host_impl.cc b/content/browser/renderer_host/render_process_host_impl.cc
index 67c5e5d64a18b8230867611b904860f3603a688c..3460b783cfc5b0d6d824ae1f24b7a49c40b4c59c 100644
--- a/content/browser/renderer_host/render_process_host_impl.cc
+++ b/content/browser/renderer_host/render_process_host_impl.cc
@@ -463,6 +463,129 @@ class SessionStorageHolder : public base::SupportsUserData::Data {
   DISALLOW_COPY_AND_ASSIGN(SessionStorageHolder);
 };
 
+// This class manages spare RenderProcessHosts.
+//
+// There is a singleton instance of this class which manages a single spare
+// renderer (g_spare_render_process_host_manager, below). This class
+// encapsulates the implementation of
+// RenderProcessHost::WarmupSpareRenderProcessHost()
+//
+// RenderProcessHostImpl should call
+// SpareRenderProcessHostManager::MaybeTakeSpareRenderProcessHost when creating
+// a new RPH. In this implementation, the spare renderer is bound to a
+// BrowserContext and its default StoragePartition. If
+// MaybeTakeSpareRenderProcessHost is called with a BrowserContext and
+// StoragePartition that does not match, the spare renderer is discarded. In
+// particular, only the default StoragePartition will be able to use a spare
+// renderer. The spare renderer will also not be used as a guest renderer
+// (is_for_guests_ == true).
+//
+// It is safe to call WarmupSpareRenderProcessHost multiple times, although if
+// called in a context where the spare renderer is not likely to be used
+// performance may suffer due to the unnecessary RPH creation.
+class SpareRenderProcessHostManager : public RenderProcessHostObserver {
+ public:
+  SpareRenderProcessHostManager() {}
+
+  void WarmupSpareRenderProcessHost(BrowserContext* browser_context) {
+    StoragePartitionImpl* current_partition =
+        static_cast<StoragePartitionImpl*>(
+            BrowserContext::GetStoragePartition(browser_context, nullptr));
+
+    if (spare_render_process_host_ &&
+        matching_browser_context_ == browser_context &&
+        matching_storage_partition_ == current_partition)
+      return;  // Nothing to warm up.
+
+    CleanupSpareRenderProcessHost();
+
+    // Don't create a spare renderer if we're running a single renderer or if

[Charlie Reis, 2017/06/29 21:09:17]

nit: s/running a single renderer/using --single-process/

[mattcary, 2017/06/30 12:37:56]

Done.

+    // we've got too many processes. See also ShouldTryToUseExistingProcessHost
+    // in this file.
+    if (RenderProcessHost::run_renderer_in_process() ||
+        g_all_hosts.Get().size() >=
+            RenderProcessHostImpl::GetMaxRendererProcessCount())
+      return;
+
+    matching_browser_context_ = browser_context;
+    matching_storage_partition_ = current_partition;
+
+    spare_render_process_host_ = RenderProcessHostImpl::CreateRenderProcessHost(
+        browser_context, current_partition, false /* is_for_guests_only */);
+    spare_render_process_host_->AddObserver(this);
+    spare_render_process_host_->Init();
+  }
+
+  RenderProcessHost* MaybeTakeSpareRenderProcessHost(
+      const BrowserContext* browser_context,
+      const StoragePartition* partition,
+      bool is_for_guests_only) {
+    if (!spare_render_process_host_ ||
+        browser_context != matching_browser_context_ ||
+        partition != matching_storage_partition_ || is_for_guests_only) {
+      // As a new RenderProcessHost will almost certainly be created, we cleanup
+      // the non-matching one so as not to waste resources.
+      CleanupSpareRenderProcessHost();
+      return nullptr;
+    }
+
+    RenderProcessHost* rph = spare_render_process_host_;

[Charlie Reis, 2017/06/29 21:09:16]

As I mentioned earlier, I think we should include checks here for the following
things now that we're handling the random reuse case:
 - listeners_.IsEmpty() should be true
 - GetWorkerRefCount() should be 0
 - pending_views_ should be 0

If those fail, it means we're putting things in the spare RPH without meaning
to.  I'd suggest having them be CHECKs, since DCHECKs won't fire in the wild and
we won't learn anything unless existing tests happen to hit them.

[mattcary, 2017/06/30 12:37:56]

Done. I had thought that was in the context of not using a spare renderer in
GetExistingProccessHost which is not what we ended up doing. I grok now.

This required extending the RPHImpl public interface. I think that I properly
characterized the difference between what we want to do and the existing
RPH::IsUnused() flag, but can you confirm?

+    DropSpareRenderProcessHost(spare_render_process_host_);
+    return rph;
+  }
+
+  // Remove |host| as a possible spare renderer. Does not shut it down cleanly;
+  // the assumption is that the host was shutdown somewhere else and has
+  // notifying the SpareRenderProcessHostManager.
+  void DropSpareRenderProcessHost(RenderProcessHost* host) {
+    if (spare_render_process_host_ && spare_render_process_host_ == host) {
+      spare_render_process_host_->RemoveObserver(this);
+      spare_render_process_host_ = nullptr;
+    }
+  }
+
+  RenderProcessHost* spare_render_process_host() {
+    return spare_render_process_host_;
+  }
+
+ private:
+  // RenderProcessHostObserver
+  void RenderProcessWillExit(RenderProcessHost* host) override {
+    DropSpareRenderProcessHost(host);
+  }
+
+  void RenderProcessExited(RenderProcessHost* host,
+                           base::TerminationStatus unused_status,
+                           int unused_exit_code) override {
+    DropSpareRenderProcessHost(host);
+  }
+
+  void RenderProcessHostDestroyed(RenderProcessHost* host) override {
+    DropSpareRenderProcessHost(host);
+  }
+
+  void CleanupSpareRenderProcessHost() {
+    if (spare_render_process_host_) {
+      spare_render_process_host_->Cleanup();
+      DropSpareRenderProcessHost(spare_render_process_host_);
+    }
+  }
+
+  // This is a bare pointer, because RenderProcessHost manages the lifetime of
+  // all its instances; see g_all_hosts, above.
+  RenderProcessHost* spare_render_process_host_;
+
+  // Used only to check if a creation request matches the spare, and not
+  // accessed.
+  const BrowserContext* matching_browser_context_ = nullptr;
+  const StoragePartition* matching_storage_partition_ = nullptr;
+
+  DISALLOW_COPY_AND_ASSIGN(SpareRenderProcessHostManager);
+};
+
+base::LazyInstance<SpareRenderProcessHostManager>::Leaky
+    g_spare_render_process_host_manager = LAZY_INSTANCE_INITIALIZER;
+
 const void* const kDefaultSubframeProcessHostHolderKey =
     &kDefaultSubframeProcessHostHolderKey;
 
@@ -476,17 +599,17 @@ class DefaultSubframeProcessHostHolder : public base::SupportsUserData::Data,
   // Gets the correct render process to use for this SiteInstance.
   RenderProcessHost* GetProcessHost(SiteInstance* site_instance,
                                     bool is_for_guests_only) {
-    StoragePartition* default_partition =
-        BrowserContext::GetDefaultStoragePartition(browser_context_);
-    StoragePartition* partition =
-        BrowserContext::GetStoragePartition(browser_context_, site_instance);
+    StoragePartitionImpl* default_partition =
+        static_cast<StoragePartitionImpl*>(
+            BrowserContext::GetDefaultStoragePartition(browser_context_));
+    StoragePartitionImpl* partition = static_cast<StoragePartitionImpl*>(
+        BrowserContext::GetStoragePartition(browser_context_, site_instance));
 
     // Is this the default storage partition? If it isn't, then just give it its
     // own non-shared process.
     if (partition != default_partition || is_for_guests_only) {
-      RenderProcessHostImpl* host = new RenderProcessHostImpl(
-          browser_context_, static_cast<StoragePartitionImpl*>(partition),
-          is_for_guests_only);
+      RenderProcessHost* host = RenderProcessHostImpl::CreateRenderProcessHost(
+          browser_context_, partition, is_for_guests_only);
       host->SetIsNeverSuitableForReuse();
       return host;
     }
@@ -496,9 +619,8 @@ class DefaultSubframeProcessHostHolder : public base::SupportsUserData::Data,
     if (host_)
       return host_;
 
-    host_ = new RenderProcessHostImpl(
-        browser_context_, static_cast<StoragePartitionImpl*>(partition),
-        false /* for guests only */);
+    host_ = RenderProcessHostImpl::CreateOrUseSpareRenderProcessHost(
+        browser_context_, partition, false /* is for guests only */);
     host_->SetIsNeverSuitableForReuse();
     host_->AddObserver(this);
 
@@ -517,7 +639,7 @@ class DefaultSubframeProcessHostHolder : public base::SupportsUserData::Data,
 
   // The default subframe render process used for the default storage partition
   // of this BrowserContext.
-  RenderProcessHostImpl* host_ = nullptr;
+  RenderProcessHost* host_ = nullptr;
 };
 
 void CreateMemoryCoordinatorHandle(
@@ -1029,6 +1151,38 @@ void RenderProcessHost::SetMaxRendererProcessCount(size_t count) {
   g_max_renderer_count_override = count;
 }
 
+// static
+RenderProcessHost* RenderProcessHostImpl::CreateRenderProcessHost(
+    BrowserContext* browser_context,
+    StoragePartitionImpl* storage_partition_impl,
+    bool is_for_guests_only) {
+  if (g_render_process_host_factory_) {
+    return g_render_process_host_factory_->CreateRenderProcessHost(
+        browser_context);
+  }
+
+  return new RenderProcessHostImpl(browser_context, storage_partition_impl,
+                                   is_for_guests_only);
+}
+
+// static
+RenderProcessHost* RenderProcessHostImpl::CreateOrUseSpareRenderProcessHost(
+    BrowserContext* browser_context,
+    StoragePartitionImpl* storage_partition_impl,
+    bool is_for_guests_only) {
+  RenderProcessHost* render_process_host =
+      g_spare_render_process_host_manager.Get().MaybeTakeSpareRenderProcessHost(
+          browser_context, storage_partition_impl, is_for_guests_only);
+
+  if (!render_process_host) {
+    render_process_host = CreateRenderProcessHost(
+        browser_context, storage_partition_impl, is_for_guests_only);
+  }
+
+  DCHECK(render_process_host);
+  return render_process_host;
+}
+
 RenderProcessHostImpl::RenderProcessHostImpl(
     BrowserContext* browser_context,
     StoragePartitionImpl* storage_partition_impl,
@@ -2113,6 +2267,12 @@ void RenderProcessHostImpl::RemoveExpectedNavigationToSite(
   tracker->DecrementSiteProcessCount(site_url, render_process_host->GetID());
 }
 
+// static
+RenderProcessHost*
+RenderProcessHostImpl::GetSpareRenderProcessHostForTesting() {
+  return g_spare_render_process_host_manager.Get().spare_render_process_host();
+}
+
 bool RenderProcessHostImpl::IsForGuestsOnly() const {
   return is_for_guests_only_;
 }
@@ -3066,6 +3226,13 @@ bool RenderProcessHostImpl::IsSuitableHost(RenderProcessHost* host,
   return GetContentClient()->browser()->IsSuitableHost(host, site_url);
 }
 
+// static
+void RenderProcessHost::WarmupSpareRenderProcessHost(
+    content::BrowserContext* browser_context) {
+  g_spare_render_process_host_manager.Get().WarmupSpareRenderProcessHost(
+      browser_context);
+}
+
 // static
 bool RenderProcessHost::run_renderer_in_process() {
   return g_run_renderer_in_process_;
@@ -3144,6 +3311,8 @@ RenderProcessHost* RenderProcessHost::GetExistingProcessHost(
   if (!suitable_renderers.empty()) {
     int suitable_count = static_cast<int>(suitable_renderers.size());
     int random_index = base::RandInt(0, suitable_count - 1);
+    g_spare_render_process_host_manager.Get().DropSpareRenderProcessHost(
+        suitable_renderers[random_index]);

[Charlie Reis, 2017/06/29 21:09:16]

Sure, that seems like an ok way to handle it.  Please include a comment, like:

If this happened to be the spare RenderProcessHost, don't treat it as a spare
anymore.

[mattcary, 2017/06/30 12:37:56]

Done.

     return suitable_renderers[random_index];
   }
 
@@ -3270,18 +3439,12 @@ RenderProcessHost* RenderProcessHostImpl::GetProcessHostForSiteInstance(
     render_process_host = GetExistingProcessHost(browser_context, site_url);
   }
 
-  // Otherwise (or if that fails), create a new one.
+  // Before creating a new process, try to use a spare process if one is found.
+  StoragePartitionImpl* partition = static_cast<StoragePartitionImpl*>(
+      BrowserContext::GetStoragePartition(browser_context, site_instance));
   if (!render_process_host) {
-    if (g_render_process_host_factory_) {
-      render_process_host =
-          g_render_process_host_factory_->CreateRenderProcessHost(
-              browser_context);
-    } else {
-      StoragePartitionImpl* partition = static_cast<StoragePartitionImpl*>(
-          BrowserContext::GetStoragePartition(browser_context, site_instance));
-      render_process_host = new RenderProcessHostImpl(
-          browser_context, partition, is_for_guests_only);
-    }
+    render_process_host = CreateOrUseSpareRenderProcessHost(
+        browser_context, partition, is_for_guests_only);
   }
 
   if (is_unmatched_service_worker) {