Cache net::HostPortPair in ResourceScheduler::ScheduledResourceRequest

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 160 matching lines @@
                            bool is_async)
       : client_id_(client_id),
         request_(request),
         ready_(false),
         deferred_(false),
         is_async_(is_async),
         attributes_(kAttributeNone),
         scheduler_(scheduler),
         priority_(priority),
         fifo_ordering_(0),
+        host_port_pair_(net::HostPortPair::FromURL(request->url())),
         weak_ptr_factory_(this) {
     DCHECK(!request_->GetUserData(kUserDataKey));
     request_->SetUserData(kUserDataKey, new UnownedPointer(this));
   }
 
   ~ScheduledResourceRequest() override {
     request_->RemoveUserData(kUserDataKey);
     scheduler_->RemoveRequest(this);
   }
 
@@ skipping 45 matching lines @@
   uint32_t fifo_ordering() const { return fifo_ordering_; }
   void set_fifo_ordering(uint32_t fifo_ordering) {
     fifo_ordering_ = fifo_ordering;
   }
   RequestAttributes attributes() const {
     return attributes_;
   }
   void set_attributes(RequestAttributes attributes) {
     attributes_ = attributes;
   }
+  const net::HostPortPair host_port_pair() { return host_port_pair_; }

[mmenke, 2016/11/10 19:11:37]

return by reference?

[mmenke, 2016/11/10 19:15:26]

Also +1 const:

const net::HostPortPair& host_port_pair() const {

[Charlie Harrison, 2016/11/10 19:15:41]

Oops, typo. Done :)

[Charlie Harrison, 2016/11/10 19:17:10]

Done.

 
  private:
   class UnownedPointer : public base::SupportsUserData::Data {
    public:
     explicit UnownedPointer(ScheduledResourceRequest* pointer)
         : pointer_(pointer) {}
 
     ScheduledResourceRequest* get() const { return pointer_; }
 
    private:
@@ skipping 13 matching lines @@
 
   const ClientId client_id_;
   net::URLRequest* request_;
   bool ready_;
   bool deferred_;
   bool is_async_;
   RequestAttributes attributes_;
   ResourceScheduler* scheduler_;
   RequestPriorityParams priority_;
   uint32_t fifo_ordering_;
+  // Cached to excessive recomputation in ShouldKeepSearching.
+  const net::HostPortPair host_port_pair_;
 
   base::WeakPtrFactory<ResourceScheduler::ScheduledResourceRequest>
       weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ScheduledResourceRequest);
 };
 
 const void* const ResourceScheduler::ScheduledResourceRequest::kUserDataKey =
     &ResourceScheduler::ScheduledResourceRequest::kUserDataKey;
 
@@ skipping 250 matching lines @@
     }
 
     return attributes;
   }
 
   bool ShouldKeepSearching(
       const net::HostPortPair& active_request_host) const {
     size_t same_host_count = 0;
     for (RequestSet::const_iterator it = in_flight_requests_.begin();
          it != in_flight_requests_.end(); ++it) {
-      net::HostPortPair host_port_pair =
-          net::HostPortPair::FromURL((*it)->url_request()->url());
+      net::HostPortPair host_port_pair = (*it)->host_port_pair();

[mmenke, 2016/11/10 19:11:37]

Store as a const ref, or just inline?  Not sure how smart compilers are, but
best to play it safe.

[Charlie Harrison, 2016/11/10 19:15:41]

Done.

       if (active_request_host.Equals(host_port_pair)) {
         same_host_count++;
         if (same_host_count >= kMaxNumDelayableRequestsPerHostPerClient)
           return true;
       }
     }
     return false;
   }
 
   void StartRequest(ScheduledResourceRequest* request,
@@ skipping 42 matching lines @@
     // Syncronous requests could block the entire render, which could impact
     // user-observable Clients.
     if (!request->is_async())
       return START_REQUEST;
 
     // TODO(simonjam): This may end up causing disk contention. We should
     // experiment with throttling if that happens.
     if (!url_request.url().SchemeIsHTTPOrHTTPS())
       return START_REQUEST;
 
-    net::HostPortPair host_port_pair =
-        net::HostPortPair::FromURL(url_request.url());
+    net::HostPortPair host_port_pair = request->host_port_pair();

[Charlie Harrison, 2016/11/10 19:15:41]

I also made this const &.

 
     if (!priority_requests_delayable_) {
       if (using_spdy_proxy_ && url_request.url().SchemeIs(url::kHttpScheme))
         return START_REQUEST;
 
       url::SchemeHostPort scheme_host_port(url_request.url());
 
       net::HttpServerProperties& http_server_properties =
           *url_request.context()->http_server_properties();
 
@@ skipping 289 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