Modify AsyncResourceHandler to use a single shared memory buffer (in ring

Modify AsyncResourceHandler to use a single shared memory buffer (in ring
buffer fashion) for transmitting resource response data to the renderer.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=157310

[darin (slow to review), 2012-09-14 05:58:12]

I ran page cycler locally on a couple different machines in an attempt to tune
the buffer sizes.  I arrived at a 512K buffer that gets filled 32K at a time
(each URLRequest::Read call).  With this configuration, in my testing with page
cycler, the buffer rarely / never filled before the renderer consumed the data.

On HTML5 videos, especially when loading from the cache, the buffer fills, but I
think having 512K in flight should be sufficient for good video playback
performance.  When not loading from cache, we actually end up being able to read
more from the network.  Previously, we could only have 20 buffers in flight at a
time.  Now, with a minimum read size of 4096, it is possible to end up with 128
ResourceMsg_DataReceived IPCs in flight.  This seems like a good thing.

Finally, capping URLRequest::Read at 32K instead of allowing it to balloon to
512K seems good from a responsiveness point of view.  I just haven't found a
good way to measure this though.  Ideas welcome.

[James Simonsen, 2012-09-15 02:19:06]

lgtm

We should keep an eye on the buffer stats, particularly on Android.

To measure responsiveness, what about a renderer histogram for how long it
spends processing OnReceivedData calls? It'll be noisy based on the content, but
we might get enough data to see some signal. Not sure how much will be because
of the buffer sizes though.

http://codereview.chromium.org/10911297/diff/12031/content/browser/renderer_h...
File content/browser/renderer_host/async_resource_handler.cc (right):

http://codereview.chromium.org/10911297/diff/12031/content/browser/renderer_h...
content/browser/renderer_host/async_resource_handler.cc:8: #include <stdio.h>
not needed?

http://codereview.chromium.org/10911297/diff/12031/content/browser/renderer_h...
File content/browser/renderer_host/async_resource_handler.h (right):

http://codereview.chromium.org/10911297/diff/12031/content/browser/renderer_h...
content/browser/renderer_host/async_resource_handler.h:8: #include <queue>
not needed

http://codereview.chromium.org/10911297/diff/12031/content/browser/renderer_h...
File content/browser/renderer_host/resource_buffer.cc (right):

http://codereview.chromium.org/10911297/diff/12031/content/browser/renderer_h...
content/browser/renderer_host/resource_buffer.cc:45: int max_allocation_size) {
Should we DCHECK that these are all integer multiples of the
min_allocation_size? It'd be wasteful if a user didn't do that.

http://codereview.chromium.org/10911297/diff/12031/content/browser/renderer_h...
File content/browser/renderer_host/resource_buffer.h (right):

http://codereview.chromium.org/10911297/diff/12031/content/browser/renderer_h...
content/browser/renderer_host/resource_buffer.h:21: // space in the buffer. 
Allocate may be called multiple times until the buffer
This class has an implicit state machine, mostly because of the handling of the
last allocation. You don't actually want a user to allocate again until they've
written into the last allocated buffer, shrunk it, and got the offset. It might
be worth spelling that out.

http://codereview.chromium.org/10911297/diff/12031/content/browser/renderer_h...
content/browser/renderer_host/resource_buffer.h:35: // the Allocate mtehod.  It
will prefer to return segments that are
mtehod -> method

[darin (slow to review), 2012-09-17 15:52:10]

On 2012/09/15 02:19:06, James Simonsen wrote:
> lgtm
> 
> We should keep an eye on the buffer stats, particularly on Android.

Agreed.  I'm expecting I'll need to revert once or twice :-/


> To measure responsiveness, what about a renderer histogram for how long it
> spends processing OnReceivedData calls? It'll be noisy based on the content,
but
> we might get enough data to see some signal. Not sure how much will be because
> of the buffer sizes though.

Added histogram.  Fixed nits.


http://codereview.chromium.org/10911297/diff/12031/content/browser/renderer_h...
> content/browser/renderer_host/resource_buffer.cc:45: int max_allocation_size)
{
> Should we DCHECK that these are all integer multiples of the
> min_allocation_size? It'd be wasteful if a user didn't do that.

Done.  Good idea!


>
http://codereview.chromium.org/10911297/diff/12031/content/browser/renderer_h...
> File content/browser/renderer_host/resource_buffer.h (right):
> 
>
http://codereview.chromium.org/10911297/diff/12031/content/browser/renderer_h...
> content/browser/renderer_host/resource_buffer.h:21: // space in the buffer. 
> Allocate may be called multiple times until the buffer
> This class has an implicit state machine, mostly because of the handling of
the
> last allocation. You don't actually want a user to allocate again until
they've
> written into the last allocated buffer, shrunk it, and got the offset. It
might
> be worth spelling that out.

I added an example of how one might use the class, which highlights these
methods.
Hopefully that plus a note about ShrinkLastAllocation should do the trick.

[brettw, 2012-09-17 23:53:10]

http://codereview.chromium.org/10911297/diff/5019/content/browser/renderer_ho...
File content/browser/renderer_host/resource_buffer.cc (right):

http://codereview.chromium.org/10911297/diff/5019/content/browser/renderer_ho...
content/browser/renderer_host/resource_buffer.cc:7: #include <math.h>
Blank line after this.

http://codereview.chromium.org/10911297/diff/5019/content/browser/renderer_ho...
content/browser/renderer_host/resource_buffer.cc:18: //   
------------[XXXXXXXXXXXXXXXXXXXXXXX]----
In these diagrams it might be better to have end pointing to the "-" right after
the "]" to show it's pointing to the next free spot.

http://codereview.chromium.org/10911297/diff/5019/content/browser/renderer_ho...
content/browser/renderer_host/resource_buffer.cc:95: if (alloc_start_ == -1) {
The different cases in this nested if statement could use quick comments to aid
skimming it. Even something like "Buffer wraparound case." would be helpful.

http://codereview.chromium.org/10911297/diff/5019/content/browser/renderer_ho...
content/browser/renderer_host/resource_buffer.cc:116:
alloc_sizes_.push(alloc_size);
It took me a minute to realize why this goes before the fixup for
max_alloc_size_ below (it looked like a bug). A comment about this would be
helpful.

http://codereview.chromium.org/10911297/diff/5019/content/browser/renderer_ho...
File content/browser/renderer_host/resource_buffer.h (right):

http://codereview.chromium.org/10911297/diff/5019/content/browser/renderer_ho...
content/browser/renderer_host/resource_buffer.h:112: int alloc_start_;
Can you comment about the values this can have (looks like there is a special
"-1" meaning.

http://codereview.chromium.org/10911297/diff/5019/content/browser/renderer_ho...
content/browser/renderer_host/resource_buffer.h:113: int alloc_end_;
Can you clarify if this is inclusive or exclusive?

http://codereview.chromium.org/10911297/diff/15001/content/browser/renderer_h...
File content/browser/renderer_host/resource_buffer.cc (right):

http://codereview.chromium.org/10911297/diff/15001/content/browser/renderer_h...
content/browser/renderer_host/resource_buffer.cc:105: DCHECK(alloc_start_ > 0);
Can you DCHECK that alloc_start_ > min_alloc_size_ and write a brief comment
about why that's the case. I missed that originally.

[brettw, 2012-09-18 04:44:54]

LGTM with fix

http://codereview.chromium.org/10911297/diff/15001/content/browser/renderer_h...
File content/browser/renderer_host/async_resource_handler.cc (right):

http://codereview.chromium.org/10911297/diff/15001/content/browser/renderer_h...
content/browser/renderer_host/async_resource_handler.cc:50: void
InitializeResourceBufferConstants() {
This seems to be never called. Maybe you want to call it from the async request
handler constructor or something.

[darin (slow to review), 2012-09-18 05:00:50]

http://codereview.chromium.org/10911297/diff/5019/content/browser/renderer_ho...
File content/browser/renderer_host/resource_buffer.cc (right):

http://codereview.chromium.org/10911297/diff/5019/content/browser/renderer_ho...
content/browser/renderer_host/resource_buffer.cc:7: #include <math.h>
On 2012/09/17 23:53:10, brettw wrote:
> Blank line after this.

Done.

http://codereview.chromium.org/10911297/diff/5019/content/browser/renderer_ho...
content/browser/renderer_host/resource_buffer.cc:18: //   
------------[XXXXXXXXXXXXXXXXXXXXXXX]----
On 2012/09/17 23:53:10, brettw wrote:
> In these diagrams it might be better to have end pointing to the "-" right
after
> the "]" to show it's pointing to the next free spot.

Done.

http://codereview.chromium.org/10911297/diff/5019/content/browser/renderer_ho...
content/browser/renderer_host/resource_buffer.cc:116:
alloc_sizes_.push(alloc_size);
On 2012/09/17 23:53:10, brettw wrote:
> It took me a minute to realize why this goes before the fixup for
> max_alloc_size_ below (it looked like a bug). A comment about this would be
> helpful.

Done.

http://codereview.chromium.org/10911297/diff/15001/content/browser/renderer_h...
File content/browser/renderer_host/resource_buffer.cc (right):

http://codereview.chromium.org/10911297/diff/15001/content/browser/renderer_h...
content/browser/renderer_host/resource_buffer.cc:105: DCHECK(alloc_start_ > 0);
On 2012/09/17 23:53:10, brettw wrote:
> Can you DCHECK that alloc_start_ > min_alloc_size_ and write a brief comment
> about why that's the case. I missed that originally.

Done.

[jar (doing other things), 2013-03-27 16:55:22]

https://codereview.chromium.org/10911297/diff/28001/content/browser/renderer_...
File content/browser/renderer_host/async_resource_handler.cc (right):

https://codereview.chromium.org/10911297/diff/28001/content/browser/renderer_...
content/browser/renderer_host/async_resource_handler.cc:235: bytes_read, 0,
kMaxAllocationSize, 100);
FYI: It is to late now.... but best practice when changing the specs on a
histogram is to obsolete the old name, and change the name on the histogram
(often just a trailing digit or such works well).

This change reached users in versions 23.0.1271.0 and later, conflating the two
variations of histograms.

This one caught my eye when folks noticed some excessive bucket counts in this
histogram(this change was only a small part of a bigger problem that I'm still
looking into).

The other histogram change (line 220) in this file should also be renamed.

[darin (slow to review), 2013-03-27 17:42:01]

Ah, sorry about that.  I had no idea.  Do you recommend changing the name
at this point?


On Wed, Mar 27, 2013 at 9:55 AM, <jar@chromium.org> wrote:

>
> https://codereview.chromium.**org/10911297/diff/28001/**
>
content/browser/renderer_host/**async_resource_handler.cc<https://codereview.chromium.org/10911297/diff/28001/content/browser/renderer_host/async_resource_handler.cc>
> File content/browser/renderer_host/**async_resource_handler.cc (right):
>
> https://codereview.chromium.**org/10911297/diff/28001/**
>
content/browser/renderer_host/**async_resource_handler.cc#**newcode235<https://codereview.chromium.org/10911297/diff/28001/content/browser/renderer_host/async_resource_handler.cc#newcode235>
> content/browser/renderer_host/**async_resource_handler.cc:235: bytes_read,
> 0, kMaxAllocationSize, 100);
> FYI: It is to late now.... but best practice when changing the specs on
> a histogram is to obsolete the old name, and change the name on the
> histogram (often just a trailing digit or such works well).
>
> This change reached users in versions 23.0.1271.0 and later, conflating
> the two variations of histograms.
>
> This one caught my eye when folks noticed some excessive bucket counts
> in this histogram(this change was only a small part of a bigger problem
> that I'm still looking into).
>
> The other histogram change (line 220) in this file should also be
> renamed.
>
>
https://codereview.chromium.**org/10911297/<https://codereview.chromium.org/1...
>

[jar (doing other things), 2013-03-28 16:27:57]

Yeah... it would be better to have a new name, so that when you look at
"Everything" you don't get combinations that include users that haven't
upgraded recently (and there are always a lot of them).

Jim


On Wed, Mar 27, 2013 at 10:41 AM, Darin Fisher <darin@chromium.org> wrote:

> Ah, sorry about that.  I had no idea.  Do you recommend changing the name
> at this point?
>
>
> On Wed, Mar 27, 2013 at 9:55 AM, <jar@chromium.org> wrote:
>
>>
>> https://codereview.chromium.**org/10911297/diff/28001/**
>>
content/browser/renderer_host/**async_resource_handler.cc<https://codereview.chromium.org/10911297/diff/28001/content/browser/renderer_host/async_resource_handler.cc>
>> File content/browser/renderer_host/**async_resource_handler.cc (right):
>>
>> https://codereview.chromium.**org/10911297/diff/28001/**
>>
content/browser/renderer_host/**async_resource_handler.cc#**newcode235<https://codereview.chromium.org/10911297/diff/28001/content/browser/renderer_host/async_resource_handler.cc#newcode235>
>> content/browser/renderer_host/**async_resource_handler.cc:235:
>> bytes_read,
>> 0, kMaxAllocationSize, 100);
>> FYI: It is to late now.... but best practice when changing the specs on
>> a histogram is to obsolete the old name, and change the name on the
>> histogram (often just a trailing digit or such works well).
>>
>> This change reached users in versions 23.0.1271.0 and later, conflating
>> the two variations of histograms.
>>
>> This one caught my eye when folks noticed some excessive bucket counts
>> in this histogram(this change was only a small part of a bigger problem
>> that I'm still looking into).
>>
>> The other histogram change (line 220) in this file should also be
>> renamed.
>>
>>
https://codereview.chromium.**org/10911297/<https://codereview.chromium.org/1...
>>
>
>