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

content/browser/loader/resource_scheduler.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 <set>
 
 #include "content/browser/loader/resource_scheduler.h"
 
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram.h"
 #include "base/stl_util.h"
 #include "base/time/time.h"
 #include "content/common/resource_messages.h"
 #include "content/browser/loader/resource_message_delegate.h"
 #include "content/public/browser/resource_controller.h"
 #include "content/public/browser/resource_request_info.h"
 #include "content/public/browser/resource_throttle.h"
 #include "ipc/ipc_message_macros.h"
 #include "net/base/host_port_pair.h"
 #include "net/base/load_flags.h"
 #include "net/base/request_priority.h"
 #include "net/http/http_server_properties.h"
 #include "net/url_request/url_request.h"
 #include "net/url_request/url_request_context.h"
 
 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,
       base::Histogram::kUmaTargetedHistogramFlag);
   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;
 static const size_t kMaxNumDelayableRequestsPerClient = 10;
 static const size_t kMaxNumDelayableRequestsPerHost = 6;
 static const size_t kMaxNumThrottledRequestsPerClient = 1;
 
 struct ResourceScheduler::RequestPriorityParams {
   RequestPriorityParams()
     : priority(net::DEFAULT_PRIORITY),
@@ skipping 121 matching lines @@
                current_state == BACKGROUND) {
       client_state = "Background";
     }
 
     base::TimeDelta time_was_deferred = base::TimeDelta::FromMicroseconds(0);
     if (deferred_) {
       deferred_ = false;
       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.
   }
 
   void set_request_priority_params(const RequestPriorityParams& priority) {
     priority_ = priority;
   }
   const RequestPriorityParams& get_request_priority_params() const {
     return priority_;
@@ skipping 163 matching lines @@
       return;
     }
     is_loaded_ = is_loaded;
     UpdateThrottleState();
     if (!is_loaded_) {
       load_started_time_ = base::TimeTicks::Now();
       last_active_switch_time_ = base::TimeTicks();
       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.
     // Find a way to fix both of these issues.
   }
 
   void SetPaused() {
     is_paused_ = true;
     UpdateThrottleState();
   }
@@ skipping 481 matching lines @@
                                         int route_id,
                                         bool is_visible,
                                         bool is_audible) {
   DCHECK(CalledOnValidThread());
   ClientId client_id = MakeClientId(child_id, route_id);
   DCHECK(!ContainsKey(client_map_, client_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();
 }
 
 void ResourceScheduler::OnClientDeleted(int child_id, int route_id) {
   DCHECK(CalledOnValidThread());
   ClientId client_id = MakeClientId(child_id, route_id);
   DCHECK(ContainsKey(client_map_, client_id));
   ClientMap::iterator it = client_map_.find(client_id);
   if (it == client_map_.end())
     return;
@@ skipping 98 matching lines @@
 
 void ResourceScheduler::DecrementActiveClientsLoading() {
   DCHECK_NE(0u, active_clients_loading_);
   --active_clients_loading_;
   DCHECK_EQ(active_clients_loading_, CountActiveClientsLoading());
   if (active_clients_loading_ == 0) {
     OnLoadingActiveClientsStateChangedForAllClients();
   }
 }
 
+size_t ResourceScheduler::GetNumClients() const {
+  return client_map_.size();
+}
+
 void ResourceScheduler::IncrementActiveClientsLoading() {
   ++active_clients_loading_;
   DCHECK_EQ(active_clients_loading_, CountActiveClientsLoading());
   if (active_clients_loading_ == 1) {
     OnLoadingActiveClientsStateChangedForAllClients();
   }
 }
 
 void ResourceScheduler::OnLoadingActiveClientsStateChangedForAllClients() {
   ClientMap::iterator client_it = client_map_.begin();
@@ skipping 103 matching lines @@
   client->ReprioritizeRequest(
       request, old_priority_params, new_priority_params);
 }
 
 ResourceScheduler::ClientId ResourceScheduler::MakeClientId(
     int child_id, int route_id) {
   return (static_cast<ResourceScheduler::ClientId>(child_id) << 32) | route_id;
 }
 
 }  // namespace content