When sending resources across the IPC barrier, use increasing sized buffers.

When sending resources across the IPC barrier, use increasing sized buffers.

AsyncResourceHandler::OnWillRead will usually allocate a new piece of shared
memory for each 32 kilobyte chunk of a resource to be sent over IPC from the
browser to the renderer. Instead, use a 32k chunk, then a 64k chunk, then a
128k chunk up to a maximum size of 512k.

Why? Because transferring large resources is really slow. To send a sequence of
32k chunks to the renderer, we need to wait for the renderer to send an ACK
message back before we send the next chunk.

The themeing on the new tab page is a pathological case for this code. Most of
the background images are large, and my test case is about 800k. This was 25
round trips. Now it is 5. According to the web inspector, it used to take ~700ms to transfer said image. Now
it's in the ~30ms range. It feels faster, and the web inspector shows the speed up, but tab_complex_theme_cold does not...

BUG=http://crbug.com/24493
TEST=Doesn't regress any performance tests.

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

[agl, 2009-10-23 17:01:12]

LGTM.

I'm ok with this although it's mostly papering over the fact that this design is
quite bad for POSIX systems. Setting up and tearing down memory is expensive
there (any maybe on Windows too, I don't know.) In the long term, we should have
caches of these shared memory segments like the TransportDIB cache.

http://codereview.chromium.org/331001/diff/1/2
File chrome/browser/renderer_host/async_resource_handler.cc (right):

http://codereview.chromium.org/331001/diff/1/2#newcode21
Line 21: const int kReadBufSize = 32768;
static

http://codereview.chromium.org/331001/diff/1/2#newcode24
Line 24: const int kMaxBufSize = 524288;
static

[Elliot Glaysher, 2009-10-23 17:06:59]

> In the long term, we should have
> caches of these shared memory segments like the
> TransportDIB cache.

I agree. I'm just doing this as a short term fix.

http://codereview.chromium.org/331001/diff/1/2
File chrome/browser/renderer_host/async_resource_handler.cc (right):

http://codereview.chromium.org/331001/diff/1/2#newcode21
Line 21: const int kReadBufSize = 32768;
On 2009/10/23 17:01:12, agl wrote:
> static

anonymous namespace

[rvargas (doing something else), 2009-10-23 17:52:04]

I think you should be able to write a unit test
(resource_dispatcher_host_unittest.cc) that verifies the number of messages
received.

Other than that LGTM

http://codereview.chromium.org/331001/diff/1/2
File chrome/browser/renderer_host/async_resource_handler.cc (right):

http://codereview.chromium.org/331001/diff/1/2#newcode107
Line 107: *buf_size = read_buffer_.get()->buffer_size();
nit: drop the get()

[darin (slow to review), 2009-10-23 17:59:40]

http://codereview.chromium.org/331001/diff/1/2
File chrome/browser/renderer_host/async_resource_handler.cc (right):

http://codereview.chromium.org/331001/diff/1/2#newcode21
Line 21: const int kReadBufSize = 32768;
kInitialReadBufSize?

http://codereview.chromium.org/331001/diff/1/2#newcode24
Line 24: const int kMaxBufSize = 524288;
kMaxReadBufSize

http://codereview.chromium.org/331001/diff/1/2#newcode120
Line 120: next_buffer_size_ = std::min(next_buffer_size_ * 2, kMaxBufSize);
we do allow up to 20 (iirc) unanswered resource IPCs.  perhaps we should only
increase the buffer size if there are unanswered IPCs?

why progressively increase the buffer size?  why not just make the buffers 64k
instead of 32k, or maybe something even larger to begin with?

[rvargas (doing something else), 2009-11-03 03:54:30]

I know this was closed long time ago, but no comment on the review comments?

On Fri, Oct 23, 2009 at 9:59 AM, <darin@chromium.org> wrote:

>
> http://codereview.chromium.org/331001/diff/1/2
> File chrome/browser/renderer_host/async_resource_handler.cc (right):
>
> http://codereview.chromium.org/331001/diff/1/2#newcode21
> Line 21: const int kReadBufSize = 32768;
> kInitialReadBufSize?
>
>
> http://codereview.chromium.org/331001/diff/1/2#newcode24
> Line 24: const int kMaxBufSize = 524288;
> kMaxReadBufSize
>
> http://codereview.chromium.org/331001/diff/1/2#newcode120
> Line 120: next_buffer_size_ = std::min(next_buffer_size_ * 2,
> kMaxBufSize);
> we do allow up to 20 (iirc) unanswered resource IPCs.  perhaps we should
> only increase the buffer size if there are unanswered IPCs?
>
> why progressively increase the buffer size?  why not just make the
> buffers 64k instead of 32k, or maybe something even larger to begin
> with?
>
>
> http://codereview.chromium.org/331001
>