QUIC_VERSION_21: headers and crypto streams are now flow controlled at

QUIC_VERSION_21: headers and crypto streams are now flow controlled at
the stream level, but do not contribute to connection level flow
control.

Merge internal change: 70318947

R=rch@chromium.org

[Ryan Hamilton, 2014-07-02 19:24:51]

lgtm

[wtc, 2014-07-02 22:38:31]

Patch set 1 LGTM.

Some of my suggested fixes for nits may not be worth making.

https://codereview.chromium.org/368803003/diff/1/net/quic/reliable_quic_strea...
File net/quic/reliable_quic_stream.cc (right):

https://codereview.chromium.org/368803003/diff/1/net/quic/reliable_quic_strea...
net/quic/reliable_quic_stream.cc:336: if
(stream_contributes_to_connection_flow_control_) {

Nit: we can merge these two if statements as follows:

  if (stream_contributes_to_connection_flow_control_) {
    connection_flow_controller_->MaybeSendBlocked();
    // If we are connection level flow control blocked, then add the stream
    // to the write blocked list. It will be given a chance to write when a
    // connection level WINDOW_UPDATE arrives.
    if (connection_flow_controller_->IsBlocked() &&
        !flow_controller_.IsBlocked()) {
      session_->MarkWriteBlocked(id(), EffectivePriority());
    }
  }

Alternatively, we can do an early return:

  if (!stream_contributes_to_connection_flow_control_) {
    return;
  }
  connection_flow_controller_->MaybeSendBlocked();
  ...

https://codereview.chromium.org/368803003/diff/1/net/quic/reliable_quic_strea...
net/quic/reliable_quic_stream.cc:500: if
(stream_contributes_to_connection_flow_control_) {

Nit: merge this inner if with the if on line 496.

https://codereview.chromium.org/368803003/diff/1/net/quic/reliable_quic_strea...
net/quic/reliable_quic_stream.cc:538: return stream_flow_control_blocked ||
connecton_flow_control_blocked;

Nit: the new code loses some of the short circuit. We can also do:

  if (flow_controller_.IsBlocked()) {
    return true;
  }
  return stream_contributes_to_connection_flow_control_ &&
         connection_flow_controller_->IsBlocked();

[jar (doing other things), 2014-07-14 21:49:23]

This looks to be a nice compact CL.... It seems very plausible to get this
landed into Dev (RSN), so it can progress to beta shortly.

https://codereview.chromium.org/368803003/diff/1/net/quic/reliable_quic_strea...
File net/quic/reliable_quic_stream.cc (right):

https://codereview.chromium.org/368803003/diff/1/net/quic/reliable_quic_strea...
net/quic/reliable_quic_stream.cc:336: if
(stream_contributes_to_connection_flow_control_) {
On 2014/07/02 22:38:31, wtc wrote:
> 
> Nit: we can merge these two if statements as follows:
> 
>   if (stream_contributes_to_connection_flow_control_) {
>     connection_flow_controller_->MaybeSendBlocked();
>     // If we are connection level flow control blocked, then add the stream
>     // to the write blocked list. It will be given a chance to write when a
>     // connection level WINDOW_UPDATE arrives.
>     if (connection_flow_controller_->IsBlocked() &&
>         !flow_controller_.IsBlocked()) {
>       session_->MarkWriteBlocked(id(), EffectivePriority());
>     }
>   }
> 
> Alternatively, we can do an early return:
> 
>   if (!stream_contributes_to_connection_flow_control_) {
>     return;
>   }
>   connection_flow_controller_->MaybeSendBlocked();
>   ...

+1 for early return suggestion.

https://codereview.chromium.org/368803003/diff/1/net/tools/quic/end_to_end_te...
File net/tools/quic/end_to_end_test.cc (right):

https://codereview.chromium.org/368803003/diff/1/net/tools/quic/end_to_end_te...
net/tools/quic/end_to_end_test.cc:1328: server_connection_flow_controller));
nit: either indent 4 from the "EXP..."  or 4 from the "("

[ramant (doing other things), 2014-07-16 19:43:09]

Fixed the comments in CL: https://codereview.chromium.org/394053003/ and will
merge that back into the internal source tree.

https://codereview.chromium.org/368803003/diff/1/net/quic/reliable_quic_strea...
File net/quic/reliable_quic_stream.cc (right):

https://codereview.chromium.org/368803003/diff/1/net/quic/reliable_quic_strea...
net/quic/reliable_quic_stream.cc:336: if
(stream_contributes_to_connection_flow_control_) {
On 2014/07/02 22:38:31, wtc wrote:
> 
> Nit: we can merge these two if statements as follows:
> 
>   if (stream_contributes_to_connection_flow_control_) {
>     connection_flow_controller_->MaybeSendBlocked();
>     // If we are connection level flow control blocked, then add the stream
>     // to the write blocked list. It will be given a chance to write when a
>     // connection level WINDOW_UPDATE arrives.
>     if (connection_flow_controller_->IsBlocked() &&
>         !flow_controller_.IsBlocked()) {
>       session_->MarkWriteBlocked(id(), EffectivePriority());
>     }
>   }
> 
> Alternatively, we can do an early return:
> 
>   if (!stream_contributes_to_connection_flow_control_) {
>     return;
>   }
>   connection_flow_controller_->MaybeSendBlocked();
>   ...

Done.

https://codereview.chromium.org/368803003/diff/1/net/quic/reliable_quic_strea...
net/quic/reliable_quic_stream.cc:336: if
(stream_contributes_to_connection_flow_control_) {
On 2014/07/14 21:49:23, jar wrote:
> On 2014/07/02 22:38:31, wtc wrote:
> > 
> > Nit: we can merge these two if statements as follows:
> > 
> >   if (stream_contributes_to_connection_flow_control_) {
> >     connection_flow_controller_->MaybeSendBlocked();
> >     // If we are connection level flow control blocked, then add the stream
> >     // to the write blocked list. It will be given a chance to write when a
> >     // connection level WINDOW_UPDATE arrives.
> >     if (connection_flow_controller_->IsBlocked() &&
> >         !flow_controller_.IsBlocked()) {
> >       session_->MarkWriteBlocked(id(), EffectivePriority());
> >     }
> >   }
> > 
> > Alternatively, we can do an early return:
> > 
> >   if (!stream_contributes_to_connection_flow_control_) {
> >     return;
> >   }
> >   connection_flow_controller_->MaybeSendBlocked();
> >   ...
> 
> +1 for early return suggestion.

Done.

https://codereview.chromium.org/368803003/diff/1/net/quic/reliable_quic_strea...
net/quic/reliable_quic_stream.cc:500: if
(stream_contributes_to_connection_flow_control_) {
On 2014/07/02 22:38:30, wtc wrote:
> 
> Nit: merge this inner if with the if on line 496.

Reworked the if statements.

https://codereview.chromium.org/368803003/diff/1/net/quic/reliable_quic_strea...
net/quic/reliable_quic_stream.cc:538: return stream_flow_control_blocked ||
connecton_flow_control_blocked;
On 2014/07/02 22:38:31, wtc wrote:
> 
> Nit: the new code loses some of the short circuit. We can also do:
> 
>   if (flow_controller_.IsBlocked()) {
>     return true;
>   }
>   return stream_contributes_to_connection_flow_control_ &&
>          connection_flow_controller_->IsBlocked();

Done.

https://codereview.chromium.org/368803003/diff/1/net/tools/quic/end_to_end_te...
File net/tools/quic/end_to_end_test.cc (right):

https://codereview.chromium.org/368803003/diff/1/net/tools/quic/end_to_end_te...
net/tools/quic/end_to_end_test.cc:1328: server_connection_flow_controller));
On 2014/07/14 21:49:23, jar wrote:
> nit: either indent 4 from the "EXP..."  or 4 from the "("

Done.