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

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 f21dd4badfadafc10f5b15e2dce73e9e003200d3..38593580a4930691d078abdef79148a8251c1025 100644
--- a/content/browser/loader/resource_scheduler.cc
+++ b/content/browser/loader/resource_scheduler.cc
@@ -22,6 +22,7 @@
 
 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;
@@ -315,6 +316,7 @@ class ResourceScheduler::Client {
     } else if (!is_active()) {
       SetThrottleState(UNTHROTTLED);
     }
+
     if (throttle_state_ == old_throttle_state) {
       return;
     }
@@ -323,6 +325,11 @@ class ResourceScheduler::Client {
     } 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() {
@@ -634,7 +641,10 @@ class ResourceScheduler::Client {
   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() {
@@ -850,6 +860,49 @@ size_t ResourceScheduler::CountActiveClientsLoading() {
   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) {