Add coalescing timer to ResourceScheduler. CL 2 from BUG=128035.

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/stl_util.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 {
 
+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),
       intra_priority(0) {
   }
 
@@ skipping 273 matching lines @@
       SetThrottleState(ACTIVE_AND_LOADING);
     } else if (is_active()) {
       SetThrottleState(UNTHROTTLED);
     } else if (!scheduler_->active_clients_loaded()) {
       SetThrottleState(THROTTLED);
     } else if (is_loaded_ && scheduler_->should_coalesce()) {
       SetThrottleState(COALESCED);
     } else if (!is_active()) {
       SetThrottleState(UNTHROTTLED);
     }
+
     if (throttle_state_ == old_throttle_state) {
       return;
     }
     if (throttle_state_ == ACTIVE_AND_LOADING) {
       scheduler_->IncrementActiveClientsLoading();
     } else if (old_throttle_state == ACTIVE_AND_LOADING) {
       scheduler_->DecrementActiveClientsLoading();
     }
+    if (throttle_state_ == COALESCED) {
+      scheduler_->IncrementCoalescedClients();
+    } else if (old_throttle_state == COALESCED) {
+      scheduler_->DecrementCoalescedClients();
+    }

[aiolos (Not reviewing), 2014/07/15 23:54:58]

We don't need to have the timer running when we don't have coalesced Clients
(Good example: user with one tab open). To avoid battery usage in those cases,
keep track of the coalesced Clients the same way we keep track of active and
loading Clients.

[mmenke, 2014/07/16 17:01:34]

We could be even more careful with this - if no coalesced clients have any
pending resource requests, we also don't need it running.  No need to worry
about it in this CL, just a suggestion for the future.

   }
 
   void OnNavigate() {
     has_body_ = false;
     is_loaded_ = false;
   }
 
   void OnWillInsertBody() {
     has_body_ = true;
     LoadAnyStartablePendingRequests();
@@ skipping 291 matching lines @@
   bool has_body_;
   bool using_spdy_proxy_;
   RequestQueue pending_requests_;
   RequestSet in_flight_requests_;
   ResourceScheduler* scheduler_;
   // The number of delayable in-flight requests.
   size_t total_delayable_count_;
   ResourceScheduler::ClientThrottleState throttle_state_;
 };
 
-ResourceScheduler::ResourceScheduler() : active_clients_loading_(0) {
+ResourceScheduler::ResourceScheduler()
+    : active_clients_loading_(0),
+      coalesced_clients_(0),
+      coalescing_timer_(new base::Timer(true, true)) {

[mmenke, 2014/07/16 17:01:34]

Suggest labelling the two arguments, like:

new base::timer(true /* retain_user_task */, ....

As-is, it's difficult to tell that it's a repeating timer and not a oneshot
timer (The latter are more common, in my experience, so easy to assume it's one
of them as well).  Could also optionally add "repeating" to the name of the
timer, to make it clearer, or add a comment where it's declared.

 }
 
 ResourceScheduler::~ResourceScheduler() {
   DCHECK(unowned_requests_.empty());
   DCHECK(client_map_.empty());
 }
 
 void ResourceScheduler::SetThrottleOptionsForTesting(bool should_throttle,
                                                      bool should_coalesce) {
   should_coalesce_ = should_coalesce;
@@ skipping 195 matching lines @@
   while (client_it != client_map_.end()) {
     Client* client = client_it->second;
     if (client->throttle_state() == ACTIVE_AND_LOADING) {
       ++active_and_loading;
     }
     ++client_it;
   }
   return active_and_loading;
 }
 
+void ResourceScheduler::IncrementCoalescedClients() {

[aiolos (Not reviewing), 2014/07/15 23:54:58]

I wondered about adding tests to check that the timer is running iff there are
coalesced Clients, but didn't know if that was too much of an implementation
detail to have in the unittests based on one of your comments from the last CL.
Thoughts?

[mmenke, 2014/07/16 17:01:34]

It is a bit of a transparent implementation detail, but I think it's worth
testing, since it has real-world performance implications, and regressions are
very likely to go unnoticed.

+  ++coalesced_clients_;
+  DCHECK_EQ(coalesced_clients_, CountCoalescedClients());
+  if (coalesced_clients_ == 1) {
+    coalescing_timer_->Start(
+        FROM_HERE,
+        base::TimeDelta::FromMilliseconds(kCoalescedTimerPeriod),
+        base::Bind(&ResourceScheduler::LoadCoalescedRequests,
+                   base::Unretained(this)));
+  }
+}
+
+void ResourceScheduler::DecrementCoalescedClients() {
+  DCHECK_NE(0U, coalesced_clients_);
+  --coalesced_clients_;
+  DCHECK_EQ(coalesced_clients_, CountCoalescedClients());
+  if (coalesced_clients_ == 0) {
+    coalescing_timer_->Stop();
+  }
+}
+
+size_t ResourceScheduler::CountCoalescedClients() {

[mmenke, 2014/07/16 17:01:33]

nit:  This should be const (As should CountActiveClientsLoading())

+  size_t coalesced_clients = 0;
+  ClientMap::iterator client_it = client_map_.begin();
+  while (client_it != client_map_.end()) {
+    Client* client = client_it->second;
+    if (client->throttle_state() == COALESCED) {
+      ++coalesced_clients;
+    }
+    ++client_it;
+  }
+  return coalesced_clients_;
+}
+
+void ResourceScheduler::LoadCoalescedRequests() {

[mmenke, 2014/07/16 17:01:34]

nit:  Should use a similar naming scheme here as in
OnLoadingActiveClientsStateChanged, given the parallels in code structure (Also
think the name doesn't make it clear enough it affects all requests, so if you
go with what they do for both functions, instead of when they're called, suggest
adding "ForAllClients" or somesuch to the names).

[aiolos (Not reviewing), 2014/07/24 17:09:54]

The code structures are parallel, but when/why they are called isn't.
OnLoadingActiveClientsStateChanged is called when the first client to becomes
active and loading enters that state, and when the last active and loading
client leaves that state. It can either throttle or unthrottle clients in other
states, depending on whether the first or last active and loading client changed
state. Added "ForAllClients" to this call.

LoadCoalescedRequests on the other hand is called specifically to load coalesced
requests. It won't effect non-coalesced clients. It is called when the first
client enters the coalesced state, and will be called when we get notified of
the network becoming active. It won't be called when the last client leaves the
coalesced state. I'd rather keep this different from
OnLoadingActiveClientsStateChangedForAllClients. I changed it to
LoadRequestsForAllCoalescedClients.

+  ClientMap::iterator client_it = client_map_.begin();
+  while (client_it != client_map_.end()) {
+    Client* client = client_it->second;
+    client->LoadCoalescedRequests();
+    ++client_it;
+  }
+}
+
 void ResourceScheduler::ReprioritizeRequest(ScheduledResourceRequest* request,
                                             net::RequestPriority new_priority,
                                             int new_intra_priority_value) {
   if (request->url_request()->load_flags() & net::LOAD_IGNORE_LIMITS) {
     // We should not be re-prioritizing requests with the
     // IGNORE_LIMITS flag.
     NOTREACHED();
     return;
   }
   RequestPriorityParams new_priority_params(new_priority,
@@ skipping 18 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