SpdySession: Combine three frames into a single packet.

SpdySession: Combine three frames into a single packet.

Combine HTTP/2 connection preface, initial SETTINGS frame, and optional
initial WINDOW_UPDATE frame into a single SpdySerializedFrame so that it
can be send on the wire in a single packet.

Also combine said frames in tests.  (Because of the way expected and
actual write data are compared, tests expecting individual packets
(before this change) pass with the packets sent separately or combined,
but tests expecting combined packets (after this change) only pass
when packets are actually combined.)

BUG=705551
Review-Url: https://codereview.chromium.org/2909653002
Cr-Commit-Position: refs/heads/master@{#477745}
Committed: https://chromium.googlesource.com/chromium/src/+/8abf64aff97080b772d924ebf79d4c74d14eea0d

[Bence, 2017-05-26 16:03:59]

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

[commit-bot: I haz the power, 2017-05-26 16:04:17]

Dry run: CQ is trying da patch.

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

[Bence, 2017-05-26 16:06:14]

Description was changed from

==========
SpdySession: Combine three frames into a single packet.

Combine HTTP/2 connection preface, initial SETTINGS frame, and optional
initial WINDOW_UPDATE frame into a single SpdySerializedFrame so that it
can be send on the wire in a single packet.

BUG=705551
==========

to

==========
SpdySession: Combine three frames into a single packet.

Combine HTTP/2 connection preface, initial SETTINGS frame, and optional
initial WINDOW_UPDATE frame into a single SpdySerializedFrame so that it
can be send on the wire in a single packet.

Note that nothing changes on the unencrypted layer, so there is
no need to update any tests.  On the other hand, this results in
sending only one TLS packet and only one underlying TCP packet
instead of three.

BUG=705551
==========

[commit-bot: I haz the power, 2017-05-26 17:34:03]

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

[commit-bot: I haz the power, 2017-05-26 17:34:04]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Bence, 2017-05-26 18:28:43]

bnc@chromium.org changed reviewers:
+ zhongyi@chromium.org

[Bence, 2017-05-26 18:28:44]

Zhongyi: PTAL.  Thank you!

[Zhongyi Shi, 2017-05-30 23:10:50]

lgtm modulo nits

Sorry about the delay, I was OOO last Friday.

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

https://codereview.chromium.org/2909653002/diff/1/net/spdy/chromium/spdy_sess...
net/spdy/chromium/spdy_session.cc:2165: if (send_window_update) {
nit: If I understand the code correctly, the previous block was updating recv
window only. And this block is actually send the window update. 

Instead of using |send_window_update|, could you change that to 
if (session_max_recv_window_size_ > session_recv_window_size_) {
  // Update RECV window
  ...
}
if (session_unacked_recv_window_bytes_ > session_max_recv_window_size_ / 2) {
  // Send Window update.
  ...
}

I think that's more readable and not so confusing.

[Bence, 2017-06-05 15:37:37]

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

[commit-bot: I haz the power, 2017-06-05 15:37:55]

Dry run: 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-06-05 17:12:19]

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

[commit-bot: I haz the power, 2017-06-05 17:12:20]

Dry run: This issue passed the CQ dry run.

[Bence, 2017-06-05 17:19:03]

Zhongyi: PTAL.  Thanks.

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

https://codereview.chromium.org/2909653002/diff/1/net/spdy/chromium/spdy_sess...
net/spdy/chromium/spdy_session.cc:2165: if (send_window_update) {
On 2017/05/30 23:10:49, Zhongyi Shi wrote:
> nit: If I understand the code correctly, the previous block was updating recv
> window only. And this block is actually send the window update. 
> 
> Instead of using |send_window_update|, could you change that to 
> if (session_max_recv_window_size_ > session_recv_window_size_) {
>   // Update RECV window
>   ...
> }
> if (session_unacked_recv_window_bytes_ > session_max_recv_window_size_ / 2) {
>   // Send Window update.
>   ...
> }
> 
> I think that's more readable and not so confusing.

I agree with you that this logic flow is unnecessary complicated, thank you for
catching it.

How about always sending a WINDOW_UPDATE frame if |session_max_recv_window_size_
> session_recv_window_size_|?  This would simplify things even further, and
would not matter in practice: |session_recv_window_size_| is defined to be
initially 64 kB in the protocol specification [1, 2], and
|session_max_recv_window_size_| is 15 MB in production [3].

[1]
https://cs.chromium.org/chromium/src/net/spdy/chromium/spdy_session.h?q=kDefa...
[2] http://httpwg.org/specs/rfc7540.html#SettingValues
[3] https://cs.chromium.org/chromium/src/net/http/http_network_session.cc?l=70

[Bence, 2017-06-07 00:19:05]

bnc@chromium.org changed reviewers:
+ rch@chromium.org

[Bence, 2017-06-07 00:19:06]

Ryan: PTAL for a second opinion please.

[Ryan Hamilton, 2017-06-07 13:52:42]

"Note that nothing changes on the unencrypted layer, so there is
no need to update any tests."

I'm not sure this is exactly correct. If I understand correctly, this changes
from 3 writes to a single write. As such, you should be able to change the
MockWrites to a single MockWrite, yes?

[Bence, 2017-06-07 18:05:30]

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

[commit-bot: I haz the power, 2017-06-07 18:05:56]

Dry run: CQ is trying da patch.

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

[Bence, 2017-06-07 18:07:41]

Description was changed from

==========
SpdySession: Combine three frames into a single packet.

Combine HTTP/2 connection preface, initial SETTINGS frame, and optional
initial WINDOW_UPDATE frame into a single SpdySerializedFrame so that it
can be send on the wire in a single packet.

Note that nothing changes on the unencrypted layer, so there is
no need to update any tests.  On the other hand, this results in
sending only one TLS packet and only one underlying TCP packet
instead of three.

BUG=705551
==========

to

==========
SpdySession: Combine three frames into a single packet.

Combine HTTP/2 connection preface, initial SETTINGS frame, and optional
initial WINDOW_UPDATE frame into a single SpdySerializedFrame so that it
can be send on the wire in a single packet.

Also combine said frames in tests.  (Because of the way expected and
actual write data are compared, tests expecting individual packets
(before this change) pass with the packets sent separately or combined,
but tests expecting combined packets (after this change) only pass
when packets are actually combined.)

BUG=705551
==========

[Bence, 2017-06-07 18:07:57]

Ryan: PTAL.  Thank you for catching this.

[Ryan Hamilton, 2017-06-07 18:10:34]

lgtm

nice!

[commit-bot: I haz the power, 2017-06-07 20:06:43]

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

[commit-bot: I haz the power, 2017-06-07 20:06:44]

Dry run: This issue passed the CQ dry run.

[Bence, 2017-06-07 20:12:21]

The CQ bit was checked by bnc@chromium.org

[Bence, 2017-06-07 20:12:22]

The patchset sent to the CQ was uploaded after l-g-t-m from zhongyi@chromium.org
Link to the patchset: https://codereview.chromium.org/2909653002/#ps60001
(title: "Combine frames in tests.")

[commit-bot: I haz the power, 2017-06-07 20:12:38]

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-06-07 20:18:56]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496866341585410,
"parent_rev": "35bd9d42270d4474a102f730800c4bfa3b859146", "commit_rev":
"8abf64aff97080b772d924ebf79d4c74d14eea0d"}

[commit-bot: I haz the power, 2017-06-07 20:19:07]

Description was changed from

==========
SpdySession: Combine three frames into a single packet.

Combine HTTP/2 connection preface, initial SETTINGS frame, and optional
initial WINDOW_UPDATE frame into a single SpdySerializedFrame so that it
can be send on the wire in a single packet.

Also combine said frames in tests.  (Because of the way expected and
actual write data are compared, tests expecting individual packets
(before this change) pass with the packets sent separately or combined,
but tests expecting combined packets (after this change) only pass
when packets are actually combined.)

BUG=705551
==========

to

==========
SpdySession: Combine three frames into a single packet.

Combine HTTP/2 connection preface, initial SETTINGS frame, and optional
initial WINDOW_UPDATE frame into a single SpdySerializedFrame so that it
can be send on the wire in a single packet.

Also combine said frames in tests.  (Because of the way expected and
actual write data are compared, tests expecting individual packets
(before this change) pass with the packets sent separately or combined,
but tests expecting combined packets (after this change) only pass
when packets are actually combined.)

BUG=705551

Review-Url: https://codereview.chromium.org/2909653002
Cr-Commit-Position: refs/heads/master@{#477745}
Committed:
https://chromium.googlesource.com/chromium/src/+/8abf64aff97080b772d924ebf79d...
==========

[commit-bot: I haz the power, 2017-06-07 20:19:08]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/8abf64aff97080b772d924ebf79d...