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 197 matching lines @@
   explicit Client(ResourceScheduler* scheduler)
       : is_audible_(false),
         is_visible_(false),
         is_loaded_(false),
         has_body_(false),
         using_spdy_proxy_(false),
         total_delayable_count_(0),
         throttle_state_(ResourceScheduler::THROTTLED) {
     scheduler_ = scheduler;
   }
-  ~Client() {}
+
+  ~Client() {
+    // Update to default state and pause to ensure the scheduler has a
+    // correct count of important types of clients.

[mmenke, 2014/07/29 15:43:26]

nit:  I think "important" is a bit weird here.  Maybe "various" or "relevant"? 
(Admittedly, the latter can be a synonym for important).

[aiolos (Not reviewing), 2014/07/29 17:58:51]

Done.

+    is_visible_ = false;
+    is_audible_ = false;
+    UpdateThrottleState(true /* should_pause */);
+  }
 
   void ScheduleRequest(
       net::URLRequest* url_request,
       ScheduledResourceRequest* request) {
     if (ShouldStartRequest(request) == START_REQUEST) {
       StartRequest(request);
     } else {
       pending_requests_.Insert(request);
     }
   }
@@ skipping 23 matching lines @@
 
   bool is_active() const { return is_visible_ || is_audible_; }
 
   bool is_loaded() const { return is_loaded_; }
 
   void OnAudibilityChanged(bool is_audible) {
     if (is_audible == is_audible_) {
       return;
     }
     is_audible_ = is_audible;
-    UpdateThrottleState();
+    UpdateThrottleState(false /* should_pause */);
   }
 
   void OnVisibilityChanged(bool is_visible) {
     if (is_visible == is_visible_) {
       return;
     }
     is_visible_ = is_visible;
-    UpdateThrottleState();
+    UpdateThrottleState(false /* should_pause */);
   }
 
   void OnLoadingStateChanged(bool is_loaded) {
     if (is_loaded == is_loaded_) {
       return;
     }
     is_loaded_ = is_loaded;
-    UpdateThrottleState();
+    UpdateThrottleState(false /* should_pause */);
   }
 
-  void UpdateThrottleState() {
+  void UpdateThrottleState(bool should_pause) {
     ClientThrottleState old_throttle_state = throttle_state_;
-    if (is_active() && !is_loaded_) {
+    if (should_pause) {
+      SetThrottleState(PAUSED);
+    } else if (is_active() && !is_loaded_) {
       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();
+    }
   }
 
   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(): should_coalesce_(false),
-                                        should_throttle_(false),
-                                        active_clients_loading_(0) {
+ResourceScheduler::ResourceScheduler()
+    : should_coalesce_(false),
+      should_throttle_(false),
+      active_clients_loading_(0),
+      coalesced_clients_(0),
+      coalescing_timer_(new base::Timer(true /* retain_user_task */,
+                                        true /* is_repeating */)) {
 }
 
 ResourceScheduler::~ResourceScheduler() {
   DCHECK(unowned_requests_.empty());
   DCHECK(client_map_.empty());
 }
 
 void ResourceScheduler::SetThrottleOptionsForTesting(bool should_throttle,
                                                      bool should_coalesce) {
   should_coalesce_ = should_coalesce;
   should_throttle_ = should_throttle;
-  OnLoadingActiveClientsStateChanged();
+  OnLoadingActiveClientsStateChangedForAllClients();
 }
 
 ResourceScheduler::ClientThrottleState
 ResourceScheduler::GetClientStateForTesting(int child_id, int route_id) {
   Client* client = GetClient(child_id, route_id);
   DCHECK(client);
   return client->throttle_state();
 }
 
 scoped_ptr<ResourceThrottle> ResourceScheduler::ScheduleRequest(
@@ skipping 41 matching lines @@
 void ResourceScheduler::OnClientCreated(int child_id, int route_id) {
   DCHECK(CalledOnValidThread());
   ClientId client_id = MakeClientId(child_id, route_id);
   DCHECK(!ContainsKey(client_map_, client_id));
 
   Client* client = new Client(this);
   client_map_[client_id] = client;
 
   // TODO(aiolos): set Client visibility/audibility when signals are added
   // this will UNTHROTTLE Clients as needed
-  client->UpdateThrottleState();
+  client->UpdateThrottleState(false /* should_paused */);
 }
 
 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;
-  client->OnLoadingStateChanged(true);
   // 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;
@@ skipping 75 matching lines @@
     return NULL;
   }
   return client_it->second;
 }
 
 void ResourceScheduler::DecrementActiveClientsLoading() {
   DCHECK_NE(0u, active_clients_loading_);
   --active_clients_loading_;
   DCHECK_EQ(active_clients_loading_, CountActiveClientsLoading());
   if (active_clients_loading_ == 0) {
-    OnLoadingActiveClientsStateChanged();
+    OnLoadingActiveClientsStateChangedForAllClients();
   }
 }
 
 void ResourceScheduler::IncrementActiveClientsLoading() {
   ++active_clients_loading_;
   DCHECK_EQ(active_clients_loading_, CountActiveClientsLoading());
   if (active_clients_loading_ == 1) {
-    OnLoadingActiveClientsStateChanged();
+    OnLoadingActiveClientsStateChangedForAllClients();
   }
 }
 
-void ResourceScheduler::OnLoadingActiveClientsStateChanged() {
+void ResourceScheduler::OnLoadingActiveClientsStateChangedForAllClients() {
   ClientMap::iterator client_it = client_map_.begin();
   while (client_it != client_map_.end()) {
     Client* client = client_it->second;
-    client->UpdateThrottleState();
+    client->UpdateThrottleState(false /* should_pause */);

[mmenke, 2014/07/29 15:43:26]

This unpauses all clients.  While not yet a bug, it seems potentially
problematic.

My suggestion:

Add a function:

Client::SetPaused(bool is_paused) {
  is_paused_ = is_paused;
  UpdateThrottleState();
}

And add a check for is_paused_ in update throttle state.  Open to other ideas.

[aiolos (Not reviewing), 2014/07/29 17:58:51]

Done.

     ++client_it;
   }
 }
 
-size_t ResourceScheduler::CountActiveClientsLoading() {
+size_t ResourceScheduler::CountActiveClientsLoading() const {
   size_t active_and_loading = 0;
-  ClientMap::iterator client_it = client_map_.begin();
+  ClientMap::const_iterator client_it = client_map_.begin();
   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() {
+  ++coalesced_clients_;
+  DCHECK_EQ(coalesced_clients_, CountCoalescedClients());

[mmenke, 2014/07/29 15:43:26]

DCHECK(should_coalesce_), maybe?

Could also put that in some of these other functions, I believe.

[aiolos (Not reviewing), 2014/07/29 17:58:51]

Done.

+  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() const {
+  size_t coalesced_clients = 0;
+  ClientMap::const_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() {
+  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