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 {
 
-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),
       base::TimeDelta::FromMinutes(5),
       50,
       base::Histogram::kUmaTargetedHistogramFlag);
   histogram_counter->AddTime(time);
 }
 
 }  // namespace
@@ skipping 116 matching lines @@
     TRACE_EVENT_ASYNC_STEP_PAST0("net", "URLRequest", request_, "Queued");
     ready_ = true;
     if (!request_->status().is_success())
       return;
     base::TimeTicks time = base::TimeTicks::Now();
     ClientState current_state = scheduler_->GetClientState(client_id_);
     // Note: the client state isn't perfectly accurate since it won't capture
     // 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_ &&
                current_state == BACKGROUND) {
       client_state = "Background";
     }
 
     base::TimeDelta time_was_deferred = base::TimeDelta::FromMicroseconds(0);
     if (deferred_) {
       deferred_ = false;
@@ skipping 179 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* 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.
     // Find a way to fix both of these issues.
   }
 
   void SetPaused() {
     is_paused_ = true;
     UpdateThrottleState();
   }
@@ skipping 393 matching lines @@
   // The number of delayable in-flight requests.
   size_t in_flight_delayable_count_;
   // The number of layout-blocking in-flight requests.
   size_t total_layout_blocking_count_;
   ResourceScheduler::ClientThrottleState throttle_state_;
 };
 
 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 */,
                                         true /* is_repeating */)) {
   std::string throttling_trial_group =
       base::FieldTrialList::FindFullName("RequestThrottlingAndCoalescing");
   if (throttling_trial_group == "Throttle") {
     should_throttle_ = true;
   } else if (throttling_trial_group == "Coalesce") {
     should_coalesce_ = true;
@@ skipping 68 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();
+  ++num_clients_;
 }
 
 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;
 
   Client* client = it->second;
   // FYI, ResourceDispatcherHost cancels all of the requests after this function
   // is called. It should end up canceling all of the requests except for a
   // cross-renderer navigation.
   RequestSet client_unowned_requests = client->RemoveAllRequests();
   for (RequestSet::iterator it = client_unowned_requests.begin();
        it != client_unowned_requests.end(); ++it) {
     unowned_requests_.insert(*it);
   }
 
   delete client;
   client_map_.erase(it);
+  --num_clients_;
 }
 
 void ResourceScheduler::OnLoadingStateChanged(int child_id,
                                               int route_id,
                                               bool is_loaded) {
   Client* client = GetClient(child_id, route_id);
   DCHECK(client);
   client->OnLoadingStateChanged(is_loaded);
 }
 
@@ skipping 66 matching lines @@
 ResourceScheduler::Client* ResourceScheduler::GetClient(int child_id,
                                                         int route_id) {
   ClientId client_id = MakeClientId(child_id, route_id);
   ClientMap::iterator client_it = client_map_.find(client_id);
   if (client_it == client_map_.end()) {
     return NULL;
   }
   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_;
   DCHECK_EQ(active_clients_loading_, CountActiveClientsLoading());
   if (active_clients_loading_ == 0) {
     OnLoadingActiveClientsStateChangedForAllClients();
   }
 }
 
 void ResourceScheduler::IncrementActiveClientsLoading() {
@@ skipping 112 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