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..f4264d43543a8053b7924b5d44d8cd4afa4f6389 100644
--- a/content/browser/loader/resource_scheduler.cc
+++ b/content/browser/loader/resource_scheduler.cc
@@ -27,11 +27,10 @@ namespace content {
 
 namespace {
 
-void PostHistogram(const char* base_name,
-                   const char* suffix,
+void PostHistogram(std::string base_name,
+                   std::string suffix,

[Ilya Sherman, 2014/12/09 02:59:36]

nit: Why not const-ref?

[aiolos (Not reviewing), 2015/01/05 23:37:59]

Done.
I had changed it to std::string to simplify things since we should be passing a
const std::string to FactoryTimeGet, but have no strong feelings either way.

                    base::TimeDelta time) {
-  std::string histogram_name =
-      base::StringPrintf("ResourceScheduler.%s.%s", base_name, suffix);
+  std::string histogram_name = "ResourceScheduler." + base_name + '.' + suffix;
   base::HistogramBase* histogram_counter = base::Histogram::FactoryTimeGet(
       histogram_name,
       base::TimeDelta::FromMilliseconds(1),
@@ -168,7 +167,7 @@ class ResourceScheduler::ScheduledResourceRequest
     // tabs which have switched between active and background multiple times.
     // Ex: A tab with the following transitions Active -> Background -> Active
     // will be recorded as Active.
-    const char* client_state = "Other";
+    std::string client_state = "Other";
     if (current_state == client_state_on_creation_ && current_state == ACTIVE) {
       client_state = "Active";
     } else if (current_state == client_state_on_creation_ &&
@@ -368,15 +367,21 @@ class ResourceScheduler::Client {
       return;
     }
     base::TimeTicks cur_time = base::TimeTicks::Now();
-    const char* client_catagory = "Other";
+    std::string num_clients = scheduler_->GetNumClientsString();

[mmenke, 2014/12/09 15:53:28]

Per suggestion below, can make this a const char*

[aiolos (Not reviewing), 2015/01/05 23:37:59]

Done.

+    std::string client_catagory = "Other";
     if (last_active_switch_time_.is_null()) {
       client_catagory = is_active() ? "Active" : "Background";
     } else if (is_active()) {
-      PostHistogram("ClientLoadedTime", "Other.SwitchedToActive",
+      PostHistogram(
+          "ClientLoadedTime", "Other.SwitchedToActive",
+          cur_time - last_active_switch_time_);

[mmenke, 2014/12/09 15:53:28]

Suggest putting "cur_time - last_active_switch_time_" in a temporary, to make it
clearer 3 of this histograms contain the same value.

[aiolos (Not reviewing), 2015/01/05 23:37:58]

Done.

+      PostHistogram("ClientLoadedTime", "Other.SwitchedToActive." + num_clients,
                     cur_time - last_active_switch_time_);
     }
     PostHistogram("ClientLoadedTime", client_catagory,
                   cur_time - load_started_time_);
+    PostHistogram("ClientLoadedTime", client_catagory + '.' + num_clients,
+                  cur_time - load_started_time_);
     // 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.
@@ -790,6 +795,7 @@ class ResourceScheduler::Client {
 ResourceScheduler::ResourceScheduler()
     : should_coalesce_(false),
       should_throttle_(false),
+      num_clients_(0),
       active_clients_loading_(0),
       coalesced_clients_(0),
       coalescing_timer_(new base::Timer(true /* retain_user_task */,
@@ -878,9 +884,8 @@ 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();
+  ++num_clients_;
 }
 
 void ResourceScheduler::OnClientDeleted(int child_id, int route_id) {
@@ -903,6 +908,7 @@ void ResourceScheduler::OnClientDeleted(int child_id, int route_id) {
 
   delete client;
   client_map_.erase(it);
+  --num_clients_;
 }
 
 void ResourceScheduler::OnLoadingStateChanged(int child_id,
@@ -989,6 +995,18 @@ ResourceScheduler::Client* ResourceScheduler::GetClient(int child_id,
   return client_it->second;
 }
 
+std::string ResourceScheduler::GetNumClientsString() {

[mmenke, 2014/12/09 15:53:28]

Can't we return a const char*?

[mmenke, 2014/12/09 15:53:28]

+const.  Or better, remove it from ResourceScheduler, and instead move it into
an anonymous namespace in this file, and take the number of clients as a string.

[aiolos (Not reviewing), 2015/01/05 23:37:59]

Done.

[aiolos (Not reviewing), 2015/01/05 23:37:59]

Done.

+  if (num_clients_ == 1)

[mmenke, 2014/12/09 15:53:28]

I'd recommend just using client_map_.size() instead - it may cost more, but it's
guaranteed to take constant time.  It also has no chance of getting out of sync
with the actual number of clients (Or at least much less chance...).

[aiolos (Not reviewing), 2015/01/05 23:37:59]

Done.

+    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";
+}
+
 void ResourceScheduler::DecrementActiveClientsLoading() {
   DCHECK_NE(0u, active_clients_loading_);
   --active_clients_loading_;