Change multi-prerender API to include per launcher slots.

chrome/browser/prerender/prerender_link_manager.cc

Index: chrome/browser/prerender/prerender_link_manager.cc
diff --git a/chrome/browser/prerender/prerender_link_manager.cc b/chrome/browser/prerender/prerender_link_manager.cc
index 8b2050f4f6a816c58f9811aac9971a932fab380e..59eba402ba331b7115e546e9a077635fee6d418f 100644
--- a/chrome/browser/prerender/prerender_link_manager.cc
+++ b/chrome/browser/prerender/prerender_link_manager.cc
@@ -5,6 +5,7 @@
 #include "chrome/browser/prerender/prerender_link_manager.h"
 
 #include <limits>
+#include <set>
 #include <utility>
 
 #include "base/memory/scoped_ptr.h"
@@ -41,125 +42,214 @@ void Send(int child_id, IPC::Message* raw_message) {
 namespace prerender {
 
 PrerenderLinkManager::PrerenderLinkManager(PrerenderManager* manager)
-    : manager_(manager) {
+    : is_shutdown_(false),
+      manager_(manager) {
 }
 
 PrerenderLinkManager::~PrerenderLinkManager() {
-  for (IdPairToPrerenderHandleMap::iterator it = ids_to_handle_map_.begin();
-       it != ids_to_handle_map_.end();
-       ++it) {
-    PrerenderHandle* prerender_handle = it->second;
-    DCHECK(!prerender_handle->IsPrerendering())
-        << "All running prerenders should stop at the same time as the "
-        << "PrerenderManager.";
-    delete prerender_handle;
+  for (std::list<Prerender>::iterator i = prerenders_.begin();
+       i != prerenders_.end(); ++i) {
+    if (i->handle) {
+      DCHECK(!i->handle->IsPrerendering())
+          << "All running prerenders should stop at the same time as the "
+          << "PrerenderManager.";
+      delete i->handle;
+      i->handle = 0;
+    }
   }
 }
 
-bool PrerenderLinkManager::OnAddPrerender(int child_id,
+void PrerenderLinkManager::OnAddPrerender(int launcher_child_id,
                                           int prerender_id,
                                           const GURL& url,
                                           const content::Referrer& referrer,
                                           const gfx::Size& size,
                                           int render_view_route_id) {
-  DVLOG(2) << "OnAddPrerender, child_id = " << child_id
-           << ", prerender_id = " << prerender_id
-           << ", url = " << url.spec();
-  DVLOG(3) << "... referrer url = " << referrer.url.spec()
-           << ", size = (" << size.width() << ", " << size.height() << ")"
-           << ", render_view_route_id = " << render_view_route_id;
-
-
-  PrerenderHandle* prerender_handle =
-      manager_->AddPrerenderFromLinkRelPrerender(
-          child_id, render_view_route_id, url, referrer, size);
-  if (!prerender_handle)
-    return false;
-
-  const ChildAndPrerenderIdPair child_and_prerender_id(child_id, prerender_id);
-  DCHECK_EQ(0u, ids_to_handle_map_.count(child_and_prerender_id));
-  ids_to_handle_map_[child_and_prerender_id] = prerender_handle;
-
-  // If we are given a prerender that is already prerendering, we have missed
-  // the start event.
-  if (prerender_handle->IsPrerendering())
-    OnPrerenderStart(prerender_handle);
-  prerender_handle->SetObserver(this);
-  return true;
+  Prerender prerender(launcher_child_id, prerender_id, url, referrer, size,
+                      render_view_route_id);
+  prerenders_.push_back(prerender);
+  StartPrerenders();
 }
 
 void PrerenderLinkManager::OnCancelPrerender(int child_id, int prerender_id) {
-  DVLOG(2) << "OnCancelPrerender, child_id = " << child_id
-           << ", prerender_id = " << prerender_id;
-  const ChildAndPrerenderIdPair child_and_prerender_id(child_id, prerender_id);
-  IdPairToPrerenderHandleMap::iterator id_to_handle_iter =
-      ids_to_handle_map_.find(child_and_prerender_id);
-  if (id_to_handle_iter == ids_to_handle_map_.end()) {
-    DVLOG(5) << "... canceling a prerender that doesn't exist.";
+  Prerender* prerender = FindByLauncherChildIdAndPrerenderId(child_id,
+                                                             prerender_id);
+  if (!prerender)
     return;
-  }
 
-  scoped_ptr<PrerenderHandle> prerender_handle(id_to_handle_iter->second);
-  ids_to_handle_map_.erase(id_to_handle_iter);
-  prerender_handle->OnCancel();
+  scoped_ptr<PrerenderHandle> own_prerender_handle(prerender->handle);
+  prerender->handle = NULL;
+  RemovePrerender(prerender);
+
+  if (own_prerender_handle)
+    own_prerender_handle->OnCancel();
+
+  StartPrerenders();
 }
 
 void PrerenderLinkManager::OnAbandonPrerender(int child_id, int prerender_id) {
-  DVLOG(2) << "OnAbandonPrerender, child_id = " << child_id
-           << ", prerender_id = " << prerender_id;
-  const ChildAndPrerenderIdPair child_and_prerender_id(child_id, prerender_id);
-  IdPairToPrerenderHandleMap::iterator id_to_handle_iter =
-      ids_to_handle_map_.find(child_and_prerender_id);
-  if (id_to_handle_iter == ids_to_handle_map_.end())
+  Prerender* prerender = FindByLauncherChildIdAndPrerenderId(child_id,
+                                                             prerender_id);
+  if (!prerender || !prerender->handle)
     return;
-  PrerenderHandle* prerender_handle = id_to_handle_iter->second;
-  prerender_handle->OnNavigateAway();
+  prerender->handle->OnNavigateAway();
+
+  // Keep the prerender in |prerenders_|, since it is still running and needs to
+  // be accounted for when tracking slots for StartPrerenders().
 }
 
 void PrerenderLinkManager::OnChannelClosing(int child_id) {
-  DVLOG(2) << "OnChannelClosing, child id = " << child_id;
-  const ChildAndPrerenderIdPair child_and_minimum_prerender_id(
-      child_id, std::numeric_limits<int>::min());
-  const ChildAndPrerenderIdPair child_and_maximum_prerender_id(
-      child_id, std::numeric_limits<int>::max());
-
-  IdPairToPrerenderHandleMap::iterator
-      it = ids_to_handle_map_.lower_bound(child_and_minimum_prerender_id);
-  IdPairToPrerenderHandleMap::iterator
-      end = ids_to_handle_map_.upper_bound(child_and_maximum_prerender_id);
-  while (it != end) {
-    IdPairToPrerenderHandleMap::iterator next = it;
+  std::list<Prerender>::iterator next = prerenders_.begin();
+  while (next != prerenders_.end()) {
+    std::list<Prerender>::iterator it = next;
     ++next;
 
-    size_t size_before_abandon = ids_to_handle_map_.size();
-    OnAbandonPrerender(child_id, it->first.second);
-    DCHECK_EQ(size_before_abandon, ids_to_handle_map_.size());
-    RemovePrerender(it);
+    if (child_id != it->launcher_child_id)
+      continue;
 
-    it = next;
+    const size_t size_before_abandon = prerenders_.size();
+    OnAbandonPrerender(child_id, it->prerender_id);
+    DCHECK_EQ(size_before_abandon, prerenders_.size());
+    RemovePrerender(&(*it));

[mmenke, 2012/12/17 20:02:23]

Erm...  This seems to violate the comment at the end of OnAbandonPrerender. 
Should we only be removing the prerenders that haven't been started, and relying
on the cancel events of the others?

As-is, it's also not clear when we'll be calling StartPrerender(), if we have
other prerenders pending and end up removing an active one from the list here,
unexpectedly.

[gavinp, 2012/12/18 00:44:14]

Guh, good point. I moved the remove up into OnAbandonPrerender, and so the
DCHECK can't be here any more. I added a DCHECK here to make sure we don't need
to start any prerenders.

   }
 }
 
+PrerenderLinkManager::Prerender::Prerender(int launcher_child_id,
+                                           int prerender_id,
+                                           const GURL& url,
+                                           const content::Referrer& referrer,
+                                           const gfx::Size& size,
+                                           int render_view_route_id)
+    : launcher_child_id(launcher_child_id),
+      prerender_id(prerender_id),
+      url(url),
+      referrer(referrer),
+      size(size),
+      render_view_route_id(render_view_route_id),
+      handle(NULL) {
+}
+
+PrerenderLinkManager::Prerender::~Prerender() {
+  DCHECK_EQ(static_cast<PrerenderHandle*>(NULL), handle);
+}
+
 bool PrerenderLinkManager::IsEmpty() const {
-  return ids_to_handle_map_.empty();
+  return prerenders_.empty();
+}
+
+void PrerenderLinkManager::StartPrerenders() {
+  if (is_shutdown_)
+    return;
+
+  std::set<std::pair<int, int> > launcher_and_prerender_id_set;
+
+  size_t running_count = 0;
+  std::multiset<std::pair<int, int> >
+      running_launcher_and_render_view_route_set;
+
+  // Scan the list, counting how many prerenders are running and how many are
+  // running for each launcher.
+  for (std::list<Prerender>::iterator i = prerenders_.begin();
+       i != prerenders_.end(); ++i) {
+    if (i->handle) {
+      ++running_count;
+      std::pair<int, int> launcher_and_render_view_route(
+          i->launcher_child_id, i->render_view_route_id);
+      running_launcher_and_render_view_route_set.insert(
+          launcher_and_render_view_route);
+      DCHECK_GE(manager_->config().max_concurrency_per_launcher,
+                running_launcher_and_render_view_route_set.count(
+                    launcher_and_render_view_route));
+    }
+
+    std::pair<int, int> launcher_and_prerender_id(i->launcher_child_id,
+                                                  i->prerender_id);
+    DCHECK_EQ(0u,
+              launcher_and_prerender_id_set.count(launcher_and_prerender_id));
+    launcher_and_prerender_id_set.insert(launcher_and_prerender_id);
+  }

[mmenke, 2012/12/17 18:34:57]

Hmm...Multiple prerenders with the same PrerenderContents count towards the max,
as do pending prerenders.

Suppose a page want to keep a prerender of another page going...and it's not
smart enough to use the API.  Could see a ton of queued prerenders resulting.

Also, I wonder about prerendering very stale prerender requests.  If a request
was issued 20 minutes ago, but we've had prerenders from other pages running
since then....should we really prerender the page?

[mmenke, 2012/12/17 20:02:23]

Also, seems like if we have one active prerender, which tries to prerender
another page, that prerender will occupy two active prerender slots, until its
cancelled.

[gavinp, 2012/12/18 00:44:14]

Hrm. I was hoping to delay adding handling for duplicates and pending prerenders
until a followup CL, but these issues make me think they might need to be dealt
with sooner.

In the standard case (concurrency = 1) it's not a problem except for the
duplicates, which hopefully isn't a common case (the UMAs say it's mostly
omnibox doing this).

In the multi prerender case though; these gaps are more serious.

[mmenke, 2012/12/18 17:15:15]

Speaking of which, you did switch the omnibox to cancelling rather than relying
that behavior in an earlier CL, didn't you?

[gavinp, 2012/12/18 20:15:12]

Done.

+  DCHECK_GE(manager_->config().max_concurrency, running_count);
+
+
+  // Scan the list again, starting prerenders as our counts allow.
+  std::list<Prerender>::iterator next = prerenders_.begin();
+  while (next != prerenders_.end()) {
+    std::list<Prerender>::iterator i = next;
+    ++next;
+
+    if (running_count >= manager_->config().max_concurrency ||
+        running_count >= prerenders_.size()) {
+      return;
+    }
+
+    if (i->handle)
+      continue;
+
+    std::pair<int, int> launcher_and_render_view_route(
+        i->launcher_child_id, i->render_view_route_id);
+
+    if (manager_->config().max_concurrency_per_launcher <=
+        running_launcher_and_render_view_route_set.count(
+            launcher_and_render_view_route)) {
+      continue;
+    }
+
+    PrerenderHandle* handle = manager_->AddPrerenderFromLinkRelPrerender(
+        i->launcher_child_id, i->render_view_route_id,
+        i->url, i->referrer, i->size);
+    if (!handle) {
+      prerenders_.erase(i);
+      continue;
+    }
+    i->handle = handle;
+    ++running_count;
+    handle->SetObserver(this);
+    if (handle->IsPrerendering())
+      OnPrerenderStart(handle);
+
+    running_launcher_and_render_view_route_set.insert(
+        launcher_and_render_view_route);
+  }
 }
 
-void PrerenderLinkManager::RemovePrerender(
-    const IdPairToPrerenderHandleMap::iterator& id_to_handle_iter) {
-  PrerenderHandle* prerender_handle = id_to_handle_iter->second;
-  delete prerender_handle;
-  ids_to_handle_map_.erase(id_to_handle_iter);
+PrerenderLinkManager::Prerender*
+PrerenderLinkManager::FindByLauncherChildIdAndPrerenderId(int launcher_child_id,
+                                                          int prerender_id) {
+  for (std::list<Prerender>::iterator i = prerenders_.begin();
+       i != prerenders_.end(); ++i) {
+    if (launcher_child_id == i->launcher_child_id &&
+        prerender_id == i->prerender_id) {
+      return &(*i);
+    }
+  }
+  return NULL;
 }
 
-PrerenderLinkManager::IdPairToPrerenderHandleMap::iterator
-PrerenderLinkManager::FindPrerenderHandle(
-    PrerenderHandle* prerender_handle) {
-  for (IdPairToPrerenderHandleMap::iterator it = ids_to_handle_map_.begin();
-       it != ids_to_handle_map_.end(); ++it) {
-    if (it->second == prerender_handle)
-      return it;
+PrerenderLinkManager::Prerender*
+PrerenderLinkManager::FindByPrerenderHandle(PrerenderHandle* prerender_handle) {
+  for (std::list<Prerender>::iterator i = prerenders_.begin();
+       i != prerenders_.end(); ++i) {
+    if (prerender_handle == i->handle)
+      return &(*i);
   }
-  return ids_to_handle_map_.end();
+  return NULL;
+}
+
+void PrerenderLinkManager::RemovePrerender(Prerender* prerender) {
+  for (std::list<Prerender>::iterator i = prerenders_.begin();
+       i != prerenders_.end(); ++i) {
+    if (&(*i) == prerender) {
+      scoped_ptr<PrerenderHandle> own_handle(i->handle);
+      i->handle = NULL;

[mmenke, 2012/12/17 20:02:23]

Don't think this does anything useful.

[gavinp, 2012/12/18 00:44:14]

There's a DCHECK() in PrerenderListManager::~Prerender which asserts that handle
is NULL. By zeroing it out before erasing, we make a bit more sure that there
isn't some destruction of a PrerenderData that is leaking one of these.

[mmenke, 2012/12/18 17:15:15]

Sounds reasonable.

+      prerenders_.erase(i);
+      return;
+    }
+  }
+  NOTREACHED();
+}
+
+void PrerenderLinkManager::Shutdown() {
+  is_shutdown_ = true;
 }
 
 // In practice, this is always called from either
@@ -167,39 +257,34 @@ PrerenderLinkManager::FindPrerenderHandle(
 // prerender case, from PrerenderHandle::AdoptPrerenderDataFrom.
 void PrerenderLinkManager::OnPrerenderStart(
     PrerenderHandle* prerender_handle) {
-  IdPairToPrerenderHandleMap::iterator it =
-      FindPrerenderHandle(prerender_handle);
-  DCHECK(it != ids_to_handle_map_.end());
-  const int child_id = it->first.first;
-  const int prerender_id = it->first.second;
-
-  Send(child_id, new PrerenderMsg_OnPrerenderStart(prerender_id));
+  Prerender* prerender = FindByPrerenderHandle(prerender_handle);
+  if (!prerender)
+    return;
+  Send(prerender->launcher_child_id,
+       new PrerenderMsg_OnPrerenderStart(prerender->prerender_id));
 }
 
 void PrerenderLinkManager::OnPrerenderAddAlias(
     PrerenderHandle* prerender_handle,
     const GURL& alias_url) {
-  IdPairToPrerenderHandleMap::iterator it =
-      FindPrerenderHandle(prerender_handle);
-  if (it == ids_to_handle_map_.end())
+  Prerender* prerender = FindByPrerenderHandle(prerender_handle);
+  if (!prerender)
     return;
-  const int child_id = it->first.first;
-  const int prerender_id = it->first.second;
-
-  Send(child_id, new PrerenderMsg_OnPrerenderAddAlias(prerender_id, alias_url));
+  Send(prerender->launcher_child_id,
+       new PrerenderMsg_OnPrerenderAddAlias(prerender->prerender_id,
+                                            alias_url));
 }
 
 void PrerenderLinkManager::OnPrerenderStop(
     PrerenderHandle* prerender_handle) {
-  IdPairToPrerenderHandleMap::iterator it =
-      FindPrerenderHandle(prerender_handle);
-  if (it == ids_to_handle_map_.end())
+  Prerender* prerender = FindByPrerenderHandle(prerender_handle);
+  if (!prerender)
     return;
-  const int child_id = it->first.first;
-  const int prerender_id = it->first.second;
 
-  Send(child_id, new PrerenderMsg_OnPrerenderStop(prerender_id));
-  RemovePrerender(it);
+  Send(prerender->launcher_child_id,
+       new PrerenderMsg_OnPrerenderStop(prerender->prerender_id));
+  RemovePrerender(prerender);
+  StartPrerenders();
 }
 
 }  // namespace prerender