Make a separate histogram for MatchComplete Final Status'es and

chrome/browser/prerender/prerender_manager.cc

 // Copyright (c) 2012 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 "chrome/browser/prerender/prerender_manager.h"
 
 #include <string>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ skipping 66 matching lines @@
   "POST",
   "TRACE",
 };
 
 // Length of prerender history, for display in chrome://net-internals
 const int kHistoryLength = 100;
 
 // Indicates whether a Prerender has been cancelled such that we need
 // a dummy replacement for the purpose of recording the correct PPLT for
 // the Match Complete case.
+// Traditionally, "Match" means that a prerendered page was actually visited &
+// the prerender was used.  Our goal is to have "Match" cases line up in the
+// control group & the experiment group, so that we can make meaningful
+// comparisons of improvements.  However, in the control group, since we don't
+// actually perform prerenders, many of the cancellation reasons cannot be
+// detected.  Therefore, in the Prerender group, when we cancel for one of these
+// reasons, we keep track of a dummy Prerender representing what we would
+// have in the control group.  If that dummy prerender in the prerender group
+// would then be swapped in (but isn't actually b/c it's a dummy), we record
+// this as a MatchComplete.  This allows us to compare MatchComplete's
+// across Prerender & Control group which ideally should be lining up.
+// This ensures that there is no bias in terms of the page load times
+// of the pages forming the difference between the two sets.
 
 bool NeedMatchCompleteDummyForFinalStatus(FinalStatus final_status) {

[cbentzel, 2012/01/24 14:55:46]

This could return an enum which has three values

MATCH_COMPLETE_DUMMY_NO
MATCH_COMPLETE_DUMMY_YES
MATCH_COMPLETE_DUMMY_IMMEDIATE

This would remove the need for DestroyAndMarkMatchCompleteAsUsed.

Inside MoveEntryToPendingDelete, you wouldn't create the dummy in the IMMEDIATE
case, but would record the histogram.

The reason I prefer this is that you only need to determine the policy in one
location. In your code, you have to get it right in the
NeedMatchCompleteDummyForFinalStatus, as well as at each caller in
MaybeUsePrerenderedPage.

[cbentzel, 2012/01/24 14:55:46]

<random thought>
I wonder if we should add a table for this on FinalStatus with a compile-time
assert. That would force people to think through whether a Match Complete Dummy
is needed every time a new FinalStatus is introduced. Otherwise, it appears too
easy to forget. 
</random thought>

It's OK to do this in a separate CL. 

[tburkard, 2012/01/24 19:15:54]

Ok will think about this for a new CL.
On 2012/01/24 14:55:46, cbentzel wrote:

[tburkard, 2012/01/24 19:15:54]

Yes and no.

Here is why I am hesitant to add something like this:

Whether a status requires special treatment or not doesn't just depend on the
status -- but also on the order in which checks are done in
MaybeUsePrerenderedPage.

Eg IMMEDIATE is only applicable for things that come after the
has_prerendering_started check.  It's conceivable that the order of these checks
gets changed.  In that case, I think having explicit code right at the place
where the change happens brings people's attention that this case may need
adjustment.  Just having a table at a different place does not make that as
obvious.

Furthermore, another issue is that the same status could in theory originate at
multiple places, requiring different handling based on the place where it
originates from.  This would not be possible with what you are proposing.

Still an interesting thought, but I think we shoudl give it more thought and do
it in a separate CL for now.

Thanks.

On 2012/01/24 14:55:46, cbentzel wrote:

   return final_status != FINAL_STATUS_USED &&
       final_status != FINAL_STATUS_TIMED_OUT &&
       final_status != FINAL_STATUS_EVICTED &&
       final_status != FINAL_STATUS_MANAGER_SHUTDOWN &&
       final_status != FINAL_STATUS_APP_TERMINATING &&
       final_status != FINAL_STATUS_RENDERER_CRASHED &&
       final_status != FINAL_STATUS_WINDOW_OPENER &&
       final_status != FINAL_STATUS_FRAGMENT_MISMATCH &&
       final_status != FINAL_STATUS_CACHE_OR_HISTORY_CLEARED &&
       final_status != FINAL_STATUS_CANCELLED &&
+      final_status != FINAL_STATUS_SESSION_STORAGE_NAMESPACE_MISMATCH &&
+      final_status != FINAL_STATUS_DEVTOOLS_ATTACHED &&
+      final_status != FINAL_STATUS_CROSS_SITE_NAVIGATION_PENDING &&
       final_status != FINAL_STATUS_MATCH_COMPLETE_DUMMY;
 }
 
 }  // namespace
 
 class PrerenderManager::OnCloseTabContentsDeleter
     : public content::WebContentsDelegate,
       public base::SupportsWeakPtr<
           PrerenderManager::OnCloseTabContentsDeleter> {
  public:
@@ skipping 321 matching lines @@
   if (!prerender_contents || !prerender_contents->Init())
     return false;
 
   histograms_->RecordPrerenderStarted(origin);
 
   // TODO(cbentzel): Move invalid checks here instead of PrerenderContents?
   PrerenderContentsData data(prerender_contents, GetCurrentTime());
 
   prerender_list_.push_back(data);
 
-  if (IsControlGroup()) {
-    data.contents_->set_final_status(FINAL_STATUS_CONTROL_GROUP);

[cbentzel, 2012/01/24 14:55:46]

You should add an OBSOLETE comment for FINAL_STATUS_CONTROL_GROUP in
prerender_final_status.h

[tburkard, 2012/01/24 19:15:54]

Done.

-  } else {
+  if (!IsControlGroup()) {
     last_prerender_start_time_ = GetCurrentTimeTicks();
     data.contents_->StartPrerendering(source_render_view_host,
                                       session_storage_namespace);
   }
   while (prerender_list_.size() > config_.max_elements) {
     data = prerender_list_.front();
     prerender_list_.pop_front();
     data.contents_->Destroy(FINAL_STATUS_EVICTED);
   }
   StartSchedulingPeriodicCleanups();
@@ skipping 74 matching lines @@
     }
   }
   // Entry not found.
   return NULL;
 }
 
 PrerenderContents* PrerenderManager::GetEntry(const GURL& url) {
   return GetEntryButNotSpecifiedWC(url, NULL);
 }
 
+void PrerenderManager::DestroyAndMarkMatchCompleteAsUsed(
+    PrerenderContents* prerender_contents,
+    FinalStatus final_status) {
+  prerender_contents->set_match_complete_status(
+      PrerenderContents::MATCH_COMPLETE_REPLACED);
+  histograms_->RecordFinalStatus(prerender_contents->origin(),
+                                 prerender_contents->experiment_id(),
+                                 PrerenderContents::MATCH_COMPLETE_REPLACEMENT,
+                                 FINAL_STATUS_USED);
+  prerender_contents->Destroy(final_status);
+}
+
 bool PrerenderManager::MaybeUsePrerenderedPage(WebContents* web_contents,
                                                const GURL& url,
                                                const GURL& opener_url) {
   DCHECK(CalledOnValidThread());
   RecordNavigation(url);
 
   scoped_ptr<PrerenderContents> prerender_contents(
       GetEntryButNotSpecifiedWC(url, web_contents));
   if (prerender_contents.get() == NULL)
     return false;
@@ skipping 15 matching lines @@
   if (url_matches && url.ref() != matching_url.ref()) {
     prerender_contents.release()->Destroy(FINAL_STATUS_FRAGMENT_MISMATCH);
     return false;
   }
 
   // If we are just in the control group (which can be detected by noticing
   // that prerendering hasn't even started yet), record that |web_contents| now
   // would be showing a prerendered contents, but otherwise, don't do anything.
   if (!prerender_contents->prerendering_has_started()) {
     MarkWebContentsAsWouldBePrerendered(web_contents);
+    prerender_contents.release()->Destroy(FINAL_STATUS_USED);
     return false;
   }
 
   // Don't use prerendered pages if debugger is attached to the tab.
   // See http://crbug.com/98541
   if (content::DevToolsAgentHostRegistry::IsDebuggerAttached(web_contents)) {
-    prerender_contents.release()->Destroy(FINAL_STATUS_DEVTOOLS_ATTACHED);
+    DestroyAndMarkMatchCompleteAsUsed(prerender_contents.release(),
+                                      FINAL_STATUS_DEVTOOLS_ATTACHED);
     return false;
   }
 
   // If the prerendered page is in the middle of a cross-site navigation,
   // don't swap it in because there isn't a good way to merge histories.
   if (prerender_contents->IsCrossSiteNavigationPending()) {
-    prerender_contents.release()->Destroy(
+    DestroyAndMarkMatchCompleteAsUsed(
+        prerender_contents.release(),
         FINAL_STATUS_CROSS_SITE_NAVIGATION_PENDING);
     return false;
   }
 
   // If the session storage namespaces don't match, cancel the prerender.
   RenderViewHost* old_render_view_host = web_contents->GetRenderViewHost();
   RenderViewHost* new_render_view_host =
       prerender_contents->prerender_contents()->web_contents()->
           GetRenderViewHost();
   DCHECK(old_render_view_host);
   DCHECK(new_render_view_host);
   if (old_render_view_host->session_storage_namespace() !=
       new_render_view_host->session_storage_namespace()) {
-    prerender_contents.release()->Destroy(
+    DestroyAndMarkMatchCompleteAsUsed(
+        prerender_contents.release(),
         FINAL_STATUS_SESSION_STORAGE_NAMESPACE_MISMATCH);
     return false;
   }
 
   // If we don't want to use prerenders at all, we are done.
   // For bookkeeping purposes, we need to mark this TabContents to
   // reflect that it would have been prerendered.
   if (GetMode() == PRERENDER_MODE_EXPERIMENT_NO_USE_GROUP) {
     MarkWebContentsAsWouldBePrerendered(web_contents);
     prerender_contents.release()->Destroy(FINAL_STATUS_NO_USE_GROUP);

[cbentzel, 2012/01/24 14:55:46]

IMPORTANT: This should just be FINAL_STATUS_USED here, similar to how
FINAL_STATUS_CONTROL_GROUP is not used anymore.

[tburkard, 2012/01/24 19:15:54]

Done.

     return false;
   }
 
   int child_id, route_id;
   CHECK(prerender_contents->GetChildId(&child_id));
   CHECK(prerender_contents->GetRouteId(&route_id));
 
   // Try to set the prerendered page as used, so any subsequent attempts to
   // cancel on other threads will fail.  If this fails because the prerender
   // was already cancelled, possibly on another thread, fail.
@@ skipping 78 matching lines @@
        it != prerender_list_.end();
        ++it) {
     if (it->contents_ == entry) {
       bool swapped_in_dummy_replacement = false;
 
       // If this PrerenderContents is being deleted due to a cancellation,
       // we need to create a dummy replacement for PPLT accounting purposes
       // for the Match Complete group.
       // This is the case if the cancellation is for any reason that would not
       // occur in the control group case.
-      if (NeedMatchCompleteDummyForFinalStatus(final_status)) {
+      if (entry->match_complete_status() ==
+          PrerenderContents::MATCH_COMPLETE_DEFAULT &&
+          NeedMatchCompleteDummyForFinalStatus(final_status)) {
         // TODO(tburkard): I'd like to DCHECK that we are actually prerendering.
         // However, what if new conditions are added and
         // NeedMatchCompleteDummyForFinalStatus, is not being updated.  Not sure
         // what's the best thing to do here.  For now, I will just check whether
         // we are actually prerendering.
         if (ActuallyPrerendering()) {
+          entry->set_match_complete_status(
+              PrerenderContents::MATCH_COMPLETE_REPLACED);
           PrerenderContents* dummy_replacement_prerender_contents =
               CreatePrerenderContents(
                   entry->prerender_url(),
                   entry->referrer(),
                   entry->origin(),
                   entry->experiment_id());
           if (dummy_replacement_prerender_contents &&
               dummy_replacement_prerender_contents->Init()) {
             dummy_replacement_prerender_contents->
                 AddAliasURLsFromOtherPrerenderContents(entry);
+            dummy_replacement_prerender_contents->set_match_complete_status(
+                PrerenderContents::MATCH_COMPLETE_REPLACEMENT);
             it->contents_ = dummy_replacement_prerender_contents;
-            it->contents_->set_final_status(FINAL_STATUS_MATCH_COMPLETE_DUMMY);
             swapped_in_dummy_replacement = true;
           }
         }
       }
       if (!swapped_in_dummy_replacement)
         prerender_list_.erase(it);
       break;
     }
   }
   AddToHistory(entry);
@@ skipping 355 matching lines @@
 void PrerenderManager::DestroyAllContents(FinalStatus final_status) {
   DeleteOldTabContents();
   while (!prerender_list_.empty()) {
     PrerenderContentsData data = prerender_list_.front();
     prerender_list_.pop_front();
     data.contents_->Destroy(final_status);
   }
   DeletePendingDeleteEntries();
 }
 
+void PrerenderManager::RecordFinalStatusWithMatchCompleteStatus(
+    Origin origin,
+    uint8 experiment_id,
+    PrerenderContents::MatchCompleteStatus mc_status,
+    FinalStatus final_status) const {
+  histograms_->RecordFinalStatus(origin,
+                                 experiment_id,
+                                 mc_status,
+                                 final_status);
+}
+
 void PrerenderManager::RecordFinalStatus(Origin origin,
                                          uint8 experiment_id,
                                          FinalStatus final_status) const {
-  histograms_->RecordFinalStatus(origin, experiment_id, final_status);
+  RecordFinalStatusWithMatchCompleteStatus(
+      origin, experiment_id,
+      PrerenderContents::MATCH_COMPLETE_DEFAULT,
+      final_status);
 }
 
+
 PrerenderManager* FindPrerenderManagerUsingRenderProcessId(
     int render_process_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   content::RenderProcessHost* render_process_host =
       content::RenderProcessHost::FromID(render_process_id);
   // Each render process is guaranteed to only hold RenderViews owned by the
   // same BrowserContext. This is enforced by
   // RenderProcessHost::GetExistingProcessHost.
   if (!render_process_host || !render_process_host->GetBrowserContext())
     return NULL;
   Profile* profile = Profile::FromBrowserContext(
       render_process_host->GetBrowserContext());
   if (!profile)
     return NULL;
   return PrerenderManagerFactory::GetInstance()->GetForProfile(profile);
 }
 
 }  // namespace prerender