Do not schedule to load pending startable request on RemoveRequest

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 5f8c5743eaa942546b867035cabaa9a94d6baefb..73bb1e5818a4ce5cf098f39857e2a7fe8874e501 100644
--- a/content/browser/loader/resource_scheduler.cc
+++ b/content/browser/loader/resource_scheduler.cc
@@ -18,6 +18,7 @@
 #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"
@@ -182,6 +183,8 @@ class ResourceScheduler::RequestQueue {
     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);
@@ -371,7 +374,7 @@ class ResourceScheduler::Client {
         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),
@@ -402,7 +405,7 @@ class ResourceScheduler::Client {
       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);
     }
@@ -765,12 +768,14 @@ class ResourceScheduler::Client {
   // 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() {
@@ -803,7 +808,13 @@ class ResourceScheduler::Client {
     //     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();
 
@@ -851,7 +862,10 @@ class ResourceScheduler::Client {
   // 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.