PlzNavigate: Add metrics to understand bf navigations PLT regression

content/browser/loader/resource_dispatcher_host_impl.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.
 
 // See http://dev.chromium.org/developers/design-documents/multi-process-resource-loading
 
 #include "content/browser/loader/resource_dispatcher_host_impl.h"
 
 #include <stddef.h>
 
@@ skipping 254 matching lines @@
     return;
 
   if (result) {
     ResourceHostMsg_SyncLoad::WriteReplyParams(sync_result.get(), *result);
   } else {
     sync_result->set_reply_error();
   }
   filter->Send(sync_result.release());
 }
 
+// Used to log the cache flags for back-forward navigation requests.

[Ilya Sherman, 2017/06/13 21:38:00]

Please document that this is used to back an UMA histogram, and should therefore
be treated as append-only.

[clamy, 2017/06/14 13:26:50]

Done.

+// TODO(clamy): Remove this once we know the reason behind PlzNavigate's
+// regression on PLT for back forward navigations.
+enum HistogramCacheFlag {
+  HISTOGRAM_VALIDATE_CACHE,
+  HISTOGRAM_BYPASS_CACHE,
+  HISTOGRAM_SKIP_CACHE_VALIDATION,
+  HISTOGRAM_ONLY_FROM_CACHE,
+  HISTOGRAM_DISABLE_CACHE,
+  HISTOGRAM_CACHE_FLAG_MAX = HISTOGRAM_DISABLE_CACHE,
+};
+
+void LogBackForwardNavigationFlagsHistogram(int load_flags) {
+  if (load_flags & net::LOAD_VALIDATE_CACHE) {
+    UMA_HISTOGRAM_ENUMERATION("Navigation.BackForward.CacheFlags",
+                              HISTOGRAM_VALIDATE_CACHE,
+                              HISTOGRAM_CACHE_FLAG_MAX);

[Ilya Sherman, 2017/06/13 21:38:00]

nit: Please create a small wrapper function, that could be called like
RecordCacheFlags(HISTOGRAM_VALIDATE_CACHE).  The UMA_HISTOGRAM_* macros expand
to a fair bit of code, so it's nice to help the compiler not duplicate that (and
also help avoid typos sneaking in to just some of the code paths).

[clamy, 2017/06/14 13:26:50]

Done.

+  }
+  if (load_flags & net::LOAD_BYPASS_CACHE) {
+    UMA_HISTOGRAM_ENUMERATION("Navigation.BackForward.CacheFlags",
+                              HISTOGRAM_BYPASS_CACHE, HISTOGRAM_CACHE_FLAG_MAX);
+  }
+  if (load_flags & net::LOAD_SKIP_CACHE_VALIDATION) {
+    UMA_HISTOGRAM_ENUMERATION("Navigation.BackForward.CacheFlags",
+                              HISTOGRAM_SKIP_CACHE_VALIDATION,
+                              HISTOGRAM_CACHE_FLAG_MAX);
+  }
+  if (load_flags & net::LOAD_ONLY_FROM_CACHE) {
+    UMA_HISTOGRAM_ENUMERATION("Navigation.BackForward.CacheFlags",
+                              HISTOGRAM_ONLY_FROM_CACHE,
+                              HISTOGRAM_CACHE_FLAG_MAX);
+  }
+  if (load_flags & net::LOAD_VALIDATE_CACHE) {
+    UMA_HISTOGRAM_ENUMERATION("Navigation.BackForward.CacheFlags",
+                              HISTOGRAM_VALIDATE_CACHE,
+                              HISTOGRAM_CACHE_FLAG_MAX);
+  }
+}
+
 }  // namespace
 
 ResourceDispatcherHostImpl::LoadInfo::LoadInfo() {}
 ResourceDispatcherHostImpl::LoadInfo::LoadInfo(const LoadInfo& other) = default;
 ResourceDispatcherHostImpl::LoadInfo::~LoadInfo() {}
 
 ResourceDispatcherHostImpl::HeaderInterceptorInfo::HeaderInterceptorInfo() {}
 
 ResourceDispatcherHostImpl::HeaderInterceptorInfo::~HeaderInterceptorInfo() {}
 
@@ skipping 1908 matching lines @@
   return kAvgBytesPerOutstandingRequest + strings_cost;
 }
 
 void ResourceDispatcherHostImpl::BeginRequestInternal(
     std::unique_ptr<net::URLRequest> request,
     std::unique_ptr<ResourceHandler> handler) {
   DCHECK(!request->is_pending());
   ResourceRequestInfoImpl* info =
       ResourceRequestInfoImpl::ForRequest(request.get());
 
+  // Log metrics for back-forward navigations.
+  // TODO(clamy): Remove this once we understand the reason behind the
+  // back-forward PLT regression with PlzNavigate
+  if ((info->GetPageTransition() & ui::PAGE_TRANSITION_FORWARD_BACK) &&
+      IsResourceTypeFrame(info->GetResourceType())) {

[nasko, 2017/06/13 23:50:08]

Since we care only about main resources, shouldn't this code be added only to
ResourceDispatcherHostImpl::BeginNavigationRequest? Or is the goal to capture
both PlzNavigate and regular navigation cases?

[clamy, 2017/06/14 13:26:50]

Yes the goal is to compare between PlzNavigate and non PlzNavigate for all
histograms.

+    LogBackForwardNavigationFlagsHistogram(request->load_flags());
+  }
+
   if ((TimeTicks::Now() - last_user_gesture_time_) <
       TimeDelta::FromMilliseconds(kUserGestureWindowMs)) {
     request->SetLoadFlags(request->load_flags() | net::LOAD_MAYBE_USER_GESTURE);
   }
 
   // Add the memory estimate that starting this request will consume.
   info->set_memory_cost(CalculateApproximateMemoryCost(request.get()));
 
   // If enqueing/starting this request will exceed our per-process memory
   // bound, abort it right away.
@@ skipping 443 matching lines @@
                                  &throttles);
     if (!throttles.empty()) {
       handler.reset(new ThrottlingResourceHandler(std::move(handler), request,
                                                   std::move(throttles)));
     }
   }
   return handler;
 }
 
 }  // namespace content