Run load state update timer only when needed

content/browser/loader/resource_dispatcher_host_impl.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.
 
 // See http://dev.chromium.org/developers/design-documents/multi-process-resource-loading
 
 #include "content/browser/loader/resource_dispatcher_host_impl.h"
 
 #include <algorithm>
 #include <set>
@@ skipping 97 matching lines @@
 
 // ----------------------------------------------------------------------------
 
 namespace content {
 
 namespace {
 
 static ResourceDispatcherHostImpl* g_resource_dispatcher_host;
 
 // The interval for calls to ResourceDispatcherHostImpl::UpdateLoadStates
-const int kUpdateLoadStatesIntervalMsec = 100;
+const int kUpdateLoadStatesIntervalMsec = 250;
 
 // Maximum byte "cost" of all the outstanding requests for a renderer.
 // See delcaration of |max_outstanding_requests_cost_per_process_| for details.
 // This bound is 25MB, which allows for around 6000 outstanding requests.
 const int kMaxOutstandingRequestsCostPerProcess = 26214400;
 
 // The number of milliseconds after noting a user gesture that we will
 // tag newly-created URLRequest objects with the
 // net::LOAD_MAYBE_USER_GESTURE load flag. This is a fairly arbitrary
 // guess at how long to expect direct impact from a user gesture, but
@@ skipping 317 matching lines @@
   ANNOTATE_BENIGN_RACE(
       &last_user_gesture_time_,
       "We don't care about the precise value, see http://crbug.com/92889");
 
   BrowserThread::PostTask(BrowserThread::IO,
                           FROM_HERE,
                           base::Bind(&ResourceDispatcherHostImpl::OnInit,
                                      base::Unretained(this)));
 
   update_load_states_timer_.reset(
-      new base::RepeatingTimer<ResourceDispatcherHostImpl>());
+      new base::OneShotTimer<ResourceDispatcherHostImpl>());
 }
 
 ResourceDispatcherHostImpl::~ResourceDispatcherHostImpl() {
   DCHECK(outstanding_requests_stats_map_.empty());
   DCHECK(g_resource_dispatcher_host);
   g_resource_dispatcher_host = NULL;
 }
 
 // static
 ResourceDispatcherHostImpl* ResourceDispatcherHostImpl::Get() {
@@ skipping 314 matching lines @@
       info->GetContext()->GetRequestContext()->job_factory();
   if (job_factory->IsHandledURL(url))
     return false;
 
   return delegate_->HandleExternalProtocol(
       url, info->GetChildID(), info->GetRouteID(), info->IsMainFrame(),
       info->GetPageTransition(), info->HasUserGesture());
 }
 
 void ResourceDispatcherHostImpl::DidStartRequest(ResourceLoader* loader) {
-  // Make sure we have the load state monitor running
-  if (!update_load_states_timer_->IsRunning()) {
-    update_load_states_timer_->Start(
-        FROM_HERE, TimeDelta::FromMilliseconds(kUpdateLoadStatesIntervalMsec),
-        this, &ResourceDispatcherHostImpl::UpdateLoadInfo);
-  }
+  if (scheduler_->HasLoadingClients())
+    ScheduleUpdateLoadInfo();
 }
 
 void ResourceDispatcherHostImpl::DidReceiveRedirect(ResourceLoader* loader,
                                                     const GURL& new_url) {
   ResourceRequestInfoImpl* info = loader->GetRequestInfo();
 
   int render_process_id, render_frame_host;
   if (!info->GetAssociatedRenderFrame(&render_process_id, &render_frame_host))
     return;
 
@@ skipping 126 matching lines @@
 }
 
 void ResourceDispatcherHostImpl::OnShutdown() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   is_shutdown_ = true;
   pending_loaders_.clear();
 
   // Make sure we shutdown the timer now, otherwise by the time our destructor
   // runs if the timer is still running the Task is deleted twice (once by
-  // the MessageLoop and the second time by RepeatingTimer).
+  // the MessageLoop and the second time by OneShotTimer).
   update_load_states_timer_.reset();
 
   // Clear blocked requests if any left.
   // Note that we have to do this in 2 passes as we cannot call
   // CancelBlockedRequestsForRoute while iterating over
   // blocked_loaders_map_, as it modifies it.
   std::set<GlobalRoutingID> ids;
   for (BlockedLoadersMap::const_iterator iter = blocked_loaders_map_.begin();
        iter != blocked_loaders_map_.end(); ++iter) {
     std::pair<std::set<GlobalRoutingID>::iterator, bool> result =
@@ skipping 820 matching lines @@
 
 void ResourceDispatcherHostImpl::RemovePendingLoader(
     const LoaderMap::iterator& iter) {
   ResourceRequestInfoImpl* info = iter->second->GetRequestInfo();
 
   // Remove the memory credit that we added when pushing the request onto
   // the pending list.
   IncrementOutstandingRequestsMemory(-1, *info);
 
   pending_loaders_.erase(iter);
-
-  // If we have no more pending requests, then stop the load state monitor
-  if (pending_loaders_.empty() && update_load_states_timer_)
-    update_load_states_timer_->Stop();
 }
 
 void ResourceDispatcherHostImpl::CancelRequest(int child_id,
                                                int request_id) {
   ResourceLoader* loader = GetLoader(child_id, request_id);
   if (!loader) {
     // We probably want to remove this warning eventually, but I wanted to be
     // able to notice when this happens during initial development since it
     // should be rare and may indicate a bug.
     DVLOG(1) << "Canceling a request that wasn't found";
@@ skipping 362 matching lines @@
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "466285 ResourceDispatcherHostImpl::UpdateLoadInfoOnUIThread"));
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   for (const auto& load_info : *info_map) {
     RenderViewHostImpl* view = RenderViewHostImpl::FromID(
         load_info.first.child_id, load_info.first.route_id);
     // The view could be gone at this point.
     if (view) {
       view->LoadStateChanged(load_info.second.url, load_info.second.load_state,
                              load_info.second.upload_position,
                              load_info.second.upload_size);

[mmenke, 2015/05/21 15:53:25]

You should consider updating the GetLoadState() docs in
content/public/browser/web_contents.h to mention this change.

Admittedly, we weren't calling WebContentsDelegate::NavigationStateChanged when
not loading, but now we're also not modifying the WebContent's publicly
accessible load state.

[Andre, 2015/05/21 22:05:39]

Done.

     }
   }
 }
 
 scoped_ptr<ResourceDispatcherHostImpl::LoadInfoMap>
 ResourceDispatcherHostImpl::GetLoadInfoForAllRoutes() {
   // Populate this map with load state changes, and then send them on to the UI
   // thread where they can be passed along to the respective RVHs.
   scoped_ptr<LoadInfoMap> info_map(new LoadInfoMap());
 
   for (const auto& loader : pending_loaders_) {

[mmenke, 2015/05/21 15:53:25]

Should actually remove this call in the other CL.

[Andre, 2015/05/21 22:05:39]

Done.

-    // Also poll for upload progress on this timer and send upload progress ipc
-    // messages to the plugin process.
-    loader.second->ReportUploadProgress();
-
     net::URLRequest* request = loader.second->request();
     net::UploadProgress upload_progress = request->GetUploadProgress();
 
     LoadInfo load_info;
     load_info.url = request->url();
     load_info.load_state = request->GetLoadState();
     load_info.upload_size = upload_progress.size();
     load_info.upload_position = upload_progress.position();
 
     GlobalRoutingID id(loader.second->GetRequestInfo()->GetGlobalRoutingID());
     LoadInfoMap::iterator existing = info_map->find(id);
 
     if (existing == info_map->end() ||
         LoadInfoIsMoreInteresting(load_info, existing->second)) {
       (*info_map)[id] = load_info;
     }
   }
   return info_map.Pass();
 }
 
 void ResourceDispatcherHostImpl::UpdateLoadInfo() {
   scoped_ptr<LoadInfoMap> info_map(GetLoadInfoForAllRoutes());
 
   if (info_map->empty())
     return;

[mmenke, 2015/05/21 15:53:25]

I think a repeating timer with an explicit Stop() here and a comment on it makes
for clearer code.  (And I think we're still fine with the one in
RemovePendingLoader removed).

So this would be:

// Comment on why both of these are fine to stop on.
if (info_map->empty() || !scheduler_->HasLoadingClients()) {
  // Stop();
  return;
}

Note that we'd also not be calling UpdateLoadInfoOnUIThread when there are no
loading clients, for potentially very modest savings.

[Andre, 2015/05/21 22:05:39]

You're right. Now that the upload progress code is gone, it's simpler to use a
RepeatingTimer.
Done.

 
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       base::Bind(&ResourceDispatcherHostImpl::UpdateLoadInfoOnUIThread,
                  base::Passed(&info_map)));
+
+  if (scheduler_->HasLoadingClients())
+     ScheduleUpdateLoadInfo();

[mmenke, 2015/05/21 15:53:25]

Should use 2 space indent here, not 3

[Andre, 2015/05/21 22:05:39]

Done.

 }
 
 void ResourceDispatcherHostImpl::BlockRequestsForRoute(int child_id,
                                                        int route_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   GlobalRoutingID key(child_id, route_id);
   DCHECK(blocked_loaders_map_.find(key) == blocked_loaders_map_.end()) <<
       "BlockRequestsForRoute called  multiple time for the same RVH";
   blocked_loaders_map_[key] = new BlockedLoadersList();
 }
@@ skipping 142 matching lines @@
   }
 
   // Add a flag to selectively bypass the data reduction proxy if the resource
   // type is not an image.
   if (request_data.resource_type != RESOURCE_TYPE_IMAGE)
     load_flags |= net::LOAD_BYPASS_DATA_REDUCTION_PROXY;
 
   return load_flags;
 }
 
+void ResourceDispatcherHostImpl::ScheduleUpdateLoadInfo() {
+  if (!update_load_states_timer_->IsRunning()) {

[mmenke, 2015/05/21 15:53:25]

Suggest early return instead - it's generally preferred.

[Andre, 2015/05/21 22:05:39]

Done.

+    update_load_states_timer_->Start(
+        FROM_HERE, TimeDelta::FromMilliseconds(kUpdateLoadStatesIntervalMsec),
+        this, &ResourceDispatcherHostImpl::UpdateLoadInfo);
+  }
+}
+
 }  // namespace content