Release SpdySession socket read buffer

Release SpdySession socket read buffer

This CL makes the following changes:

(1) Make SpdySession use Socket::ReadIfReady(). This new behavior is
    hidden behind a field trial flag, SocketReadIfReady.
(2) Make SpdySession lazily allocate |read_buffer| on demand.
(3) Make SpdySession release |read_buffer| when Read() completes or 
    when ReadIfReady() cannot be completed synchronously.

Re-allocating an 8KiB buffer should be cheap, see more discussion
at https://groups.google.com/a/chromium.org/d/msg/project-trim/QtliUsApxyk/AOSnyuY7CgAJ

BUG=690915
Review-Url: https://codereview.chromium.org/2734933002
Cr-Commit-Position: refs/heads/master@{#455770}
Committed: https://chromium.googlesource.com/chromium/src/+/c182846fcda260cc3d11dbac775b1b7a666729c8

[xunjieli, 2017-03-06 19:56:57]

Description was changed from

==========
[TEMP] Release SpdySession socket read buffer

BUG=690915
==========

to

==========
[TEMP] Release SpdySession socket read buffer

Make SpdySession use Socket::ReadIfReady() and releases |read_buffer|
and allocates it on demand.

This new feature is hidden behind a field trial flag.

BUG=690915
==========

[xunjieli, 2017-03-06 19:57:06]

Description was changed from

==========
[TEMP] Release SpdySession socket read buffer

Make SpdySession use Socket::ReadIfReady() and releases |read_buffer|
and allocates it on demand.

This new feature is hidden behind a field trial flag.

BUG=690915
==========

to

==========
[TEMP] Release SpdySession socket read buffer

Make SpdySession use Socket::ReadIfReady() and releases |read_buffer|
and allocates it on demand.

This new behavior is hidden behind a field trial flag.

BUG=690915
==========

[xunjieli, 2017-03-06 20:12:21]

Description was changed from

==========
[TEMP] Release SpdySession socket read buffer

Make SpdySession use Socket::ReadIfReady() and releases |read_buffer|
and allocates it on demand.

This new behavior is hidden behind a field trial flag.

BUG=690915
==========

to

==========
[TEMP] Release SpdySession socket read buffer

Make SpdySession use Socket::ReadIfReady() and releases |read_buffer_|
and allocates it on demand.

This new behavior is hidden behind a field trial flag.

BUG=690915
==========

[DmitrySkiba, 2017-03-06 21:42:36]

dskiba@chromium.org changed reviewers:
+ dskiba@chromium.org

[DmitrySkiba, 2017-03-06 21:42:37]

https://codereview.chromium.org/2734933002/diff/1/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/2734933002/diff/1/net/spdy/spdy_session.cc#ne...
net/spdy/spdy_session.cc:1877: if (rv == ERR_IO_PENDING)
Are we killing buffer with read operation in progress? I thought read_buffer_
should be alive until DoReadComplete() is called.

[xunjieli, 2017-03-06 21:58:59]

https://codereview.chromium.org/2734933002/diff/1/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/2734933002/diff/1/net/spdy/spdy_session.cc#ne...
net/spdy/spdy_session.cc:1877: if (rv == ERR_IO_PENDING)
On 2017/03/06 21:42:37, DmitrySkiba wrote:
> Are we killing buffer with read operation in progress? I thought read_buffer_
> should be alive until DoReadComplete() is called.

Great question. The new ReadIfReady() method that I implemented allows us to
release |read_buffer_| if a read is pending.

The CL is at https://codereview.chromium.org/2593063003/, which is expected to
land soon.

[xunjieli, 2017-03-06 22:00:51]

https://codereview.chromium.org/2734933002/diff/1/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/2734933002/diff/1/net/spdy/spdy_session.cc#ne...
net/spdy/spdy_session.cc:1877: if (rv == ERR_IO_PENDING)
On 2017/03/06 21:58:59, xunjieli wrote:
> On 2017/03/06 21:42:37, DmitrySkiba wrote:
> > Are we killing buffer with read operation in progress? I thought
read_buffer_
> > should be alive until DoReadComplete() is called.
> 
> Great question. The new ReadIfReady() method that I implemented allows us to
> release |read_buffer_| if a read is pending.
> 
> The CL is at https://codereview.chromium.org/2593063003/, which is expected to
> land soon.

Btw, if you apply the CL linked above and start Chrome with
"--enable-features=SocketReadIfReady", all BIOAdapter buffers will be gone.

[xunjieli, 2017-03-07 21:15:10]

Description was changed from

==========
[TEMP] Release SpdySession socket read buffer

Make SpdySession use Socket::ReadIfReady() and releases |read_buffer_|
and allocates it on demand.

This new behavior is hidden behind a field trial flag.

BUG=690915
==========

to

==========
Release SpdySession socket read buffer

This CL makes the following changes:

(1) Make SpdySession use Socket::ReadIfReady(). This new behavior is
    hidden behind a field trial flag, SocketReadIfReady.
(2) Make SpdySession lazily allocate |read_buffer| on demand.
(3) Make SpdySession releases |read_buffer| when Read() completes or 
    when ReadIfReady() cannot be completed synchronously.

Re-allocating an 8KiB buffer should be cheap, see more discussion
at
https://groups.google.com/a/chromium.org/d/msg/project-trim/QtliUsApxyk/AOSny...

BUG=690915
==========

[xunjieli, 2017-03-07 21:15:10]

xunjieli@chromium.org changed reviewers:
+ bnc@chromium.org, davidben@chromium.org
- dskiba@chromium.org

[xunjieli, 2017-03-07 21:43:36]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 21:44:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[davidben, 2017-03-07 22:53:21]

lgtm

https://codereview.chromium.org/2734933002/diff/20001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/2734933002/diff/20001/net/spdy/spdy_session.c...
net/spdy/spdy_session.cc:1886: read_buffer_ = nullptr;
This could be moved into the line 1895 check, right?

[xunjieli, 2017-03-07 23:09:30]

https://codereview.chromium.org/2734933002/diff/20001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/2734933002/diff/20001/net/spdy/spdy_session.c...
net/spdy/spdy_session.cc:1886: read_buffer_ = nullptr;
On 2017/03/07 22:53:21, davidben wrote:
> This could be moved into the line 1895 check, right?

No, in the case of a normal Read(), we can only release the buffer in
DoReadComplete().

[davidben, 2017-03-07 23:12:37]

https://codereview.chromium.org/2734933002/diff/20001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/2734933002/diff/20001/net/spdy/spdy_session.c...
net/spdy/spdy_session.cc:1886: read_buffer_ = nullptr;
On 2017/03/07 23:09:30, xunjieli wrote:
> On 2017/03/07 22:53:21, davidben wrote:
> > This could be moved into the line 1895 check, right?
> 
> No, in the case of a normal Read(), we can only release the buffer in
> DoReadComplete().

Right, but normal Read doesn't hit that code. A normal Read would never go from
DoRead to DoRead I think.

[commit-bot: I haz the power, 2017-03-07 23:17:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-07 23:17:44]

Dry run: This issue passed the CQ dry run.

[xunjieli, 2017-03-07 23:39:53]

https://codereview.chromium.org/2734933002/diff/20001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/2734933002/diff/20001/net/spdy/spdy_session.c...
net/spdy/spdy_session.cc:1886: read_buffer_ = nullptr;
On 2017/03/07 23:12:37, davidben wrote:
> On 2017/03/07 23:09:30, xunjieli wrote:
> > On 2017/03/07 22:53:21, davidben wrote:
> > > This could be moved into the line 1895 check, right?
> > 
> > No, in the case of a normal Read(), we can only release the buffer in
> > DoReadComplete().
> 
> Right, but normal Read doesn't hit that code. A normal Read would never go
from
> DoRead to DoRead I think.

Done.  Oops.. You are right. I can't believe I missed that case. I moved the
state change here. I think that flows slightly better.

[xunjieli, 2017-03-07 23:40:46]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 23:41:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[davidben, 2017-03-08 00:13:29]

lgtm

[commit-bot: I haz the power, 2017-03-08 01:12:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-08 01:12:38]

Dry run: This issue passed the CQ dry run.

[Bence, 2017-03-09 14:27:19]

In CL description, s/releases/release/ in first line of (3).  Otherwise LGTM.  I
apologize for the delay.

[xunjieli, 2017-03-09 15:34:13]

Description was changed from

==========
Release SpdySession socket read buffer

This CL makes the following changes:

(1) Make SpdySession use Socket::ReadIfReady(). This new behavior is
    hidden behind a field trial flag, SocketReadIfReady.
(2) Make SpdySession lazily allocate |read_buffer| on demand.
(3) Make SpdySession releases |read_buffer| when Read() completes or 
    when ReadIfReady() cannot be completed synchronously.

Re-allocating an 8KiB buffer should be cheap, see more discussion
at
https://groups.google.com/a/chromium.org/d/msg/project-trim/QtliUsApxyk/AOSny...

BUG=690915
==========

to

==========
Release SpdySession socket read buffer

This CL makes the following changes:

(1) Make SpdySession use Socket::ReadIfReady(). This new behavior is
    hidden behind a field trial flag, SocketReadIfReady.
(2) Make SpdySession lazily allocate |read_buffer| on demand.
(3) Make SpdySession release |read_buffer| when Read() completes or 
    when ReadIfReady() cannot be completed synchronously.

Re-allocating an 8KiB buffer should be cheap, see more discussion
at
https://groups.google.com/a/chromium.org/d/msg/project-trim/QtliUsApxyk/AOSny...

BUG=690915
==========

[xunjieli, 2017-03-09 15:34:24]

On 2017/03/09 14:27:19, Bence wrote:
> In CL description, s/releases/release/ in first line of (3).  Otherwise LGTM. 
I
> apologize for the delay.

Done. Thanks!

[xunjieli, 2017-03-09 15:34:33]

The CQ bit was checked by xunjieli@chromium.org

[commit-bot: I haz the power, 2017-03-09 15:34:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-09 16:49:23]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489073673809360,
"parent_rev": "9b88406d1a87dfeef593c6d1a91beb3a2b696391", "commit_rev":
"c182846fcda260cc3d11dbac775b1b7a666729c8"}

[commit-bot: I haz the power, 2017-03-09 16:49:59]

Description was changed from

==========
Release SpdySession socket read buffer

This CL makes the following changes:

(1) Make SpdySession use Socket::ReadIfReady(). This new behavior is
    hidden behind a field trial flag, SocketReadIfReady.
(2) Make SpdySession lazily allocate |read_buffer| on demand.
(3) Make SpdySession release |read_buffer| when Read() completes or 
    when ReadIfReady() cannot be completed synchronously.

Re-allocating an 8KiB buffer should be cheap, see more discussion
at
https://groups.google.com/a/chromium.org/d/msg/project-trim/QtliUsApxyk/AOSny...

BUG=690915
==========

to

==========
Release SpdySession socket read buffer

This CL makes the following changes:

(1) Make SpdySession use Socket::ReadIfReady(). This new behavior is
    hidden behind a field trial flag, SocketReadIfReady.
(2) Make SpdySession lazily allocate |read_buffer| on demand.
(3) Make SpdySession release |read_buffer| when Read() completes or 
    when ReadIfReady() cannot be completed synchronously.

Re-allocating an 8KiB buffer should be cheap, see more discussion
at
https://groups.google.com/a/chromium.org/d/msg/project-trim/QtliUsApxyk/AOSny...

BUG=690915

Review-Url: https://codereview.chromium.org/2734933002
Cr-Commit-Position: refs/heads/master@{#455770}
Committed:
https://chromium.googlesource.com/chromium/src/+/c182846fcda260cc3d11dbac775b...
==========

[commit-bot: I haz the power, 2017-03-09 16:50:00]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/c182846fcda260cc3d11dbac775b...