Add a constraint on how many requests can be outstanding for any given render (browser-side).

Add a constraint on how many requests can be outstanding for any given render process (browser-side).

Once the constraint is reached, subsequent requests will fail with net::ERR_INSUFFICIENT_RESOURCES.

The bound is defined as "25 MB", which represents the amount of private bytes we expect  the pending requests to consume in the browser. This number translates into around 6000 typical requests.

Note that the upload data of a request is not currently considered part of the request's in-memory cost -- more data is needed on the average/maximum upload sizes of users before deciding what a compatible limit is.

BUG=5688
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=9298

[eroman, 2009-01-23 02:16:11]

http://codereview.chromium.org/18541/diff/1/6
File chrome/browser/renderer_host/resource_dispatcher_host_unittest.cc (right):

http://codereview.chromium.org/18541/diff/1/6#newcode197
Line 197: EXPECT_EQ(0, host_.GetOutstandingRequestsCount(0));
The reason for sprinkling all these GetOutstandingRequestCount() assertions in
the existing test cases, is to make sure the count remains balaned in different
cancellation / blocked / pending scenarios.

The "reaching the threshold" policy itself is tested by
TooManyOutstandingRequests().

http://codereview.chromium.org/18541/diff/1/4
File net/base/net_error_list.h (right):

http://codereview.chromium.org/18541/diff/1/4#newcode231
Line 231: NET_ERROR(TOO_MANY_OUTSTANDING_REQUESTS, -401)
I am not thrilled about this -- it seems awkward to have a
ResourceHostDispatcher specific constant living in the net module.

Another solution is to reserve a range of error codes that can be defined
outside of net, and then define this error in ResourceDispatcherHost.

What do you think?

http://codereview.chromium.org/18541/diff/1/2
File net/url_request/url_request_test_job.cc (left):

http://codereview.chromium.org/18541/diff/1/2#oldcode132
Line 132: // Note that this state will still cause a NotifyDone to get called
This comment wasn't actually true :-)

[darin (slow to review), 2009-01-23 16:43:13]

http://codereview.chromium.org/18541/diff/1/7
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/18541/diff/1/7#newcode74
Line 74: static const int kMaxOutstandingRequestsPerProcess = 100;
100 seems too small.  i could easily imagine normal web pages generating this
many requests.

http://codereview.chromium.org/18541/diff/1/7#newcode581
Line 581: return kMaxOutstandingRequestsPerProcess;
can this just be defined as a constant on the ResourceDispatcherHost class?

http://codereview.chromium.org/18541/diff/1/5
File chrome/browser/renderer_host/resource_dispatcher_host.h (right):

http://codereview.chromium.org/18541/diff/1/5#newcode255
Line 255: static int max_outstanding_requests_per_process();
nit: since this is not implemented inline, i would go with CamelCaps naming.

http://codereview.chromium.org/18541/diff/1/5#newcode494
Line 494: typedef std::map<int, int> OutstandingRequestsCountMap;
you should probably comment that this is a map from render_process_host_id

http://codereview.chromium.org/18541/diff/1/4
File net/base/net_error_list.h (right):

http://codereview.chromium.org/18541/diff/1/4#newcode231
Line 231: NET_ERROR(TOO_MANY_OUTSTANDING_REQUESTS, -401)
how about just naming it TOO_MANY_REQUESTS.  that seems like a generic enough
error code to be useful in other contexts.  making it more generic like that
suggests that we should probably move it into the first error code section,
giving this a value of -12.

[Peter Kasting, 2009-01-23 18:39:26]

http://codereview.chromium.org/18541/diff/1/7
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/18541/diff/1/7#newcode74
Line 74: static const int kMaxOutstandingRequestsPerProcess = 100;
On 2009/01/23 16:43:14, darin wrote:
> 100 seems too small.  i could easily imagine normal web pages generating this
> many requests.

I agree.  Some valid edge case pages could generate hundreds of in-flight
requests.  Do we know what the memory cost of each request is?  If so, I'd like
to set this number so we don't prune requests until the memory footprint really
is way too big (I'd love to see this number be more like 5000).

[eroman, 2009-01-23 21:20:35]

Thanks for the feedback.

> 100 seems too small.  i could easily imagine normal web pages generating this
> many requests.

I will rework this patch's heuristic from:
  Constraint{"Number of outstanding requests"}
to:
  Constrain{"Total bytes consumed by outstanding requests"}

Where the byte cost for an individual request is modeled by:
  sizeof_upload_data(req) + sizeof_request_headers(req) +
max_sizeof_response_buffer + K

(subject to change, haven't confirmed details yet :-)

While a bit more effort, this avoids defining a magical "average request size"
constant.


This won't change the implementation details much -- basically the "counts" map
becomes a "total bytes" map.

And instead of decrementing by 1 on request completion, I decrement by whatever
value that request added into the map (will add this as a new field in
ExtraRequestInfo).


I will ping back when this is ready for re-review.

[eroman, 2009-01-23 21:51:51]

> I'm probably not understanding the proposal... what happens for very 
> large downloads?  (3 GB data files for instance)
> -Darin

This should continue to work as before.

I am only interested in the in-memory cost of requests, not how much they
transfer.

So I would credit this outstanding request for the in-memory datastructures
supporting it (URLRequest instance, HttpNetworkTransaction instance, etc...).

Does that make sense?

[eroman, 2009-01-27 01:55:31]

The bound is now defined as a "25MB per render process host".
Given average pending request cost of 4.5Kb [*], this is equivalent to ~6000.

[*] Empirically I found each pending request increases browser memory by ~4.5kb.

The testing methodology was to print the private bytes used by chrome.exe at 1
pending request, then at 50,000 pending requests, and average the increase. (All
requests were to the same host).


(I will update the issue title at the end -- doing so now would mess up the
thread view).

http://codereview.chromium.org/18541/diff/1/7
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/18541/diff/1/7#newcode74
Line 74: static const int kMaxOutstandingRequestsPerProcess = 100;
On 2009/01/23 18:39:26, pkasting wrote:
> On 2009/01/23 16:43:14, darin wrote:
> > 100 seems too small.  i could easily imagine normal web pages generating
this
> > many requests.
> 
> I agree.  Some valid edge case pages could generate hundreds of in-flight
> requests.  Do we know what the memory cost of each request is?  If so, I'd
like
> to set this number so we don't prune requests until the memory footprint
really
> is way too big (I'd love to see this number be more like 5000).
> 

Done, the limit is now close to 6000.

http://codereview.chromium.org/18541/diff/1/7#newcode581
Line 581: return kMaxOutstandingRequestsPerProcess;
On 2009/01/23 16:43:14, darin wrote:
> can this just be defined as a constant on the ResourceDispatcherHost class?

Done.

http://codereview.chromium.org/18541/diff/1/5
File chrome/browser/renderer_host/resource_dispatcher_host.h (right):

http://codereview.chromium.org/18541/diff/1/5#newcode255
Line 255: static int max_outstanding_requests_per_process();
On 2009/01/23 16:43:14, darin wrote:
> nit: since this is not implemented inline, i would go with CamelCaps naming.

No longer applicable (these are now enums).

http://codereview.chromium.org/18541/diff/1/5#newcode494
Line 494: typedef std::map<int, int> OutstandingRequestsCountMap;
On 2009/01/23 16:43:14, darin wrote:
> you should probably comment that this is a map from render_process_host_id

Done.

http://codereview.chromium.org/18541/diff/1/4
File net/base/net_error_list.h (right):

http://codereview.chromium.org/18541/diff/1/4#newcode231
Line 231: NET_ERROR(TOO_MANY_OUTSTANDING_REQUESTS, -401)
On 2009/01/23 16:43:14, darin wrote:
> how about just naming it TOO_MANY_REQUESTS.  that seems like a generic enough
> error code to be useful in other contexts.  making it more generic like that
> suggests that we should probably move it into the first error code section,
> giving this a value of -12.

Since I changed the bound to be defined in terms of memory rather than count,
renamed to:

INSUFFICIENT_RESOURCES

which is fairly generic.

http://codereview.chromium.org/18541/diff/202/29
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/18541/diff/202/29#newcode868
Line 868: if (request->has_upload()) {
What do you think of counting the upload sizes against renderers?

I am uncertain if this is a good idea, since don't know the size of (non-file)
uploads in the wild. Figured I would throw it in anyway to get feedback...

http://codereview.chromium.org/18541/diff/202/27
File chrome/browser/renderer_host/resource_dispatcher_host.h (right):

http://codereview.chromium.org/18541/diff/202/27#newcode263
Line 263: #ifdef UNIT_TEST
This is yucky, but the unit-test runs pretty slowly when using the regular bound
(slower than real life).

[eroman, 2009-01-30 10:58:32]

ping.

[darin (slow to review), 2009-01-30 16:56:43]

LGTM

http://codereview.chromium.org/18541/diff/202/29
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/18541/diff/202/29#newcode868
Line 868: if (request->has_upload()) {
it could be large... not sure.  a web app may be forced to use this form of data
upload since it does not have a way to write a file to the filesystem.

http://codereview.chromium.org/18541/diff/202/27
File chrome/browser/renderer_host/resource_dispatcher_host.h (right):

http://codereview.chromium.org/18541/diff/202/27#newcode266
Line 266: enum { kMaxOutstandingRequestsCostPerProcess = 440000 };
this is a little weird since resource_dispatcher_host.cc is not re-compiled for
unit tests.  so, ResourceDispatcherHost will still see the larger value in unit
tests.  changing the enum for unit tests only effects the _unittest.cc
compilation.  feels like it might be better to make this a variable instead of a
constant so that it can be modified by the unit test at runtime instead?

http://codereview.chromium.org/18541/diff/202/27#newcode274
Line 274: enum { kAvgBytesPerOutstandingRequest = 4400 };
i think the prevailing style for these is "const int kAvgBytes.." instead of
using an enum.

[eroman, 2009-01-31 02:31:27]

http://codereview.chromium.org/18541/diff/202/29
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/18541/diff/202/29#newcode868
Line 868: if (request->has_upload()) {
On 2009/01/30 16:56:43, darin wrote:
> it could be large... not sure.  a web app may be forced to use this form of
data
> upload since it does not have a way to write a file to the filesystem.

Maybe then I should remove this for now (and disable the associated unit test).

I can then add a histogram to figure out how much data our users upload, in
order to make a more informed decision.

http://codereview.chromium.org/18541/diff/202/27
File chrome/browser/renderer_host/resource_dispatcher_host.h (right):

http://codereview.chromium.org/18541/diff/202/27#newcode266
Line 266: enum { kMaxOutstandingRequestsCostPerProcess = 440000 };
On 2009/01/30 16:56:43, darin wrote:
> this is a little weird since resource_dispatcher_host.cc is not re-compiled
for
> unit tests.  so, ResourceDispatcherHost will still see the larger value in
unit
> tests.  changing the enum for unit tests only effects the _unittest.cc
> compilation.  feels like it might be better to make this a variable instead of
a
> constant so that it can be modified by the unit test at runtime instead?

D'oh!

I will make it into a private variable in that case.

[darin (slow to review), 2009-02-05 06:50:02]

LGTM w/ this change:

http://codereview.chromium.org/18541/diff/202/29
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/18541/diff/202/29#newcode868
Line 868: if (request->has_upload()) {
histogramming sounds like the better idea to me too.  +1 :)