Add page-level aggregation of SubresourceFilter time metrics.

components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc

Index: components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
diff --git a/components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc b/components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
index dbb8f3a5918a9cbba2421ab24925f5035f118b7d..c38d4078968239248fdf01c4215d38ed2f504c3b 100644
--- a/components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
+++ b/components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
@@ -6,11 +6,13 @@
 
 #include "base/metrics/histogram_macros.h"
 #include "base/rand_util.h"
+#include "base/time/time.h"
 #include "components/subresource_filter/content/browser/content_subresource_filter_driver.h"
 #include "components/subresource_filter/content/common/subresource_filter_messages.h"
 #include "components/subresource_filter/core/browser/subresource_filter_client.h"
 #include "components/subresource_filter/core/browser/subresource_filter_features.h"
 #include "components/subresource_filter/core/common/activation_list.h"
+#include "components/subresource_filter/core/common/time_measurements.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
@@ -28,8 +30,15 @@ std::string DistillURLToHostAndPath(const GURL& url) {
   return url.host() + url.path();
 }
 
-bool ShouldMeasurePerformance(double rate) {
-  return base::RandDouble() < rate;
+// Returns true with a probability of GetPerformanceMeasurementRate() if
+// ThreadTicks is supported, otherwise returns false. Caches
+// GetPerformanceMeasurementRate() in a static local variavle, therefore is not
+// thread-safe.
+bool ShouldMeasurePerformanceForThisPage() {

[engedy, 2016/12/19 10:18:42]

nit: s/ThisPage/PageLoad/

[pkalinnikov, 2016/12/21 15:28:46]

Done.

+  if (!base::ThreadTicks::IsSupported())
+    return false;
+  static const double rate = GetPerformanceMeasurementRate();

[pkalinnikov, 2016/12/21 15:28:46]

static local turned out to be a bad idea, because in tests variation params are
changed dynamically. I leave a TODO to address this in a follow-up CL.

[engedy, 2016/12/21 19:10:27]

Acknowledged. Recommended sounds good too.

+  return rate == 1 || (rate > 0 && base::RandDouble() < rate);

[engedy, 2016/12/19 10:18:42]

Isn't this equivalent to just `base::RandDouble() < rate`?

[pkalinnikov, 2016/12/21 15:28:46]

Right, but I did this in order to optimize for 0 and 1 cases (by not calling
RandDouble), because those are the most anticipated values of |rate|.

[engedy, 2016/12/21 19:10:27]

Ack, could you please add a one-liner comment to point this out?

 }
 
 }  // namespace
@@ -83,6 +92,13 @@ void ContentSubresourceFilterDriverFactory::OnFirstSubresourceLoadDisallowed() {
                                         ActivationState::ENABLED);
 }
 
+void ContentSubresourceFilterDriverFactory::OnEvaluationDurationAggregated(
+    base::TimeDelta evaluation_total_wall_duration,
+    base::TimeDelta evaluation_total_cpu_duration) {
+  evaluation_total_wall_duration_ += evaluation_total_wall_duration;
+  evaluation_total_cpu_duration_ += evaluation_total_cpu_duration;
+}
+
 bool ContentSubresourceFilterDriverFactory::IsWhitelisted(
     const GURL& url) const {
   return whitelisted_hosts_.find(url.host()) != whitelisted_hosts_.end();
@@ -169,6 +185,8 @@ void ContentSubresourceFilterDriverFactory::DidStartNavigation(
     client_->ToggleNotificationVisibility(false);
     activation_state_ = ActivationState::DISABLED;
     measure_performance_ = false;
+    evaluation_total_wall_duration_ = base::TimeDelta();
+    evaluation_total_cpu_duration_ = base::TimeDelta();
   }
 }
 
@@ -198,6 +216,25 @@ void ContentSubresourceFilterDriverFactory::ReadyToCommitNavigation(
   ReadyToCommitNavigationInternal(render_frame_host, url);
 }
 
+void ContentSubresourceFilterDriverFactory::DidFinishLoad(
+    content::RenderFrameHost* render_frame_host,
+    const GURL& validated_url) {
+  if (render_frame_host->GetParent())
+    return;
+
+  if (activation_state_ != ActivationState::DISABLED && measure_performance_) {

[pkalinnikov, 2016/12/16 16:54:23]

Probably just need to check |measure_performance_|, but set it to true below
only if |activation_state_| != DISABLED. WDYT?

[engedy, 2016/12/19 10:18:42]

Sounds good. Then you can add this as a DCHECK to document the assumption.

[pkalinnikov, 2016/12/21 15:28:46]

Done.

+    UMA_HISTOGRAM_CUSTOM_MICRO_TIMES(
+        "SubresourceFilter.PageLoad.SubresourceEvaluation.TotalWallDuration",
+        evaluation_total_wall_duration_, base::TimeDelta::FromMicroseconds(1),
+        base::TimeDelta::FromSeconds(10), 50);
+
+    UMA_HISTOGRAM_CUSTOM_MICRO_TIMES(
+        "SubresourceFilter.PageLoad.SubresourceEvaluation.TotalCPUDuration",
+        evaluation_total_cpu_duration_, base::TimeDelta::FromMicroseconds(1),
+        base::TimeDelta::FromSeconds(10), 50);
+  }
+}
+
 bool ContentSubresourceFilterDriverFactory::OnMessageReceived(
     const IPC::Message& message,
     content::RenderFrameHost* render_frame_host) {
@@ -205,6 +242,8 @@ bool ContentSubresourceFilterDriverFactory::OnMessageReceived(
   IPC_BEGIN_MESSAGE_MAP(ContentSubresourceFilterDriverFactory, message)
     IPC_MESSAGE_HANDLER(SubresourceFilterHostMsg_DidDisallowFirstSubresource,
                         OnFirstSubresourceLoadDisallowed)
+    IPC_MESSAGE_HANDLER(SubresourceFilterHostMsg_DidAggregateEvaluationDuration,
+                        OnEvaluationDurationAggregated)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
@@ -217,12 +256,13 @@ void ContentSubresourceFilterDriverFactory::ReadyToCommitNavigationInternal(
     RecordRedirectChainMatchPattern();
     if (ShouldActivateForMainFrameURL(url)) {
       activation_state_ = GetMaximumActivationState();
-      measure_performance_ =
-          ShouldMeasurePerformance(GetPerformanceMeasurementRate());
+      measure_performance_ = ShouldMeasurePerformanceForThisPage();
       ActivateForFrameHostIfNeeded(render_frame_host, url);
     } else {
       activation_state_ = ActivationState::DISABLED;
       measure_performance_ = false;
+      evaluation_total_wall_duration_ = base::TimeDelta();
+      evaluation_total_cpu_duration_ = base::TimeDelta();
     }
   } else {
     ActivateForFrameHostIfNeeded(render_frame_host, url);