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 236 matching lines @@
       (*it)->set_accounted_as_delayable_request(false);
     }
     ClearInFlightRequests();
     return unowned_requests;
   }
 
   bool is_active() const { return is_visible_ || is_audible_; }
 
   bool is_loaded() const { return is_loaded_; }
 
+  // Reset state to default values for client deletion.

[mmenke, 2014/07/25 16:33:27]

Should mention this must be called prior to deletion.  Maybe also rename this
ResetStateForDeletion?

+  void ResetState() {

[mmenke, 2014/07/25 16:33:27]

Why can't you just do this on destruction?  I seem to recall some re-entrancy
related reason, but I'm having trouble figuring out what it was.

[aiolos (Not reviewing), 2014/07/25 16:54:50]

Good point.

+    is_visible_ = false;
+    is_audible_ = false;
+    is_loaded_ = false;
+    UpdateThrottleState();

[mmenke, 2014/07/25 16:33:27]

This strikes me as regression-prone logic.  We're relying on a non-visible,
non-audible, unloaded client never to be in a state that the ResourceScheduler
tracks.  If that changes, things break, and it's hard to have DCHECKs that
verify that never changes.

You had talked about adding a paused state...Maybe add something like that and
just use it for state on destruction, for now?  Open to other ideas.

[aiolos (Not reviewing), 2014/07/25 16:54:50]

I'd considered doing that. I was planning on adding that in the patch limiting
the number of throttled tabs, to keep this CL smaller. But putting it in now
works too. I assume you'd like tests for paused state added at the same time?

[mmenke, 2014/07/25 17:02:41]

If we just use it for shutdown for now, and then more broadly later, I'm fine
with the current shutdown tests for the moment.

Also, don't think we'll need quite as many tests for pausing as we have for the
current behaviors - just one or two tests pausing and unpausing should probably
be enough.

[aiolos (Not reviewing), 2014/07/25 17:12:58]

Agreed. Although shouldn't sync requests get issued even for paused Clients so
we don't hang the render? I assume we should have a test for that as well.

[mmenke, 2014/07/25 17:22:19]

Good point - a test for that definitely makes sense.

+  }
+
   void OnAudibilityChanged(bool is_audible) {
     if (is_audible == is_audible_) {
       return;
     }
     is_audible_ = is_audible;
     UpdateThrottleState();
   }
 
   void OnVisibilityChanged(bool is_visible) {
     if (is_visible == is_visible_) {
@@ skipping 17 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();
+    }
   }
 
   void OnNavigate() {
     has_body_ = false;
     is_loaded_ = false;
   }
 
   void OnWillInsertBody() {
     has_body_ = true;
     LoadAnyStartablePendingRequests();
@@ skipping 50 matching lines @@
     throttle_state_ = throttle_state;
     LoadAnyStartablePendingRequests();
     // TODO(aiolos): Stop any started but not inflght requests when
     // switching to stricter throttle state?
   }
 
   ResourceScheduler::ClientThrottleState throttle_state() const {
     return throttle_state_;
   }
 
-  void LoadCoalescedRequests() {
+  void LoadRequestsForAllCoalescedClients() {

[mmenke, 2014/07/25 16:33:27]

Undo this rename.

     if (throttle_state_ != COALESCED) {
       return;
     }
     if (scheduler_->active_clients_loaded()) {
       SetThrottleState(UNTHROTTLED);
     } else {
       SetThrottleState(THROTTLED);
     }
     LoadAnyStartablePendingRequests();
     SetThrottleState(COALESCED);
@@ skipping 220 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 53 matching lines @@
 
 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);
+  client->ResetState();
   // 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);
   }

[mmenke, 2014/07/25 16:33:27]

Should this be done before changing the state?

 
   delete client;
   client_map_.erase(it);
 }
 
 void ResourceScheduler::OnNavigate(int child_id, int route_id) {
   DCHECK(CalledOnValidThread());
   ClientId client_id = MakeClientId(child_id, route_id);
 
   ClientMap::iterator it = client_map_.find(client_id);
@@ skipping 67 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_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());
+  if (coalesced_clients_ == 1) {
+    coalescing_timer_->Start(
+        FROM_HERE,
+        base::TimeDelta::FromMilliseconds(kCoalescedTimerPeriod),
+        base::Bind(&ResourceScheduler::LoadRequestsForAllCoalescedClients,
+                   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::LoadRequestsForAllCoalescedClients() {
+  ClientMap::iterator client_it = client_map_.begin();
+  while (client_it != client_map_.end()) {
+    Client* client = client_it->second;
+    client->LoadRequestsForAllCoalescedClients();
+    ++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