Delay resource scheduling decisions until network access.

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>
@@ skipping 14 matching lines @@
 #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 {
 
 namespace {
 
-enum StartMode {
-  START_SYNC,
-  START_ASYNC
+enum ResumeMode {
+  RESUME_SYNC,
+  RESUME_ASYNC
 };
 
 // Field trial constants
 const char kRequestLimitFieldTrial[] = "OutstandingRequestLimiting";
 const char kRequestLimitFieldTrialGroupPrefix[] = "Limit";
 
 const char kResourcePrioritiesFieldTrial[] = "ResourcePriorities";
 
 // Flags identifying various attributes of the request that are used
 // when making scheduling decisions.
@@ skipping 96 matching lines @@
 // interact with the request.
 class ResourceScheduler::ScheduledResourceRequest : public ResourceThrottle {
  public:
   ScheduledResourceRequest(const ClientId& client_id,
                            net::URLRequest* request,
                            ResourceScheduler* scheduler,
                            const RequestPriorityParams& priority,
                            bool is_async)
       : client_id_(client_id),
         request_(request),
-        ready_(false),
-        deferred_(false),
+        paused_(false),
         is_async_(is_async),
         attributes_(kAttributeNone),
         scheduler_(scheduler),
         priority_(priority),
         fifo_ordering_(0),
         weak_ptr_factory_(this) {
     DCHECK(!request_->GetUserData(kUserDataKey));
     request_->SetUserData(kUserDataKey, new UnownedPointer(this));
   }
 
   ~ScheduledResourceRequest() override {
-    request_->RemoveUserData(kUserDataKey);
-    scheduler_->RemoveRequest(this);
-  }
+      request_->RemoveUserData(kUserDataKey);
+      scheduler_->RemoveRequest(this);
+    }

[mmenke, 2016/02/25 18:09:51]

Fix indent (Or just run git cl format)

 
   static ScheduledResourceRequest* ForRequest(net::URLRequest* request) {
     return static_cast<UnownedPointer*>(request->GetUserData(kUserDataKey))
         ->get();
   }
 
-  // Starts the request. If |start_mode| is START_ASYNC, the request will not
-  // be started immediately.
-  void Start(StartMode start_mode) {
-    DCHECK(!ready_);
-
+  // Resumes the request. If |resume_mode| is RESUME_ASYNC, the request will not
+  // be resumed immediately.
+  void Resume(ResumeMode resume_mode) {
     // If the request was cancelled, do nothing.
     if (!request_->status().is_success())
       return;
 
-    // If the request was deferred, need to start it.  Otherwise, will just not
-    // defer starting it in the first place, and the value of |start_mode|
+    // If the request was paused, need to resume it.  Otherwise, will just not
+    // pause it in the first place, and the value of |resume_mode|
     // makes no difference.
-    if (deferred_) {
+    if (paused_) {
       // If can't start the request synchronously, post a task to start the
       // request.
-      if (start_mode == START_ASYNC) {
+      if (resume_mode == RESUME_ASYNC) {
         base::ThreadTaskRunnerHandle::Get()->PostTask(
             FROM_HERE,
-            base::Bind(&ScheduledResourceRequest::Start,
+            base::Bind(&ScheduledResourceRequest::Resume,
                        weak_ptr_factory_.GetWeakPtr(),
-                       START_SYNC));
+                       RESUME_SYNC));
         return;
       }
-      deferred_ = false;
+      paused_ = false;
       controller()->Resume();
     }
-
-    ready_ = true;
   }
 
   void set_request_priority_params(const RequestPriorityParams& priority) {
     priority_ = priority;
   }
   const RequestPriorityParams& get_request_priority_params() const {
     return priority_;
   }
   const ClientId& client_id() const { return client_id_; }
   net::URLRequest* url_request() { return request_; }
@@ skipping 20 matching lines @@
 
    private:
     ScheduledResourceRequest* const pointer_;
 
     DISALLOW_COPY_AND_ASSIGN(UnownedPointer);
   };
 
   static const void* const kUserDataKey;
 
   // ResourceThrottle interface:
-  void WillStartRequest(bool* defer) override {
-    deferred_ = *defer = !ready_;
+  void WillStartUsingNetwork(bool* defer) override {
+    paused_ = *defer = !scheduler_->ShouldSendRequest(this);
   }
 
   const char* GetNameForLogging() const override { return "ResourceScheduler"; }
 
   const ClientId client_id_;
   net::URLRequest* request_;
-  bool ready_;
-  bool deferred_;
+  bool paused_;
   bool is_async_;
   RequestAttributes attributes_;
   ResourceScheduler* scheduler_;
   RequestPriorityParams priority_;
   uint32_t fifo_ordering_;
 
   base::WeakPtrFactory<ResourceScheduler::ScheduledResourceRequest>
       weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ScheduledResourceRequest);
@@ skipping 31 matching lines @@
   explicit Client(ResourceScheduler* scheduler)
       : is_loaded_(false),
         has_html_body_(false),
         using_spdy_proxy_(false),
         scheduler_(scheduler),
         in_flight_delayable_count_(0),
         total_layout_blocking_count_(0) {}
 
   ~Client() {}
 
   void ScheduleRequest(net::URLRequest* url_request,

[mmenke, 2016/02/25 18:09:51]

SchedulerRequest -> AddRequest?  This is no longer doing any actual scheduling.

                        ScheduledResourceRequest* request) {
+    all_requests_.insert(request);
     SetRequestAttributes(request, DetermineRequestAttributes(request));
+  }
+
+  bool ShouldSendRequest(ScheduledResourceRequest* request) {

[mmenke, 2016/02/25 18:09:51]

Think this needs a description

[mmenke, 2016/02/25 18:09:51]

This should be renamed.  "Should" implies no side effects, and doing nothing but
returning a bool.

     if (ShouldStartRequest(request) == START_REQUEST) {
-      // New requests can be started synchronously without issue.
-      StartRequest(request, START_SYNC);
+      InsertInFlightRequest(request);
+      return true;
     } else {
       pending_requests_.Insert(request);

[mmenke, 2016/02/25 18:09:51]

pending_requests_ -> paused_requests_, maybe?

+      return false;
     }
   }
 
   void RemoveRequest(ScheduledResourceRequest* request) {
-    if (pending_requests_.IsQueued(request)) {
+    if (pending_requests_.IsQueued(request))
       pending_requests_.Erase(request);

[mmenke, 2016/02/25 18:09:51]

Why did you remove the DCHECK here?

-      DCHECK(!ContainsKey(in_flight_requests_, request));
-    } else {
+    else if (ContainsKey(in_flight_requests_, request))
       EraseInFlightRequest(request);

[mmenke, 2016/02/25 18:09:51]

nit:  Network stack team generally uses braces for all else-ifs.  Since we're
now the owns who now own this file, should probably stick with our style.

-
-      // Removing this request may have freed up another to load.
-      LoadAnyStartablePendingRequests();
-    }
+    SetRequestAttributes(request, kAttributeNone);
+    all_requests_.erase(request);

[mmenke, 2016/02/25 18:09:51]

We should update the list before calling SetRequestAttributes, like we do
elsewhere, since you remove the current_request parameter of
SetRequestAttributes.

This order happens not to cause the DCHECKs at the end of SetRequestAttributes
to trigger, but...

+    // Removing this request may have freed up another to load or may have
+    // cleared out the queue of layout-blocking requests.
+    LoadAnyStartablePendingRequests();
   }
 
   RequestSet StartAndRemoveAllRequests() {
     // First start any pending requests so that they will be moved into
     // in_flight_requests_. This may exceed the limits
     // kDefaultMaxNumDelayableRequestsPerClient and
     // kMaxNumDelayableRequestsPerHost, so this method must not do anything that
     // depends on those limits before calling ClearInFlightRequests() below.
     while (!pending_requests_.IsEmpty()) {
       ScheduledResourceRequest* request =
           *pending_requests_.GetNextHighestIterator();
-      pending_requests_.Erase(request);
-      // Starting requests asynchronously ensures no side effects, and avoids
-      // starting a bunch of requests that may be about to be deleted.
-      StartRequest(request, START_ASYNC);
+      // Resuming requests asynchronously ensures no side effects, and avoids
+      // resuming a bunch of requests that may be about to be deleted.
+      ResumeRequest(request, RESUME_ASYNC);
     }
     RequestSet unowned_requests;
     for (RequestSet::iterator it = in_flight_requests_.begin();
          it != in_flight_requests_.end(); ++it) {
       unowned_requests.insert(*it);
       (*it)->set_attributes(kAttributeNone);
     }
     ClearInFlightRequests();
     return unowned_requests;
   }
@@ skipping 21 matching lines @@
     }
   }
 
   void ReprioritizeRequest(ScheduledResourceRequest* request,
                            RequestPriorityParams old_priority_params,
                            RequestPriorityParams new_priority_params) {
     request->url_request()->SetPriority(new_priority_params.priority);
     request->set_request_priority_params(new_priority_params);
     SetRequestAttributes(request, DetermineRequestAttributes(request));
     if (!pending_requests_.IsQueued(request)) {
-      DCHECK(ContainsKey(in_flight_requests_, request));
-      // Request has already started.
+      // Request is not waiting to be sent.
       return;
     }
 
     pending_requests_.Erase(request);
     pending_requests_.Insert(request);
 
     if (new_priority_params.priority > old_priority_params.priority) {
       // Check if this request is now able to load at its new priority.
       LoadAnyStartablePendingRequests();
     }
@@ skipping 17 matching lines @@
     // Clear any special state that we were tracking for this request.
     SetRequestAttributes(request, kAttributeNone);
   }
 
   void ClearInFlightRequests() {
     in_flight_requests_.clear();
     in_flight_delayable_count_ = 0;
     total_layout_blocking_count_ = 0;
   }
 
-  size_t CountRequestsWithAttributes(
-      const RequestAttributes attributes,
-      ScheduledResourceRequest* current_request) {
+  size_t CountRequestsWithAttributes(const RequestAttributes attributes) {
     size_t matching_request_count = 0;
-    for (RequestSet::const_iterator it = in_flight_requests_.begin();
-         it != in_flight_requests_.end(); ++it) {
-      if (RequestAttributesAreSet((*it)->attributes(), attributes))
-        matching_request_count++;
-    }
-    if (!RequestAttributesAreSet(attributes, kAttributeInFlight)) {
-      bool current_request_is_pending = false;
-      for (RequestQueue::NetQueue::const_iterator
-           it = pending_requests_.GetNextHighestIterator();
-           it != pending_requests_.End(); ++it) {
+    if (RequestAttributesAreSet(attributes, kAttributeInFlight)) {
+      for (RequestSet::const_iterator it = in_flight_requests_.begin();
+           it != in_flight_requests_.end(); ++it) {

[mmenke, 2016/02/25 18:09:51]

Switch to range loops while you're here, on both of these loops?

         if (RequestAttributesAreSet((*it)->attributes(), attributes))
           matching_request_count++;
-        if (*it == current_request)
-          current_request_is_pending = true;
       }
-      // Account for the current request if it is not in one of the lists yet.
-      if (current_request &&
-          !ContainsKey(in_flight_requests_, current_request) &&
-          !current_request_is_pending) {
-        if (RequestAttributesAreSet(current_request->attributes(), attributes))
+    } else {
+      for (RequestSet::const_iterator it = all_requests_.begin();
+           it != all_requests_.end(); ++it) {
+        if (RequestAttributesAreSet((*it)->attributes(), attributes))
           matching_request_count++;
       }
     }
     return matching_request_count;
   }
 
   bool RequestAttributesAreSet(RequestAttributes request_attributes,
                                RequestAttributes matching_attributes) const {
     return (request_attributes & matching_attributes) == matching_attributes;
   }
@@ skipping 13 matching lines @@
 
     if (RequestAttributesAreSet(attributes,
                                 kAttributeInFlight | kAttributeDelayable)) {
       in_flight_delayable_count_++;
     }
     if (RequestAttributesAreSet(attributes, kAttributeLayoutBlocking))
       total_layout_blocking_count_++;
 
     request->set_attributes(attributes);
     DCHECK_EQ(CountRequestsWithAttributes(
-        kAttributeInFlight | kAttributeDelayable, request),
-        in_flight_delayable_count_);
-    DCHECK_EQ(CountRequestsWithAttributes(kAttributeLayoutBlocking, request),
+              kAttributeInFlight | kAttributeDelayable),
+              in_flight_delayable_count_);
+    DCHECK_EQ(CountRequestsWithAttributes(kAttributeLayoutBlocking),
               total_layout_blocking_count_);
   }
 
   RequestAttributes DetermineRequestAttributes(
       ScheduledResourceRequest* request) {
     RequestAttributes attributes = kAttributeNone;
 
     if (ContainsKey(in_flight_requests_, request))
       attributes |= kAttributeInFlight;
 
@@ skipping 33 matching lines @@
           net::HostPortPair::FromURL((*it)->url_request()->url());
       if (active_request_host.Equals(host_port_pair)) {
         same_host_count++;
         if (same_host_count >= kMaxNumDelayableRequestsPerHost)
           return true;
       }
     }
     return false;
   }
 
-  void StartRequest(ScheduledResourceRequest* request,
-                    StartMode start_mode) {
+  void ResumeRequest(ScheduledResourceRequest* request,
+                     ResumeMode resume_mode) {
+    DCHECK(pending_requests_.IsQueued(request));
+    pending_requests_.Erase(request);
     InsertInFlightRequest(request);
-    request->Start(start_mode);
+    request->Resume(resume_mode);
   }
 
   // ShouldStartRequest is the main scheduling algorithm.
   //
   // Requests are evaluated on five attributes:
   //
   // 1. Non-delayable requests:
   //   * Synchronous requests.
   //   * Non-HTTP[S] requests.
   //
@@ skipping 122 matching lines @@
     // 3) We do not start the request, same as above, but StartRequest() tells
     //     us there's no point in checking any further requests.
     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);
+        ResumeRequest(request, RESUME_ASYNC);
 
         // StartRequest can modify the pending list, so we (re)start evaluation
         // from the currently highest priority request. Avoid copying a singular
         // iterator, which would trigger undefined behavior.
         if (pending_requests_.GetNextHighestIterator() ==
             pending_requests_.End())
           break;
         request_iter = pending_requests_.GetNextHighestIterator();
       } else if (query_result == DO_NOT_START_REQUEST_AND_KEEP_SEARCHING) {
         ++request_iter;
         continue;
       } else {
         DCHECK(query_result == DO_NOT_START_REQUEST_AND_STOP_SEARCHING);
         break;
       }
     }
   }
 
   bool is_loaded_;
   // Tracks if the main HTML parser has reached the body which marks the end of
   // layout-blocking resources.
   bool has_html_body_;
   bool using_spdy_proxy_;
+  RequestSet all_requests_;

[mmenke, 2016/02/25 18:09:51]

Hrm...  I really don't like keeping around a third set of requests, instead of
the original two.  Can we just get rid of one of the old ones?  Could even get
rid of both, I suppose, though that may have performance implications.

   RequestQueue pending_requests_;
   RequestSet in_flight_requests_;
   ResourceScheduler* scheduler_;
   // 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_;
 };
 
 ResourceScheduler::ResourceScheduler()
@@ skipping 72 matching lines @@
       client_id, url_request, this,
       RequestPriorityParams(url_request->priority(), 0), is_async));
 
   ClientMap::iterator it = client_map_.find(client_id);
   if (it == client_map_.end()) {
     // There are several ways this could happen:
     // 1. <a ping> requests don't have a route_id.
     // 2. Most unittests don't send the IPCs needed to register Clients.
     // 3. The tab is closed while a RequestResource IPC is in flight.
     unowned_requests_.insert(request.get());
-    request->Start(START_SYNC);
     return std::move(request);
   }
 
   Client* client = it->second;
   client->ScheduleRequest(url_request, request.get());
   return std::move(request);
 }
 
+bool ResourceScheduler::ShouldSendRequest(ScheduledResourceRequest* request) {

[mmenke, 2016/02/25 18:09:51]

Per earlier comment, the number of side effects this has makes "Should*" a
confusing name for this method.

+  DCHECK(CalledOnValidThread());
+  ClientMap::iterator client_it = client_map_.find(request->client_id());
+  if (client_it == client_map_.end()) {

[mmenke, 2016/02/25 18:09:51]

Think this warrants a comment.

+    return true;
+  }

[mmenke, 2016/02/25 18:09:51]

nit:  Remove braces.

+
+  Client* client = client_it->second;
+  return client->ShouldSendRequest(request);
+}
+
 void ResourceScheduler::RemoveRequest(ScheduledResourceRequest* request) {
   DCHECK(CalledOnValidThread());
   if (ContainsKey(unowned_requests_, request)) {
     unowned_requests_.erase(request);
     return;
   }
 
   ClientMap::iterator client_it = client_map_.find(request->client_id());
   if (client_it == client_map_.end()) {
     return;
@@ skipping 141 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