Do not schedule to load pending startable request on RemoveRequest

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 "content/browser/loader/resource_scheduler.h"
 
 #include <stdint.h>
 #include <set>
 #include <string>
 #include <utility>
 #include <vector>
 
 #include "base/feature_list.h"
 #include "base/macros.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/field_trial_params.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/stl_util.h"
 #include "base/supports_user_data.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "base/trace_event/trace_event.h"
 #include "content/common/resource_messages.h"
 #include "content/public/browser/resource_request_info.h"
 #include "content/public/browser/resource_throttle.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"
 #include "url/scheme_host_port.h"
@@ skipping 144 matching lines @@
   }
 
   NetQueue::iterator GetNextHighestIterator() {
     return queue_.begin();
   }
 
   NetQueue::iterator End() {
     return queue_.end();
   }
 
+  size_t size() { return queue_.size(); }
+
   // Returns true if |request| is queued.
   bool IsQueued(ScheduledResourceRequest* request) const {
     return base::ContainsKey(pointers_, request);
   }
 
   // Returns true if no requests are queued.
   bool IsEmpty() const { return queue_.size() == 0; }
 
  private:
   typedef std::map<ScheduledResourceRequest*, NetQueue::iterator> PointerMap;
@@ skipping 169 matching lines @@
  public:
   Client(bool priority_requests_delayable,
          bool yielding_scheduler_enabled,
          int max_requests_before_yielding)
       : is_loaded_(false),
         has_html_body_(false),
         using_spdy_proxy_(false),
         in_flight_delayable_count_(0),
         total_layout_blocking_count_(0),
         priority_requests_delayable_(priority_requests_delayable),
-        has_pending_start_task_(false),
+        num_skipped_request_scans_for_delayed_start_(0),
         started_requests_since_yielding_(0),
         did_scheduler_yield_(false),
         yielding_scheduler_enabled_(yielding_scheduler_enabled),
         max_requests_before_yielding_(max_requests_before_yielding),
         weak_ptr_factory_(this) {}
 
   ~Client() {}
 
   void ScheduleRequest(net::URLRequest* url_request,
                        ScheduledResourceRequest* request) {
@@ skipping 10 matching lines @@
   }
 
   void RemoveRequest(ScheduledResourceRequest* request) {
     if (pending_requests_.IsQueued(request)) {
       pending_requests_.Erase(request);
       DCHECK(!base::ContainsKey(in_flight_requests_, request));
     } else {
       EraseInFlightRequest(request);
 
       // Removing this request may have freed up another to load.
-      ScheduleLoadAnyStartablePendingRequests(
+      LoadAnyStartablePendingRequests(
           has_html_body_ ? RequestStartTrigger::COMPLETION_POST_BODY
                          : RequestStartTrigger::COMPLETION_PRE_BODY);
     }
   }
 
   RequestSet StartAndRemoveAllRequests() {
     // First start any pending requests so that they will be moved into
     // in_flight_requests_. This may exceed the limits
     // kDefaultMaxNumDelayableRequestsPerClient and
     // kMaxNumDelayableRequestsPerHostPerClient, so this method must not do
@@ skipping 342 matching lines @@
   // It is common for a burst of messages to come from the renderer which
   // trigger starting pending requests. Naively, this would result in O(n*m)
   // behavior for n pending requests and m <= n messages, as
   // LoadAnyStartablePendingRequest is O(n) for n pending requests. To solve
   // this, just post a task to the end of the queue to call the method,
   // coalescing the m messages into a single call to
   // LoadAnyStartablePendingRequests.
   // TODO(csharrison): Reconsider this if IPC batching becomes an easy to use
   // pattern.
   void ScheduleLoadAnyStartablePendingRequests(RequestStartTrigger trigger) {
-    if (has_pending_start_task_)
-      return;
-    has_pending_start_task_ = true;
-    base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::Bind(&Client::LoadAnyStartablePendingRequests,
-                              weak_ptr_factory_.GetWeakPtr(), trigger));
+    if (num_skipped_request_scans_for_delayed_start_ == 0) {
+      TRACE_EVENT0("loading", "ScheduleLoadAnyStartablePendingRequests");
+      base::ThreadTaskRunnerHandle::Get()->PostTask(
+          FROM_HERE,
+          base::Bind(&Client::LoadAnyStartablePendingRequests,
+                     weak_ptr_factory_.GetWeakPtr(), trigger));
+    }
+    num_skipped_request_scans_for_delayed_start_ += pending_requests_.size();

[kinuko, 2017/02/24 01:38:28]

Why do we add pending_requests_ here?

[kinuko, 2017/02/24 01:55:45]

I think this wants to get the numbers that we might have been needed to
calculate if we didn't delay?  But if we called LoadAnyStartablePendingRequests
the # of pending_requests_ would have been changed, which feels a bit unfair?

[Charlie Harrison, 2017/02/24 02:02:00]

Hm... I'm not sure what better signal to use. Note that it is already an upper
bound, because we won't necessarily scan through all of these in
LoadAnyStartablePendingRequests.

   }
 
   void ResumeIfYielded() {
     bool yielded = did_scheduler_yield_;
     started_requests_since_yielding_ = 0;
     did_scheduler_yield_ = false;
 
     if (yielded)
       LoadAnyStartablePendingRequests(RequestStartTrigger::START_WAS_YIELDED);
   }
@@ skipping 12 matching lines @@
 
   void LoadAnyStartablePendingRequests(RequestStartTrigger trigger) {
     // We iterate through all the pending requests, starting with the highest
     // priority one. For each entry, one of three things can happen:
     // 1) We start the request, remove it from the list, and keep checking.
     // 2) We do NOT start the request, but ShouldStartRequest() signals us that
     //     there may be room for other requests, so we keep checking and leave
     //     the previous request still in the list.
     // 3) We do not start the request, same as above, but StartRequest() tells
     //     us there's no point in checking any further requests.
-    has_pending_start_task_ = false;
+    TRACE_EVENT0("loading", "LoadAnyStartablePendingRequests");
+    if (num_skipped_request_scans_for_delayed_start_ > 0) {
+      UMA_HISTOGRAM_COUNTS_1M(
+          "ResourceScheduler.NumSkippedRequestScans.ScheduleStart",
+          num_skipped_request_scans_for_delayed_start_);
+    }
+    num_skipped_request_scans_for_delayed_start_ = 0;
     RequestQueue::NetQueue::iterator request_iter =
         pending_requests_.GetNextHighestIterator();
 
     while (request_iter != pending_requests_.End()) {
       ScheduledResourceRequest* request = *request_iter;
       ShouldStartReqResult query_result = ShouldStartRequest(request);
 
       if (query_result == START_REQUEST) {
         pending_requests_.Erase(request);
         StartRequest(request, START_ASYNC, trigger);
@@ skipping 27 matching lines @@
   RequestSet in_flight_requests_;
   // The number of delayable in-flight requests.
   size_t in_flight_delayable_count_;
   // The number of layout-blocking in-flight requests.
   size_t total_layout_blocking_count_;
 
   // True if requests to servers that support priorities (e.g., H2/QUIC) can
   // be delayed.
   bool priority_requests_delayable_;
 
-  bool has_pending_start_task_;
+  // The upper bound for the number of requests that were not scanned due to
+  // logic in ScheduleLoadAnyStartablePendingRequests. This number also doubles
+  // as a signal for whether a task has already been scheduled.
+  int num_skipped_request_scans_for_delayed_start_;
 
   // The number of started requests since the last ResumeIfYielded task was
   // run.
   int started_requests_since_yielding_;
 
   // If the scheduler had to yield the start of a request since the last
   // ResumeIfYielded task was run.
   bool did_scheduler_yield_;
 
   // Whether or not to periodically yield when starting lots of requests.
@@ skipping 204 matching lines @@
   client->ReprioritizeRequest(scheduled_resource_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