content/browser/loader/resource_scheduler.h - Issue 12874003: Limit to only 10 image requests per page in ResourceScheduler.

Side by Side Diff: content/browser/loader/resource_scheduler.h

Issue 12874003: Limit to only 10 image requests per page in ResourceScheduler. (Closed) Base URL: svn://svn.chromium.org/chrome/trunk/src

Patch Set: Created 7 years, 9 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch | Annotate | Revision Log

« no previous file with comments | « content/browser/loader/resource_request_info_impl.h ('k') | content/browser/loader/resource_scheduler.cc » ('j') | content/browser/loader/resource_scheduler.cc » ('J')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
1 // Copyright (c) 2012 The Chromium Authors. All rights reserved.	1 // Copyright (c) 2012 The Chromium Authors. All rights reserved.

2 // Use of this source code is governed by a BSD-style license that can be	2 // Use of this source code is governed by a BSD-style license that can be

3 // found in the LICENSE file.	3 // found in the LICENSE file.

4	4

5 #ifndef CONTENT_BROWSER_LOADER_RESOURCE_SCHEDULER_H_	5 #ifndef CONTENT_BROWSER_LOADER_RESOURCE_SCHEDULER_H_

6 #define CONTENT_BROWSER_LOADER_RESOURCE_SCHEDULER_H_	6 #define CONTENT_BROWSER_LOADER_RESOURCE_SCHEDULER_H_

7	7

8 #include <map>	8 #include <map>

9 #include <set>	9 #include <set>

10 #include <vector>

11	10

12 #include "base/basictypes.h"	11 #include "base/basictypes.h"

13 #include "base/compiler_specific.h"	12 #include "base/compiler_specific.h"

14 #include "base/memory/linked_ptr.h"	13 #include "base/memory/linked_ptr.h"

15 #include "base/memory/scoped_ptr.h"	14 #include "base/memory/scoped_ptr.h"

16 #include "base/threading/non_thread_safe.h"	15 #include "base/threading/non_thread_safe.h"

17 #include "content/common/content_export.h"	16 #include "content/common/content_export.h"

18 #include "content/public/browser/global_request_id.h"	17 #include "content/public/browser/global_request_id.h"

	18 #include "net/base/priority_queue.h"

19	19

20 namespace net {	20 namespace net {

21 class URLRequest;	21 class URLRequest;

22 }	22 }

23	23

24 namespace content {	24 namespace content {

25 class ResourceThrottle;	25 class ResourceThrottle;

26	26

27 // There is one ResourceScheduler. All renderer-initiated HTTP requests are	27 // There is one ResourceScheduler. All renderer-initiated HTTP requests are

28 // expected to pass through it.	28 // expected to pass through it.

(...skipping 35 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
64	64

65 // Called when a client navigates to a new main document.	65 // Called when a client navigates to a new main document.

66 void OnNavigate(int child_id, int route_id);	66 void OnNavigate(int child_id, int route_id);

67	67

68 // Called when the client has parsed the <body> element. This is a signal that	68 // Called when the client has parsed the <body> element. This is a signal that

69 // resource loads won't interfere with first paint.	69 // resource loads won't interfere with first paint.

70 void OnWillInsertBody(int child_id, int route_id);	70 void OnWillInsertBody(int child_id, int route_id);

71	71

72 private:	72 private:

73 class ScheduledResourceRequest;	73 class ScheduledResourceRequest;

74 friend class ScheduledResourceRequest;	74 friend class ScheduledResourceRequest;
	willchan no longer on Chromium 2013/03/18 07:30:33 In C++, inner classes are always friends of the ou In C++, inner classes are always friends of the outer class. This line is not necessary. James Simonsen 2013/03/18 23:50:13 Done. Show quoted text 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.
75 struct Client;	75 struct Client;

76	76

77 typedef int64 ClientId;	77 typedef int64 ClientId;

78 typedef std::map<ClientId, Client*> ClientMap;	78 typedef std::map<ClientId, Client*> ClientMap;

79 typedef std::vector<ScheduledResourceRequest*> RequestQueue;	79 typedef net::PriorityQueue<ScheduledResourceRequest*> RequestQueue;

80 typedef std::set<ScheduledResourceRequest*> RequestSet;	80 typedef std::set<ScheduledResourceRequest*> RequestSet;

81	81

82 struct Client {	82 struct Client {

83 Client();	83 Client();

84 ~Client();	84 ~Client();

85	85

86 bool has_body;	86 bool has_body;

87 RequestQueue pending_requests;	87 RequestQueue pending_requests;

88 RequestSet in_flight_requests;	88 RequestSet in_flight_requests;
	willchan no longer on Chromium 2013/03/18 07:30:33 In the future, we may want to consider revisiting 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. James Simonsen 2013/03/18 23:50:13 0 samples. Show quoted text 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.
89 };	89 };

90	90

91 // Called when a ScheduledResourceRequest is destroyed.	91 // Called when a ScheduledResourceRequest is destroyed.

92 void RemoveRequest(ScheduledResourceRequest* request);	92 void RemoveRequest(ScheduledResourceRequest* request);

93	93

94 // Unthrottles the \|request\| and adds it to \|client\|.	94 // Unthrottles the \|request\| and adds it to \|client\|.

95 void StartRequest(ScheduledResourceRequest* request, Client* client);	95 void StartRequest(ScheduledResourceRequest* request, Client* client);

96	96

97 // Calls StartRequest on all pending requests for \|client\|.	97 // Update the queue position for \|request\|, possibly causing it to start

	98 // loading.

	99 void ReprioritizeRequest(ScheduledResourceRequest* request);
	willchan no longer on Chromium 2013/03/18 07:30:33 There's some undocumented behavior here, namely th 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). James Simonsen 2013/03/18 23:50:13 Done. I also thought about the always-at-end thin Show quoted text 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.
	100

	101 // Attempts to load any pending requests in \|client\|, based on the

	102 // ShouldStartRequest().

98 void LoadPendingRequests(Client* client);	103 void LoadPendingRequests(Client* client);
	willchan no longer on Chromium 2013/03/18 07:30:33 Nit: Can we get a better name? I leave it up to yo 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. James Simonsen 2013/03/18 23:50:13 Done. Show quoted text 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.
99	104

	105 // Returns the number of requests with priority < LOW that are currently in

	106 // flight.

	107 int GetNumLowPriorityRequestsInFlight(Client* client);
	willchan no longer on Chromium 2013/03/18 07:30:33 Looks like it should be const Looks like it should be const James Simonsen 2013/03/18 23:50:13 Done. Show quoted text On 2013/03/18 07:30:33, willchan wrote: > Looks like it should be const Done.
	108

	109 // Returns true if the request should start. This is the core scheduling

	110 // algorithm.

	111 bool ShouldStartRequest(ScheduledResourceRequest* request, Client* client);
	willchan no longer on Chromium 2013/03/18 07:30:33 Looks like it should be const Looks like it should be const James Simonsen 2013/03/18 23:50:13 Done. Show quoted text On 2013/03/18 07:30:33, willchan wrote: > Looks like it should be const Done.
	112

100 // Returns the client ID for the given \|child_id\| and \|route_id\| combo.	113 // Returns the client ID for the given \|child_id\| and \|route_id\| combo.

101 ClientId MakeClientId(int child_id, int route_id);	114 ClientId MakeClientId(int child_id, int route_id);

102	115

103 ClientMap client_map_;	116 ClientMap client_map_;

104 RequestSet unowned_requests_;	117 RequestSet unowned_requests_;

105 };	118 };

106	119

107 } // namespace content	120 } // namespace content

108	121

109 #endif // CONTENT_BROWSER_LOADER_RESOURCE_SCHEDULER_H_	122 #endif // CONTENT_BROWSER_LOADER_RESOURCE_SCHEDULER_H_

OLD	NEW