Breakdown ClientLoadedTime histograms into buckets based on number of clients. Fix RequestTimeThrot…

content/browser/loader/resource_scheduler.cc

Index: content/browser/loader/resource_scheduler.cc
diff --git a/content/browser/loader/resource_scheduler.cc b/content/browser/loader/resource_scheduler.cc
index 64d7e654e61a95d47241026038eefe681ebdbc27..68b2927ac689e563eefb884282168cbb26ea6e7f 100644
--- a/content/browser/loader/resource_scheduler.cc
+++ b/content/browser/loader/resource_scheduler.cc
@@ -27,13 +27,21 @@ namespace content {
 
 namespace {
 
+// Post ResourceScheduler histograms of the following forms:
+// If histogram_suffix is NULL or the empty string:

[mmenke, 2015/01/06 19:30:03]

nit:  |histogram_suffix|

[aiolos (Not reviewing), 2015/01/07 22:28:15]

Done.

+//   ResourceScheduler.base_name.histogram_name
+// Else:
+//   ResourceScheduler.base_name.histogram_name.histogram_suffix
 void PostHistogram(const char* base_name,
-                   const char* suffix,
+                   const char* histogram_name,
+                   const char* histogram_suffix,

[Ilya Sherman, 2015/01/06 02:19:14]

I meant, why not pass the strings as "const std::string&" (by const ref) rather
than "std::string" (by copy)?  I do think that std::string, as you had before,
or StringPiece is better than char*.  But, I don't feel strongly about this.

[aiolos (Not reviewing), 2015/01/07 22:28:15]

Matt: you wanted const char* for the suffix in other parts of the code. Do you
have a preference here?

[mmenke, 2015/01/07 22:32:42]

I generally think the fewer strings we make the better, and making chars into
strings when we call this method, only to append them together again seems like
more work for every request, with no real advantage...

So using const char* seems to make more sense, but it's not something I'm going
to fight over.  :)

                    base::TimeDelta time) {
-  std::string histogram_name =
-      base::StringPrintf("ResourceScheduler.%s.%s", base_name, suffix);
+  std::string histogram =
+      base::StringPrintf("ResourceScheduler.%s.%s", base_name, histogram_name);
+  if (histogram_suffix && histogram_suffix[0] != '\0')
+    histogram = histogram + "." + histogram_suffix;
   base::HistogramBase* histogram_counter = base::Histogram::FactoryTimeGet(
-      histogram_name,
+      histogram,
       base::TimeDelta::FromMilliseconds(1),
       base::TimeDelta::FromMinutes(5),
       50,
@@ -41,6 +49,20 @@ void PostHistogram(const char* base_name,
   histogram_counter->AddTime(time);
 }
 
+// For use with PostHistogram to specify the correct string for histogram
+// suffixes based on number of Clients.
+const char* GetNumClientsString(size_t num_clients) {
+  if (num_clients == 1)
+    return "1Client";
+  else if (num_clients <= 5)
+    return "Max5Clients";
+  else if (num_clients <= 15)
+    return "Max15Clients";
+  else if (num_clients <= 30)
+    return "Max30Clients";
+  return "Over30Clients";
+}
+
 }  // namespace
 
 static const size_t kCoalescedTimerPeriod = 5000;
@@ -182,9 +204,9 @@ class ResourceScheduler::ScheduledResourceRequest
       controller()->Resume();
       time_was_deferred = time - time_deferred_;
     }
-    PostHistogram("RequestTimeDeferred", client_state, time_was_deferred);
-    PostHistogram(
-        "RequestTimeThrottled", client_state, time - request_->creation_time());
+    PostHistogram("RequestTimeDeferred", client_state, NULL, time_was_deferred);
+    PostHistogram("RequestTimeThrottled", client_state, NULL,
+                  time - request_->creation_time());
     // TODO(aiolos): Remove one of the above histograms after gaining an
     // understanding of the difference between them and which one is more
     // interesting.
@@ -368,15 +390,22 @@ class ResourceScheduler::Client {
       return;
     }
     base::TimeTicks cur_time = base::TimeTicks::Now();
+    const char* num_clients = GetNumClientsString(scheduler_->GetNumClients());
     const char* client_catagory = "Other";
     if (last_active_switch_time_.is_null()) {
       client_catagory = is_active() ? "Active" : "Background";
     } else if (is_active()) {
-      PostHistogram("ClientLoadedTime", "Other.SwitchedToActive",
-                    cur_time - last_active_switch_time_);
+      base::TimeDelta time_since_active = cur_time - last_active_switch_time_;
+      PostHistogram("ClientLoadedTime", "Other.SwitchedToActive", NULL,
+                    time_since_active);
+      PostHistogram("ClientLoadedTime", "Other.SwitchedToActive", num_clients,
+                    time_since_active);
     }
-    PostHistogram("ClientLoadedTime", client_catagory,
-                  cur_time - load_started_time_);
+    base::TimeDelta time_since_load_started = cur_time - load_started_time_;
+    PostHistogram("ClientLoadedTime", client_catagory, NULL,
+                  time_since_load_started);
+    PostHistogram("ClientLoadedTime", client_catagory, num_clients,
+                  time_since_load_started);
     // TODO(aiolos): The above histograms will not take main resource load time
     // into account with PlzNavigate into account. The ResourceScheduler also
     // will load the main resources without a clients with the current logic.
@@ -878,8 +907,6 @@ void ResourceScheduler::OnClientCreated(int child_id,
   Client* client = new Client(this, is_visible, is_audible);
   client_map_[client_id] = client;
 
-  // TODO(aiolos): set Client visibility/audibility when signals are added
-  // this will UNTHROTTLE Clients as needed
   client->UpdateThrottleState();
 }
 
@@ -998,6 +1025,10 @@ void ResourceScheduler::DecrementActiveClientsLoading() {
   }
 }
 
+size_t ResourceScheduler::GetNumClients() const {
+  return client_map_.size();
+}
+
 void ResourceScheduler::IncrementActiveClientsLoading() {
   ++active_clients_loading_;
   DCHECK_EQ(active_clients_loading_, CountActiveClientsLoading());