|
|
Created:
7 years, 9 months ago by James Simonsen Modified:
7 years, 9 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionLimit to only 10 image requests per page in ResourceScheduler.
The pending requests are now kept in a net::PriorityQueue. Priority changes
affect the order of the queue. After painting or network idle, we'll load up to
10 images simultaneously in PriorityQueue order.
This is a 2% improvement in Speed Index, 4% in first paint, and 5% in
DOMContentLoaded. There is a 0.5% regression in onload.
BUG=None
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190201
Patch Set 1 #
Total comments: 36
Patch Set 2 : Use custom priority queue #Patch Set 3 : Remove unnecessary friend line #Patch Set 4 : Move inner classes to .cc #
Total comments: 17
Patch Set 5 : Stop using high/low priority #
Total comments: 4
Patch Set 6 : Remove unnecessary includes #Patch Set 7 : Fix sign comparison #Patch Set 8 : Fix content exports #Patch Set 9 : Fix constructor #Patch Set 10 : Rebase #Patch Set 11 : Rebase #
Messages
Total messages: 30 (0 generated)
Will, please review this. Darin, please rubber stamp when he's done. It's purely a scheduling change, not an architectural change. Note that this CL is based off of https://codereview.chromium.org/12600018/. That will need to land first.
Sorry, I wasn't able to fit this in today due to IETF. Maybe I'll get to it before I board my plane, but I'm honestly feeling pretty sleepy. I'll get to it by Monday. On Thu, Mar 14, 2013 at 5:38 PM, <simonjam@chromium.org> wrote: > Reviewers: willchan, darin, > > Message: > Will, please review this. Darin, please rubber stamp when he's done. It's > purely > a scheduling change, not an architectural change. > > Note that this CL is based off of https://codereview.chromium.** > org/12600018/ <https://codereview.chromium.org/12600018/>. > That will need to land first. > > Description: > Limit to only 10 image requests per page in ResourceScheduler. > > The pending requests are now kept in a net::PriorityQueue. Priority changes > affect the order of the queue. After painting or network idle, we'll load > up to > 10 images simultaneously in PriorityQueue order. > > This is a 2% improvement in Speed Index, 4% in first paint, and 5% in > DOMContentLoaded. There is a 0.5% regression in onload. > > > BUG=None > > > Please review this at https://codereview.chromium.**org/12874003/<https://codereview.chromium.org/1... > > SVN Base: svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src> > > Affected files: > M content/browser/loader/**resource_request_info_impl.h > M content/browser/loader/**resource_scheduler.h > M content/browser/loader/**resource_scheduler.cc > M content/browser/loader/**resource_scheduler_unittest.cc > > >
I didn't get a chance to review the test yet. https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... content/browser/loader/resource_scheduler.cc:82: if (new_priority != old_priority) { When is this ever false? Sounds like a renderer bug if that ever happens. Should we DCHECK to catch this? https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... content/browser/loader/resource_scheduler.cc:165: // Removing this request may have freed up another to load. Isn't this only true if we remove an in flight request? Why would removing a pending request free up another to load? https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... content/browser/loader/resource_scheduler.cc:185: // cross-renderer navigation. How does this work? We move all the requests into unowned_requests_...so when does the new renderer take over ownership? What happens if it sends reprioritization IPCs for these requests? https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... content/browser/loader/resource_scheduler.cc:237: // The client was likely deleted shortly before we received this IPC. How? The comment for OnClientDeleted() says it happens when the renderer is destroyed. Where are we getting an IPC from if the renderer is destroyed? https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... content/browser/loader/resource_scheduler.cc:244: // Request has already started. But don't we need to call set_priority()? OH, I see now. You're already calling set_priority() before ReprioritizeRequest() is called. That's non-intuitive. Can we move the set_priority() call into this function? Is there a reason to keep it separate? https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... content/browser/loader/resource_scheduler.cc:248: client->pending_requests.Erase(request->queue_pointer()); When I see stuff like this where the caller needs to remember to set or clear the queue_pointer when inserting/erasing into/from the queue, I sorta feel like the RequestQueue shouldn't be a typedef of the net::PriorityQueue, but rather a class that composes a net::PriorityQueue. I leave it up to you, but that's what I would do. https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... content/browser/loader/resource_scheduler.cc:253: // Check if this request is now able to load at its new priority. We only want to do this if the new priority is higher than the old one, right? https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... content/browser/loader/resource_scheduler.cc:282: bool ResourceScheduler::ShouldStartRequest(ScheduledResourceRequest* request, Please add a comment here explaining in plain English what the algorithm is. https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... content/browser/loader/resource_scheduler.cc:284: bool is_synchronous = I know you're just moving this, but this caught my eye. LOAD_IGNORE_LIMITS smells like the wrong signal to use here. Should we be reading from the ResourceRequestInfoImpl instead? It explicitly says whether or not this is async. https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... content/browser/loader/resource_scheduler.cc:287: bool is_low_priority = Sorry again for nitting you when you're just moving stuff. This reads weird. I think we should delete this local variable and just do the if check directly. is_low_priority == priority < LOW && !is_synchronous is just...weird. Reads like..."the priority is low when the priority is less than low and it's not sync"? https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... content/browser/loader/resource_scheduler.cc:299: int num_high_priority_requests_in_flight = You don't need the number, you just need to know if there are any high priority requests in flight. I suggest changing this to: bool have_high_priority_requests_in_flight = client->in_flight_requests.size() > num_low_priority_requests_in_flight; https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... File content/browser/loader/resource_scheduler.h (right): https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... content/browser/loader/resource_scheduler.h:74: friend class ScheduledResourceRequest; In C++, inner classes are always friends of the outer class. This line is not necessary. https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... content/browser/loader/resource_scheduler.h:88: RequestSet in_flight_requests; In the future, we may want to consider revisiting this and using an array of RequestSets, indexed by priority. The reason I think about this is GetNumLowPriorityRequestsInFlight() could get expensive, since it might get called in a loop, since LoadPendingRequests() has a loop that calls ShouldStartRequest(), which calls GetNumLowPriorityRequestsInFlight(). This is potentially premature optimization. I'd leave a comment noting this possibility though. It might not be that unlikely. You should probably test out 123dj.com to see :) If you know how to run a profiler, you could see if this shows up in the CPU profile for the browser process. Otherwise it's a non-issue. https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... content/browser/loader/resource_scheduler.h:99: void ReprioritizeRequest(ScheduledResourceRequest* request); There's some undocumented behavior here, namely the position of in the priority queue. Whenever it moves, it is appended to the priority-specific queue. I'm trying to figure out if that always makes sense. If an image moves out of the viewport, should it be demoted and be at the end of the FIFO queue? In any case, if nothing else, we should document that reprioritizations always move the request to the end of the relevant queue. Also note that it might not be in a queue, in which case it just reprioritizes at the URLRequest level (when that's supported). https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... content/browser/loader/resource_scheduler.h:103: void LoadPendingRequests(Client* client); Nit: Can we get a better name? I leave it up to you, but when I read LoadPendingRequests(), I think that it'll load all of the pending requests, not "attempt" to load them. https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... content/browser/loader/resource_scheduler.h:107: int GetNumLowPriorityRequestsInFlight(Client* client); Looks like it should be const https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... content/browser/loader/resource_scheduler.h:111: bool ShouldStartRequest(ScheduledResourceRequest* request, Client* client); Looks like it should be const
https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... content/browser/loader/resource_scheduler.cc:82: if (new_priority != old_priority) { On 2013/03/18 07:30:33, willchan wrote: > When is this ever false? Sounds like a renderer bug if that ever happens. Should > we DCHECK to catch this? Agreed. Done. https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... content/browser/loader/resource_scheduler.cc:165: // Removing this request may have freed up another to load. On 2013/03/18 07:30:33, willchan wrote: > Isn't this only true if we remove an in flight request? Why would removing a > pending request free up another to load? Done. https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... content/browser/loader/resource_scheduler.cc:185: // cross-renderer navigation. On 2013/03/18 07:30:33, willchan wrote: > How does this work? We move all the requests into unowned_requests_...so when > does the new renderer take over ownership? What happens if it sends > reprioritization IPCs for these requests? It doesn't get picked up in the new renderer. I should fix that. I'll do it in another CL. If it's not found, the priority change is ignored (it's already in flight anyway, so it really doesn't matter). https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... content/browser/loader/resource_scheduler.cc:237: // The client was likely deleted shortly before we received this IPC. On 2013/03/18 07:30:33, willchan wrote: > How? The comment for OnClientDeleted() says it happens when the renderer is > destroyed. Where are we getting an IPC from if the renderer is destroyed? RenderViewHosts can be deleted by user action on the UI thread. As that's happening, the renderer might send us an IPC. Sometimes, we'll get the Task from the UI thread before we process the IPC. I've documented this in the header. https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... content/browser/loader/resource_scheduler.cc:244: // Request has already started. On 2013/03/18 07:30:33, willchan wrote: > But don't we need to call set_priority()? OH, I see now. You're already calling > set_priority() before ReprioritizeRequest() is called. That's non-intuitive. Can > we move the set_priority() call into this function? Is there a reason to keep it > separate? Done. https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... content/browser/loader/resource_scheduler.cc:248: client->pending_requests.Erase(request->queue_pointer()); On 2013/03/18 07:30:33, willchan wrote: > When I see stuff like this where the caller needs to remember to set or clear > the queue_pointer when inserting/erasing into/from the queue, I sorta feel like > the RequestQueue shouldn't be a typedef of the net::PriorityQueue, but rather a > class that composes a net::PriorityQueue. > > I leave it up to you, but that's what I would do. Done. https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... content/browser/loader/resource_scheduler.cc:253: // Check if this request is now able to load at its new priority. On 2013/03/18 07:30:33, willchan wrote: > We only want to do this if the new priority is higher than the old one, right? Done. https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... content/browser/loader/resource_scheduler.cc:282: bool ResourceScheduler::ShouldStartRequest(ScheduledResourceRequest* request, On 2013/03/18 07:30:33, willchan wrote: > Please add a comment here explaining in plain English what the algorithm is. Done. https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... content/browser/loader/resource_scheduler.cc:284: bool is_synchronous = On 2013/03/18 07:30:33, willchan wrote: > I know you're just moving this, but this caught my eye. LOAD_IGNORE_LIMITS > smells like the wrong signal to use here. Should we be reading from the > ResourceRequestInfoImpl instead? It explicitly says whether or not this is > async. Done. https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... content/browser/loader/resource_scheduler.cc:287: bool is_low_priority = On 2013/03/18 07:30:33, willchan wrote: > Sorry again for nitting you when you're just moving stuff. This reads weird. I > think we should delete this local variable and just do the if check directly. > is_low_priority == priority < LOW && !is_synchronous is just...weird. Reads > like..."the priority is low when the priority is less than low and it's not > sync"? Done. https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... content/browser/loader/resource_scheduler.cc:299: int num_high_priority_requests_in_flight = On 2013/03/18 07:30:33, willchan wrote: > You don't need the number, you just need to know if there are any high priority > requests in flight. I suggest changing this to: > bool have_high_priority_requests_in_flight = client->in_flight_requests.size() > > num_low_priority_requests_in_flight; Done. https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... File content/browser/loader/resource_scheduler.h (right): https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... content/browser/loader/resource_scheduler.h:74: friend class ScheduledResourceRequest; On 2013/03/18 07:30:33, willchan wrote: > In C++, inner classes are always friends of the outer class. This line is not > necessary. Done. https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... content/browser/loader/resource_scheduler.h:88: RequestSet in_flight_requests; On 2013/03/18 07:30:33, willchan wrote: > In the future, we may want to consider revisiting this and using an array of > RequestSets, indexed by priority. > > The reason I think about this is GetNumLowPriorityRequestsInFlight() could get > expensive, since it might get called in a loop, since LoadPendingRequests() has > a loop that calls ShouldStartRequest(), which calls > GetNumLowPriorityRequestsInFlight(). > > This is potentially premature optimization. I'd leave a comment noting this > possibility though. It might not be that unlikely. You should probably test out > http://123dj.com to see :) If you know how to run a profiler, you could see if this > shows up in the CPU profile for the browser process. Otherwise it's a non-issue. 0 samples. https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... content/browser/loader/resource_scheduler.h:99: void ReprioritizeRequest(ScheduledResourceRequest* request); On 2013/03/18 07:30:33, willchan wrote: > There's some undocumented behavior here, namely the position of in the priority > queue. Whenever it moves, it is appended to the priority-specific queue. I'm > trying to figure out if that always makes sense. If an image moves out of the > viewport, should it be demoted and be at the end of the FIFO queue? > > In any case, if nothing else, we should document that reprioritizations always > move the request to the end of the relevant queue. > > Also note that it might not be in a queue, in which case it just reprioritizes > at the URLRequest level (when that's supported). Done. I also thought about the always-at-end thing. It does make it harder for web developers to reason about. However, from our perspective, if one request is more important than the others, it should be a higher priority. IMO, we shouldn't concern ourselves too much with strict FIFO. https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... content/browser/loader/resource_scheduler.h:103: void LoadPendingRequests(Client* client); On 2013/03/18 07:30:33, willchan wrote: > Nit: Can we get a better name? I leave it up to you, but when I read > LoadPendingRequests(), I think that it'll load all of the pending requests, not > "attempt" to load them. Done. https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... content/browser/loader/resource_scheduler.h:107: int GetNumLowPriorityRequestsInFlight(Client* client); On 2013/03/18 07:30:33, willchan wrote: > Looks like it should be const Done. https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... content/browser/loader/resource_scheduler.h:111: bool ShouldStartRequest(ScheduledResourceRequest* request, Client* client); On 2013/03/18 07:30:33, willchan wrote: > Looks like it should be const Done.
https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... content/browser/loader/resource_scheduler.cc:185: // cross-renderer navigation. On 2013/03/18 23:50:13, James Simonsen wrote: > On 2013/03/18 07:30:33, willchan wrote: > > How does this work? We move all the requests into unowned_requests_...so when > > does the new renderer take over ownership? What happens if it sends > > reprioritization IPCs for these requests? > > It doesn't get picked up in the new renderer. I should fix that. I'll do it in > another CL. > > If it's not found, the priority change is ignored (it's already in flight > anyway, so it really doesn't matter). Do we want to ignore it in debug mode? Are there any valid cases for this to happen? Do we want to be able to DCHECK on this condition? https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... content/browser/loader/resource_scheduler.cc:237: // The client was likely deleted shortly before we received this IPC. On 2013/03/18 23:50:13, James Simonsen wrote: > On 2013/03/18 07:30:33, willchan wrote: > > How? The comment for OnClientDeleted() says it happens when the renderer is > > destroyed. Where are we getting an IPC from if the renderer is destroyed? > > RenderViewHosts can be deleted by user action on the UI thread. As that's > happening, the renderer might send us an IPC. Sometimes, we'll get the Task from > the UI thread before we process the IPC. > > I've documented this in the header. Why are we listening for the signal that the RenderViewHost is gone instead of the signal that the RenderView (the client) is actually gone? Why do we have to accept this race? https://codereview.chromium.org/12874003/diff/13001/content/browser/loader/re... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/12874003/diff/13001/content/browser/loader/re... content/browser/loader/resource_scheduler.cc:143: for (ClientMap::iterator it ALLOW_UNUSED = client_map_.begin(); Sorry again for nitting on existing code. Why does this loop exist? We have a DCHECK(client_map_.empty()) below. https://codereview.chromium.org/12874003/diff/13001/content/browser/loader/re... content/browser/loader/resource_scheduler.cc:276: DCHECK(new_priority != old_priority); Does DCHECK_NE() not work? https://codereview.chromium.org/12874003/diff/13001/content/browser/loader/re... content/browser/loader/resource_scheduler.cc:280: // The client was likely deleted shortly before we received this IPC. I think this RenderViewHost deletion signal seems wrong if it allows this. We should just track when the IPC channel is dead. If Darin's around tomorrow, I'm going to bug him about this. https://codereview.chromium.org/12874003/diff/13001/content/browser/loader/re... content/browser/loader/resource_scheduler.cc:326: // 1. High priority requests (>= net::LOW are always issued. This is very unfortunately named. Can we fix this somehow? See the above GetNumLowPriorityRequestsInFlight() which does < net::LOW. Hah! https://codereview.chromium.org/12874003/diff/13001/content/browser/loader/re... content/browser/loader/resource_scheduler.cc:327: // 2. Synchronous requests are always high priority. I'd say instead that synchronous requests are always issued. https://codereview.chromium.org/12874003/diff/13001/content/browser/loader/re... File content/browser/loader/resource_scheduler.h (right): https://codereview.chromium.org/12874003/diff/13001/content/browser/loader/re... content/browser/loader/resource_scheduler.h:102: // level. Add a test for this behavior. https://codereview.chromium.org/12874003/diff/13001/content/browser/loader/re... File content/browser/loader/resource_scheduler_unittest.cc (right): https://codereview.chromium.org/12874003/diff/13001/content/browser/loader/re... content/browser/loader/resource_scheduler_unittest.cc:85: class TestResourceContext : public ResourceContext { In the future, I suggest you follow the normal conventions here. See http://stackoverflow.com/questions/346372/whats-the-difference-between-faking.... This is a Fake. Same as the other stuff below. https://codereview.chromium.org/12874003/diff/13001/content/browser/loader/re... content/browser/loader/resource_scheduler_unittest.cc:161: int route_id = kRouteId) { You should ditch these default arguments since they're against the style guide. Instead I'd split this into two functions: NewRequestWithRoute() NewRequest() { NewRequestWithRoute(kRouteId); }
On 2013/03/19 20:53:52, willchan wrote: > https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... > File content/browser/loader/resource_scheduler.cc (right): > > https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resour... > content/browser/loader/resource_scheduler.cc:185: // cross-renderer navigation. > On 2013/03/18 23:50:13, James Simonsen wrote: > > On 2013/03/18 07:30:33, willchan wrote: > > > How does this work? We move all the requests into unowned_requests_...so > when > > > does the new renderer take over ownership? What happens if it sends > > > reprioritization IPCs for these requests? > > > > It doesn't get picked up in the new renderer. I should fix that. I'll do it in > > another CL. > > > > If it's not found, the priority change is ignored (it's already in flight > > anyway, so it really doesn't matter). > > Do we want to ignore it in debug mode? Are there any valid cases for this to > happen? Do we want to be able to DCHECK on this condition? I'll address that in the next CL where I clean this up.
https://codereview.chromium.org/12874003/diff/13001/content/browser/loader/re... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/12874003/diff/13001/content/browser/loader/re... content/browser/loader/resource_scheduler.cc:143: for (ClientMap::iterator it ALLOW_UNUSED = client_map_.begin(); On 2013/03/19 20:53:53, willchan wrote: > Sorry again for nitting on existing code. Why does this loop exist? We have a > DCHECK(client_map_.empty()) below. Done. It used to be more useful. https://codereview.chromium.org/12874003/diff/13001/content/browser/loader/re... content/browser/loader/resource_scheduler.cc:276: DCHECK(new_priority != old_priority); On 2013/03/19 20:53:53, willchan wrote: > Does DCHECK_NE() not work? Done. https://codereview.chromium.org/12874003/diff/13001/content/browser/loader/re... content/browser/loader/resource_scheduler.cc:280: // The client was likely deleted shortly before we received this IPC. On 2013/03/19 20:53:53, willchan wrote: > I think this RenderViewHost deletion signal seems wrong if it allows this. We > should just track when the IPC channel is dead. If Darin's around tomorrow, I'm > going to bug him about this. There can be several renderers on the same channel. Watching the channel isn't sufficient to know when the renderers have come and gone. The UI thread is the authority on that. https://codereview.chromium.org/12874003/diff/13001/content/browser/loader/re... content/browser/loader/resource_scheduler.cc:326: // 1. High priority requests (>= net::LOW are always issued. On 2013/03/19 20:53:53, willchan wrote: > This is very unfortunately named. Can we fix this somehow? See the above > GetNumLowPriorityRequestsInFlight() which does < net::LOW. Hah! The whole notion of using priority names is ridiculous. We should fix that instead. I've changed the wording here in the meantime. https://codereview.chromium.org/12874003/diff/13001/content/browser/loader/re... content/browser/loader/resource_scheduler.cc:327: // 2. Synchronous requests are always high priority. On 2013/03/19 20:53:53, willchan wrote: > I'd say instead that synchronous requests are always issued. Done. https://codereview.chromium.org/12874003/diff/13001/content/browser/loader/re... File content/browser/loader/resource_scheduler.h (right): https://codereview.chromium.org/12874003/diff/13001/content/browser/loader/re... content/browser/loader/resource_scheduler.h:102: // level. On 2013/03/19 20:53:53, willchan wrote: > Add a test for this behavior. Done. https://codereview.chromium.org/12874003/diff/13001/content/browser/loader/re... File content/browser/loader/resource_scheduler_unittest.cc (right): https://codereview.chromium.org/12874003/diff/13001/content/browser/loader/re... content/browser/loader/resource_scheduler_unittest.cc:85: class TestResourceContext : public ResourceContext { On 2013/03/19 20:53:53, willchan wrote: > In the future, I suggest you follow the normal conventions here. See > http://stackoverflow.com/questions/346372/whats-the-difference-between-faking.... > > This is a Fake. Same as the other stuff below. Done. https://codereview.chromium.org/12874003/diff/13001/content/browser/loader/re... content/browser/loader/resource_scheduler_unittest.cc:161: int route_id = kRouteId) { On 2013/03/19 20:53:53, willchan wrote: > You should ditch these default arguments since they're against the style guide. > Instead I'd split this into two functions: > NewRequestWithRoute() > NewRequest() { NewRequestWithRoute(kRouteId); } Done.
LGTM. I still don't like the RenderViewHost signal, but that's more Darin's call if anything. If that's what he recommends, then go for it. But as I say, it smells weird to me :) https://codereview.chromium.org/12874003/diff/13001/content/browser/loader/re... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/12874003/diff/13001/content/browser/loader/re... content/browser/loader/resource_scheduler.cc:280: // The client was likely deleted shortly before we received this IPC. On 2013/03/19 23:00:46, James Simonsen wrote: > On 2013/03/19 20:53:53, willchan wrote: > > I think this RenderViewHost deletion signal seems wrong if it allows this. We > > should just track when the IPC channel is dead. If Darin's around tomorrow, > I'm > > going to bug him about this. > > There can be several renderers on the same channel. Watching the channel isn't > sufficient to know when the renderers have come and gone. The UI thread is the > authority on that. Sorry, forgot about that. But why don't we get a RenderView shutdown notification? I assume we do. We should use that signal instead of RenderViewHost deletion. What I'm really saying is that this race is silly. Whenever we know for sure the peer (RenderView) is dead, then we know it's safe to delete the client. RenderViewHost deletion smells wrong, since that corresponds to a UI thread object. Frankly, the whole UI+IO thread usage of RVH smells weird to me. https://codereview.chromium.org/12874003/diff/18001/content/browser/loader/re... File content/browser/loader/resource_scheduler.h (right): https://codereview.chromium.org/12874003/diff/18001/content/browser/loader/re... content/browser/loader/resource_scheduler.h:13: #include "base/memory/linked_ptr.h" delete? https://codereview.chromium.org/12874003/diff/18001/content/browser/loader/re... content/browser/loader/resource_scheduler.h:17: #include "content/public/browser/global_request_id.h" delete (maybe you need to move to .cc?)?
On 2013/03/19 23:08:32, willchan wrote: > LGTM. I still don't like the RenderViewHost signal, but that's more Darin's call > if anything. If that's what he recommends, then go for it. But as I say, it > smells weird to me :) You can re-read the original code review for the scheduler. We tried other signals, but this was the best of the options. It's orthogonal to this CL, so I'm just going to land this. We can chat about it in person tomorrow if you want.
https://codereview.chromium.org/12874003/diff/18001/content/browser/loader/re... File content/browser/loader/resource_scheduler.h (right): https://codereview.chromium.org/12874003/diff/18001/content/browser/loader/re... content/browser/loader/resource_scheduler.h:13: #include "base/memory/linked_ptr.h" On 2013/03/19 23:08:32, willchan wrote: > delete? Done. https://codereview.chromium.org/12874003/diff/18001/content/browser/loader/re... content/browser/loader/resource_scheduler.h:17: #include "content/public/browser/global_request_id.h" On 2013/03/19 23:08:32, willchan wrote: > delete (maybe you need to move to .cc?)? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonjam@chromium.org/12874003/24017
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
To answer Will's question, note that sometimes we nuke the renderer process in which case we might not always receive IPCs from the renderer when it is going away. It might also crash. We could monitor the IPC connection to observe when it gets disconnected as well, but it seems pretty straightforward to just observe the lifetime of the RVH. Am I missing something about why that is yucky?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonjam@chromium.org/12874003/11005
The yuckiness is that RVH can die on the UI thread while IPCs may be forthcoming from the renderer process. This means in the ResourceScheduler we have to ignore IPC messages for non-existent "Client"s, since we may have just deleted the Client when the RVH went away. It is marginally desirable to have an invariant that we never receive IPCs for non-existent Clients, since then we can assert on it and catch bugs. It also makes the code more intuitive (we don't need comments explaining why we can receive IPC reprioritization messages for non-existent Clients. On 2013/03/20 06:54:23, darin wrote: > To answer Will's question, note that sometimes we nuke the renderer process in > which case we might not always receive IPCs from the renderer when it is going > away. It might also crash. We could monitor the IPC connection to observe when > it gets disconnected as well, but it seems pretty straightforward to just > observe the lifetime of the RVH. Am I missing something about why that is > yucky?
On Wed, Mar 20, 2013 at 3:23 PM, <willchan@chromium.org> wrote: > The yuckiness is that RVH can die on the UI thread while IPCs may be > forthcoming > from the renderer process. This means in the ResourceScheduler we have to > ignore > IPC messages for non-existent "Client"s, since we may have just deleted the > Client when the RVH went away. > > It is marginally desirable to have an invariant that we never receive IPCs > for > non-existent Clients, since then we can assert on it and catch bugs. It > also > makes the code more intuitive (we don't need comments explaining why we can > receive IPC reprioritization messages for non-existent Clients. The worst is when those incoming IPCs are resource requests. We actually start loading those, only to cancel them later, when there's nobody to send the response IPC to. James
Retried try job too often on linux_aura for step(s) views_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonjam@chromium.org/12874003/11005
Retried try job too often on win7_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonjam@chromium.org/12874003/11005
Failed to request the patch to try. Please note that binary files are still unsupported at the moment, this is being worked on. Thanks for your patience. HTTP Error 500: <html><head> <meta http-equiv="content-type" content="text/html;charset=utf-8"> <title>500 Server Error</title> </head> <body text=#000000 bgcolor=#ffffff> <h1>Error: Server Error</h1> <h2>The server encountered an error and could not complete your request.<p>If the problem persists, please <A HREF="http://code.google.com/appengine/community.html">report</A> your problem and mention this error message and the query that caused it.</h2> <h2></h2> </body></html>
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonjam@chromium.org/12874003/11005
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonjam@chromium.org/12874003/11005
Failed to apply patch for content/browser/loader/resource_scheduler.cc: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file content/browser/loader/resource_scheduler.cc Hunk #4 FAILED at 114. 1 out of 9 hunks FAILED -- saving rejects to file content/browser/loader/resource_scheduler.cc.rej Patch: 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 0a171ad06923cbadc9d9dad2704a5fc84c34fb8a..25758385dbe0d99a73c02d2af6e404985899fa5d 100644 --- a/content/browser/loader/resource_scheduler.cc +++ b/content/browser/loader/resource_scheduler.cc @@ -8,6 +8,7 @@ #include "content/common/resource_messages.h" #include "content/browser/loader/resource_message_delegate.h" #include "content/public/browser/resource_controller.h" +#include "content/public/browser/resource_request_info.h" #include "content/public/browser/resource_throttle.h" #include "ipc/ipc_message_macros.h" #include "net/base/load_flags.h" @@ -16,6 +17,54 @@ namespace content { +static const size_t kMaxNumDelayableRequestsPerClient = 10; + +// A thin wrapper around net::PriorityQueue that deals with +// ScheduledResourceRequests instead of PriorityQueue::Pointers. +class ResourceScheduler::RequestQueue { + public: + RequestQueue() : queue_(net::NUM_PRIORITIES) {} + ~RequestQueue() {} + + // Adds |request| to the queue with given |priority|. + void Insert(ScheduledResourceRequest* request, + net::RequestPriority priority) { + DCHECK(!ContainsKey(pointers_, request)); + NetQueue::Pointer pointer = queue_.Insert(request, priority); + pointers_[request] = pointer; + } + + // Removes |request| from the queue. + void Erase(ScheduledResourceRequest* request) { + PointerMap::iterator it = pointers_.find(request); + DCHECK(it != pointers_.end()); + queue_.Erase(it->second); + pointers_.erase(it); + } + + // Returns the highest priority request that's queued, or NULL if none are. + ScheduledResourceRequest* FirstMax() { + return queue_.FirstMax().value(); + } + + // Returns true if |request| is queued. + bool IsQueued(ScheduledResourceRequest* request) const { + return ContainsKey(pointers_, request); + } + + // Returns true if no requests are queued. + bool IsEmpty() const { return queue_.size() == 0; } + + private: + typedef net::PriorityQueue<ScheduledResourceRequest*> NetQueue; + typedef std::map<ScheduledResourceRequest*, NetQueue::Pointer> PointerMap; + + NetQueue queue_; + PointerMap pointers_; +}; + +// This is the handle we return to the ResourceDispatcherHostImpl so it can +// interact with the request. class ResourceScheduler::ScheduledResourceRequest : public ResourceMessageDelegate, public ResourceThrottle { @@ -44,7 +93,8 @@ class ResourceScheduler::ScheduledResourceRequest } const ClientId& client_id() const { return client_id_; } - const net::URLRequest& url_request() const { return *request_; } + net::URLRequest* url_request() { return request_; } + const net::URLRequest* url_request() const { return request_; } private: // ResourceMessageDelegate interface: @@ -64,11 +114,7 @@ class ResourceScheduler::ScheduledResourceRequest } void DidChangePriority(int request_id, net::RequestPriority new_priority) { - net::RequestPriority old_priority = request_->priority(); - request_->set_priority(new_priority); - if (new_priority > old_priority) { - Start(); - } + scheduler_->ReprioritizeRequest(this, new_priority); } ClientId client_id_; @@ -80,15 +126,20 @@ class ResourceScheduler::ScheduledResourceRequest DISALLOW_COPY_AND_ASSIGN(ScheduledResourceRequest); }; +// Each client represents a tab. +struct ResourceScheduler::Client { + Client() : has_body(false) {} + ~Client() {} + + bool has_body; + RequestQueue pending_requests; + RequestSet in_flight_requests; +}; + ResourceScheduler::ResourceScheduler() { } ResourceScheduler::~ResourceScheduler() { - for (ClientMap::iterator it ALLOW_UNUSED = client_map_.begin(); - it != client_map_.end(); ++it) { - DCHECK(it->second->pending_requests.empty()); - DCHECK(it->second->in_flight_requests.empty()); - } DCHECK(unowned_requests_.empty()); DCHECK(client_map_.empty()); } @@ -114,17 +165,10 @@ scoped_ptr<ResourceThrottle> ResourceScheduler::ScheduleRequest( } Client* client = it->second; - - bool is_synchronous = (url_request->load_flags() & net::LOAD_IGNORE_LIMITS) == - net::LOAD_IGNORE_LIMITS; - bool is_low_priority = - url_request->priority() < net::LOW && !is_synchronous; - - if (is_low_priority && !client->in_flight_requests.empty() && - !client->has_body) { - client->pending_requests.push_back(request.get()); - } else { + if (ShouldStartRequest(request.get(), client)) { StartRequest(request.get(), client); + } else { + client->pending_requests.Insert(request.get(), url_request->priority()); } return request.PassAs<ResourceThrottle>(); } @@ -142,29 +186,16 @@ void ResourceScheduler::RemoveRequest(ScheduledResourceRequest* request) { } Client* client = client_it->second; - RequestSet::iterator request_it = client->in_flight_requests.find(request); - if (request_it == client->in_flight_requests.end()) { - bool removed = false; - RequestQueue::iterator queue_it; - for (queue_it = client->pending_requests.begin(); - queue_it != client->pending_requests.end(); ++queue_it) { - if (*queue_it == request) { - client->pending_requests.erase(queue_it); - removed = true; - break; - } - } - DCHECK(removed); + + if (client->pending_requests.IsQueued(request)) { + client->pending_requests.Erase(request); DCHECK(!ContainsKey(client->in_flight_requests, request)); } else { size_t erased = client->in_flight_requests.erase(request); DCHECK(erased); - } - if (client->in_flight_requests.empty()) { - // Since the network is now idle, we may as well load some of the low - // priority requests. - LoadPendingRequests(client); + // Removing this request may have freed up another to load. + LoadAnyStartablePendingRequests(client); } } @@ -224,7 +255,7 @@ void ResourceScheduler::OnWillInsertBody(int child_id, int route_id) { client->has_body = false; if (!client->has_body) { client->has_body = true; - LoadPendingRequests(client); + LoadAnyStartablePendingRequests(client); } } @@ -234,24 +265,97 @@ void ResourceScheduler::StartRequest(ScheduledResourceRequest* request, request->Start(); } -void ResourceScheduler::LoadPendingRequests(Client* client) { - while (!client->pending_requests.empty()) { - ScheduledResourceRequest* request = client->pending_requests.front(); - client->pending_requests.erase(client->pending_requests.begin()); - StartRequest(request, client); +void ResourceScheduler::ReprioritizeRequest(ScheduledResourceRequest* request, + net::RequestPriority new_priority) { + net::RequestPriority old_priority = request->url_request()->priority(); + DCHECK_NE(new_priority, old_priority); + request->url_request()->set_priority(new_priority); + ClientMap::iterator client_it = client_map_.find(request->client_id()); + if (client_it == client_map_.end()) { + // The client was likely deleted shortly before we received this IPC. + return; + } + + Client *client = client_it->second; + if (!client->pending_requests.IsQueued(request)) { + DCHECK(ContainsKey(client->in_flight_requests, request)); + // Request has already started. + return; + } + + client->pending_requests.Erase(request); + client->pending_requests.Insert(request, request->url_request()->priority()); + + if (new_priority > old_priority) { + // Check if this request is now able to load at its new priority. + LoadAnyStartablePendingRequests(client); } } -ResourceScheduler::ClientId ResourceScheduler::MakeClientId( - int child_id, int route_id) { - return (static_cast<ResourceScheduler::ClientId>(child_id) << 32) | route_id; +void ResourceScheduler::LoadAnyStartablePendingRequests(Client* client) { + while (!client->pending_requests.IsEmpty()) { + ScheduledResourceRequest* request = client->pending_requests.FirstMax(); + if (ShouldStartRequest(request, client)) { + client->pending_requests.Erase(request); + StartRequest(request, client); + } else { + break; + } + } +} + +size_t ResourceScheduler::GetNumDelayableRequestsInFlight( + Client* client) const { + size_t count = 0; + for (RequestSet::iterator it = client->in_flight_requests.begin(); + it != client->in_flight_requests.end(); ++it) { + if ((*it)->url_request()->priority() < net::LOW) { + ++count; + } + } + return count; } -ResourceScheduler::Client::Client() - : has_body(false) { +// ShouldStartRequest is the main scheduling algorithm. +// +// Requests are categorized into two categories: +// +// 1. Immediately issued requests, which are: +// +// * Higher priority requests (>= net::LOW). +// * Synchronous requests. +// +// 2. The remainder are delayable requests, which follow these rules: +// +// * If no high priority requests are in flight, start loading low priority +// requests. +// * Once the renderer has a <body>, start loading delayable requests. +// * Never exceed 10 delayable requests in flight per client. +bool ResourceScheduler::ShouldStartRequest(ScheduledResourceRequest* request, + Client* client) const { + if (request->url_request()->priority() >= net::LOW || + !ResourceRequestInfo::ForRequest(request->url_request())->IsAsync()) { + return true; + } + + size_t num_delayable_requests_in_flight = + GetNumDelayableRequests… (message too large)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonjam@chromium.org/12874003/68001
Failed to apply patch for content/browser/loader/resource_scheduler.cc: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file content/browser/loader/resource_scheduler.cc Hunk #4 FAILED at 114. 1 out of 9 hunks FAILED -- saving rejects to file content/browser/loader/resource_scheduler.cc.rej Patch: 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 0a171ad06923cbadc9d9dad2704a5fc84c34fb8a..25758385dbe0d99a73c02d2af6e404985899fa5d 100644 --- a/content/browser/loader/resource_scheduler.cc +++ b/content/browser/loader/resource_scheduler.cc @@ -8,6 +8,7 @@ #include "content/common/resource_messages.h" #include "content/browser/loader/resource_message_delegate.h" #include "content/public/browser/resource_controller.h" +#include "content/public/browser/resource_request_info.h" #include "content/public/browser/resource_throttle.h" #include "ipc/ipc_message_macros.h" #include "net/base/load_flags.h" @@ -16,6 +17,54 @@ namespace content { +static const size_t kMaxNumDelayableRequestsPerClient = 10; + +// A thin wrapper around net::PriorityQueue that deals with +// ScheduledResourceRequests instead of PriorityQueue::Pointers. +class ResourceScheduler::RequestQueue { + public: + RequestQueue() : queue_(net::NUM_PRIORITIES) {} + ~RequestQueue() {} + + // Adds |request| to the queue with given |priority|. + void Insert(ScheduledResourceRequest* request, + net::RequestPriority priority) { + DCHECK(!ContainsKey(pointers_, request)); + NetQueue::Pointer pointer = queue_.Insert(request, priority); + pointers_[request] = pointer; + } + + // Removes |request| from the queue. + void Erase(ScheduledResourceRequest* request) { + PointerMap::iterator it = pointers_.find(request); + DCHECK(it != pointers_.end()); + queue_.Erase(it->second); + pointers_.erase(it); + } + + // Returns the highest priority request that's queued, or NULL if none are. + ScheduledResourceRequest* FirstMax() { + return queue_.FirstMax().value(); + } + + // Returns true if |request| is queued. + bool IsQueued(ScheduledResourceRequest* request) const { + return ContainsKey(pointers_, request); + } + + // Returns true if no requests are queued. + bool IsEmpty() const { return queue_.size() == 0; } + + private: + typedef net::PriorityQueue<ScheduledResourceRequest*> NetQueue; + typedef std::map<ScheduledResourceRequest*, NetQueue::Pointer> PointerMap; + + NetQueue queue_; + PointerMap pointers_; +}; + +// This is the handle we return to the ResourceDispatcherHostImpl so it can +// interact with the request. class ResourceScheduler::ScheduledResourceRequest : public ResourceMessageDelegate, public ResourceThrottle { @@ -44,7 +93,8 @@ class ResourceScheduler::ScheduledResourceRequest } const ClientId& client_id() const { return client_id_; } - const net::URLRequest& url_request() const { return *request_; } + net::URLRequest* url_request() { return request_; } + const net::URLRequest* url_request() const { return request_; } private: // ResourceMessageDelegate interface: @@ -64,11 +114,7 @@ class ResourceScheduler::ScheduledResourceRequest } void DidChangePriority(int request_id, net::RequestPriority new_priority) { - net::RequestPriority old_priority = request_->priority(); - request_->set_priority(new_priority); - if (new_priority > old_priority) { - Start(); - } + scheduler_->ReprioritizeRequest(this, new_priority); } ClientId client_id_; @@ -80,15 +126,20 @@ class ResourceScheduler::ScheduledResourceRequest DISALLOW_COPY_AND_ASSIGN(ScheduledResourceRequest); }; +// Each client represents a tab. +struct ResourceScheduler::Client { + Client() : has_body(false) {} + ~Client() {} + + bool has_body; + RequestQueue pending_requests; + RequestSet in_flight_requests; +}; + ResourceScheduler::ResourceScheduler() { } ResourceScheduler::~ResourceScheduler() { - for (ClientMap::iterator it ALLOW_UNUSED = client_map_.begin(); - it != client_map_.end(); ++it) { - DCHECK(it->second->pending_requests.empty()); - DCHECK(it->second->in_flight_requests.empty()); - } DCHECK(unowned_requests_.empty()); DCHECK(client_map_.empty()); } @@ -114,17 +165,10 @@ scoped_ptr<ResourceThrottle> ResourceScheduler::ScheduleRequest( } Client* client = it->second; - - bool is_synchronous = (url_request->load_flags() & net::LOAD_IGNORE_LIMITS) == - net::LOAD_IGNORE_LIMITS; - bool is_low_priority = - url_request->priority() < net::LOW && !is_synchronous; - - if (is_low_priority && !client->in_flight_requests.empty() && - !client->has_body) { - client->pending_requests.push_back(request.get()); - } else { + if (ShouldStartRequest(request.get(), client)) { StartRequest(request.get(), client); + } else { + client->pending_requests.Insert(request.get(), url_request->priority()); } return request.PassAs<ResourceThrottle>(); } @@ -142,29 +186,16 @@ void ResourceScheduler::RemoveRequest(ScheduledResourceRequest* request) { } Client* client = client_it->second; - RequestSet::iterator request_it = client->in_flight_requests.find(request); - if (request_it == client->in_flight_requests.end()) { - bool removed = false; - RequestQueue::iterator queue_it; - for (queue_it = client->pending_requests.begin(); - queue_it != client->pending_requests.end(); ++queue_it) { - if (*queue_it == request) { - client->pending_requests.erase(queue_it); - removed = true; - break; - } - } - DCHECK(removed); + + if (client->pending_requests.IsQueued(request)) { + client->pending_requests.Erase(request); DCHECK(!ContainsKey(client->in_flight_requests, request)); } else { size_t erased = client->in_flight_requests.erase(request); DCHECK(erased); - } - if (client->in_flight_requests.empty()) { - // Since the network is now idle, we may as well load some of the low - // priority requests. - LoadPendingRequests(client); + // Removing this request may have freed up another to load. + LoadAnyStartablePendingRequests(client); } } @@ -224,7 +255,7 @@ void ResourceScheduler::OnWillInsertBody(int child_id, int route_id) { client->has_body = false; if (!client->has_body) { client->has_body = true; - LoadPendingRequests(client); + LoadAnyStartablePendingRequests(client); } } @@ -234,24 +265,97 @@ void ResourceScheduler::StartRequest(ScheduledResourceRequest* request, request->Start(); } -void ResourceScheduler::LoadPendingRequests(Client* client) { - while (!client->pending_requests.empty()) { - ScheduledResourceRequest* request = client->pending_requests.front(); - client->pending_requests.erase(client->pending_requests.begin()); - StartRequest(request, client); +void ResourceScheduler::ReprioritizeRequest(ScheduledResourceRequest* request, + net::RequestPriority new_priority) { + net::RequestPriority old_priority = request->url_request()->priority(); + DCHECK_NE(new_priority, old_priority); + request->url_request()->set_priority(new_priority); + ClientMap::iterator client_it = client_map_.find(request->client_id()); + if (client_it == client_map_.end()) { + // The client was likely deleted shortly before we received this IPC. + return; + } + + Client *client = client_it->second; + if (!client->pending_requests.IsQueued(request)) { + DCHECK(ContainsKey(client->in_flight_requests, request)); + // Request has already started. + return; + } + + client->pending_requests.Erase(request); + client->pending_requests.Insert(request, request->url_request()->priority()); + + if (new_priority > old_priority) { + // Check if this request is now able to load at its new priority. + LoadAnyStartablePendingRequests(client); } } -ResourceScheduler::ClientId ResourceScheduler::MakeClientId( - int child_id, int route_id) { - return (static_cast<ResourceScheduler::ClientId>(child_id) << 32) | route_id; +void ResourceScheduler::LoadAnyStartablePendingRequests(Client* client) { + while (!client->pending_requests.IsEmpty()) { + ScheduledResourceRequest* request = client->pending_requests.FirstMax(); + if (ShouldStartRequest(request, client)) { + client->pending_requests.Erase(request); + StartRequest(request, client); + } else { + break; + } + } +} + +size_t ResourceScheduler::GetNumDelayableRequestsInFlight( + Client* client) const { + size_t count = 0; + for (RequestSet::iterator it = client->in_flight_requests.begin(); + it != client->in_flight_requests.end(); ++it) { + if ((*it)->url_request()->priority() < net::LOW) { + ++count; + } + } + return count; } -ResourceScheduler::Client::Client() - : has_body(false) { +// ShouldStartRequest is the main scheduling algorithm. +// +// Requests are categorized into two categories: +// +// 1. Immediately issued requests, which are: +// +// * Higher priority requests (>= net::LOW). +// * Synchronous requests. +// +// 2. The remainder are delayable requests, which follow these rules: +// +// * If no high priority requests are in flight, start loading low priority +// requests. +// * Once the renderer has a <body>, start loading delayable requests. +// * Never exceed 10 delayable requests in flight per client. +bool ResourceScheduler::ShouldStartRequest(ScheduledResourceRequest* request, + Client* client) const { + if (request->url_request()->priority() >= net::LOW || + !ResourceRequestInfo::ForRequest(request->url_request())->IsAsync()) { + return true; + } + + size_t num_delayable_requests_in_flight = + GetNumDelayableRequests… (message too large)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonjam@chromium.org/12874003/71001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonjam@chromium.org/12874003/71001
Message was sent while issue was closed.
Change committed as 190201
Took long enough :) On Mar 23, 2013 9:58 PM, <commit-bot@chromium.org> wrote: > Change committed as 190201 > > https://chromiumcodereview.**appspot.com/12874003/<https://chromiumcodereview... > |