Avoids the "re-encode HPACK as SPDY3" step.

Avoids the "re-encode HPACK as SPDY3" step.

This change causes header data to be propagated from SpdyFramer to the
visitor as logical key/value pairs. This avoids the "re-encode HPACK as
SPDY3" step, and therefore should improve performance.

Now with QUIC support!

This CL lands server change 120879191 by birenroy.

BUG=488484
Committed: https://crrev.com/600f0c57e92bfbd0e27f889d00d4f3089c464077
Cr-Commit-Position: refs/heads/master@{#394445}

Committed: https://crrev.com/af379649901296f53bb985e4f99996e8c6beb4fa
Cr-Commit-Position: refs/heads/master@{#395958}

[dahollings, 2016-05-06 20:28:45]

Patchset #1 (id:1) has been deleted

[dahollings, 2016-05-06 20:28:54]

Patchset #1 (id:20001) has been deleted

[dahollings, 2016-05-09 21:32:29]

Patchset #1 (id:40001) has been deleted

[dahollings, 2016-05-13 20:53:14]

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

[Ryan Hamilton, 2016-05-13 22:03:15]

https://codereview.chromium.org/1961573002/diff/160001/net/http/bidirectional...
File net/http/bidirectional_stream_unittest.cc (right):

https://codereview.chromium.org/1961573002/diff/160001/net/http/bidirectional...
net/http/bidirectional_stream_unittest.cc:1198: EXPECT_EQ(0,
delegate->GetTotalReceivedBytes());
Can you explain why this expectation is changing?

https://codereview.chromium.org/1961573002/diff/160001/net/spdy/header_coales...
File net/spdy/header_coalescer.h (right):

https://codereview.chromium.org/1961573002/diff/160001/net/spdy/header_coales...
net/spdy/header_coalescer.h:14: class NET_EXPORT_PRIVATE HeaderCoalescer :
public SpdyHeadersHandlerInterface {
Is this class from the internal repository? It looks very similar to
SpdyUtils::CopyAndValidateHeaders. I'm nervous about chrome using different code
to assemble headers than other classes. Can we share code instead?

[dahollings, 2016-05-16 22:00:17]

https://codereview.chromium.org/1961573002/diff/160001/net/http/bidirectional...
File net/http/bidirectional_stream_unittest.cc (right):

https://codereview.chromium.org/1961573002/diff/160001/net/http/bidirectional...
net/http/bidirectional_stream_unittest.cc:1198: EXPECT_EQ(0,
delegate->GetTotalReceivedBytes());
On 2016/05/13 22:03:15, Ryan Hamilton wrote:
> Can you explain why this expectation is changing?

The coalescer (new file) was catching the invalid uppercase header before
total_received_bytes was getting incremented in OnHeaders. I then realized that
we don't need to protect against uppercase headers in the coalescer, so, upon
removing that, we fall back to the previous check which updates this debug stat
properly. Thanks for the catch!

https://codereview.chromium.org/1961573002/diff/160001/net/spdy/header_coales...
File net/spdy/header_coalescer.h (right):

https://codereview.chromium.org/1961573002/diff/160001/net/spdy/header_coales...
net/spdy/header_coalescer.h:14: class NET_EXPORT_PRIVATE HeaderCoalescer :
public SpdyHeadersHandlerInterface {
On 2016/05/13 22:03:15, Ryan Hamilton wrote:
> Is this class from the internal repository? It looks very similar to
> SpdyUtils::CopyAndValidateHeaders. I'm nervous about chrome using different
code
> to assemble headers than other classes. Can we share code instead?

This class is not from the internal repository-- we created it as an analogue to
the internal spdy_headers_handler class. I see your point about the similarities
with SpdyUtils::CopyAndValidateHeaders, but I'm not sure how best to share the
code. The new changes need a class implementing the handler interface to pass to
hpack_decoder, as it now builds a HeaderBlock up by repeated calls to OnHeader.
CopyAndValidate receives a QuicHeaderList from the beginning. Suggestions will
be welcome!

[Ryan Hamilton, 2016-05-16 22:08:43]

https://codereview.chromium.org/1961573002/diff/160001/net/spdy/header_coales...
File net/spdy/header_coalescer.h (right):

https://codereview.chromium.org/1961573002/diff/160001/net/spdy/header_coales...
net/spdy/header_coalescer.h:14: class NET_EXPORT_PRIVATE HeaderCoalescer :
public SpdyHeadersHandlerInterface {
On 2016/05/16 22:00:17, dahollings wrote:
> On 2016/05/13 22:03:15, Ryan Hamilton wrote:
> > Is this class from the internal repository? It looks very similar to
> > SpdyUtils::CopyAndValidateHeaders. I'm nervous about chrome using different
> code
> > to assemble headers than other classes. Can we share code instead?
> 
> This class is not from the internal repository-- we created it as an analogue
to
> the internal spdy_headers_handler class. 

*nod* And since that traffics in BalsaHeaders, it's be a poor fit here :(

> I see your point about the similarities
> with SpdyUtils::CopyAndValidateHeaders, but I'm not sure how best to share the
> code. The new changes need a class implementing the handler interface to pass
to
> hpack_decoder, as it now builds a HeaderBlock up by repeated calls to
OnHeader.
> CopyAndValidate receives a QuicHeaderList from the beginning. Suggestions will
> be welcome!

Since we have Chrome, and the QUIC toy client and server parsing headers, it
would be good for them to all share the same implementation. I'm not sure that I
understand what you mean about the difference between the needs of
QuicChromiumClientStream (which in this CL uses the coalescer) and
QuicSpdyClientStream (which uses SpdyUtils::CopyAndValidateHeaders). I'm sure
*you* understand it, however :> Care to hop on VC and we can discuss at higher
bandwidth?

[dahollings, 2016-05-17 20:57:15]

On 2016/05/16 22:08:43, Ryan Hamilton wrote:
>
https://codereview.chromium.org/1961573002/diff/160001/net/spdy/header_coales...
> File net/spdy/header_coalescer.h (right):
> 
>
https://codereview.chromium.org/1961573002/diff/160001/net/spdy/header_coales...
> net/spdy/header_coalescer.h:14: class NET_EXPORT_PRIVATE HeaderCoalescer :
> public SpdyHeadersHandlerInterface {
> On 2016/05/16 22:00:17, dahollings wrote:
> > On 2016/05/13 22:03:15, Ryan Hamilton wrote:
> > > Is this class from the internal repository? It looks very similar to
> > > SpdyUtils::CopyAndValidateHeaders. I'm nervous about chrome using
different
> > code
> > > to assemble headers than other classes. Can we share code instead?
> > 
> > This class is not from the internal repository-- we created it as an
analogue
> to
> > the internal spdy_headers_handler class. 
> 
> *nod* And since that traffics in BalsaHeaders, it's be a poor fit here :(
> 
> > I see your point about the similarities
> > with SpdyUtils::CopyAndValidateHeaders, but I'm not sure how best to share
the
> > code. The new changes need a class implementing the handler interface to
pass
> to
> > hpack_decoder, as it now builds a HeaderBlock up by repeated calls to
> OnHeader.
> > CopyAndValidate receives a QuicHeaderList from the beginning. Suggestions
will
> > be welcome!
> 
> Since we have Chrome, and the QUIC toy client and server parsing headers, it
> would be good for them to all share the same implementation. I'm not sure that
I
> understand what you mean about the difference between the needs of
> QuicChromiumClientStream (which in this CL uses the coalescer) and
> QuicSpdyClientStream (which uses SpdyUtils::CopyAndValidateHeaders). I'm sure
> *you* understand it, however :> Care to hop on VC and we can discuss at higher
> bandwidth?

Ok, I made an attempt to resolve this per our discussion such that both quic
clients
will now use CopyAndValidate. I believe this is cleaner, thanks for suggesting
it.

This should now be ready for another review when convenient.

[Ryan Hamilton, 2016-05-17 22:33:05]

Woot! Thanks for doing this. LGTM

[dahollings, 2016-05-18 16:51:44]

The CQ bit was checked by dahollings@chromium.org

[commit-bot: I haz the power, 2016-05-18 16:51:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1961573002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1961573002/260001

[commit-bot: I haz the power, 2016-05-18 16:56:32]

Committed patchset #11 (id:260001)

[commit-bot: I haz the power, 2016-05-18 16:59:16]

Description was changed from

==========
Avoids the "re-encode HPACK as SPDY3" step.

This change causes header data to be propagated from SpdyFramer to the
visitor as logical key/value pairs. This avoids the "re-encode HPACK as
SPDY3" step, and therefore should improve performance.

Now with QUIC support!

This CL lands server change 120879191 by birenroy.

BUG=488484
==========

to

==========
Avoids the "re-encode HPACK as SPDY3" step.

This change causes header data to be propagated from SpdyFramer to the
visitor as logical key/value pairs. This avoids the "re-encode HPACK as
SPDY3" step, and therefore should improve performance.

Now with QUIC support!

This CL lands server change 120879191 by birenroy.

BUG=488484

Committed: https://crrev.com/600f0c57e92bfbd0e27f889d00d4f3089c464077
Cr-Commit-Position: refs/heads/master@{#394445}
==========

[commit-bot: I haz the power, 2016-05-18 16:59:18]

Patchset 11 (id:??) landed as
https://crrev.com/600f0c57e92bfbd0e27f889d00d4f3089c464077
Cr-Commit-Position: refs/heads/master@{#394445}

[Ryan Hamilton, 2016-05-19 21:11:45]

A revert of this CL (patchset #11 id:260001) has been created in
https://codereview.chromium.org/1997863002/ by rch@chromium.org.

The reason for reverting is:
https://bugs.chromium.org/p/chromium/issues/detail?id=613079#c4

Seems to break gmail :(.

[Buck, 2016-05-19 22:02:15]

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

[Buck, 2016-05-19 22:02:16]

Hi. Just want to note that it's caused me some problems in a change that I'm
working on (https://codereview.chromium.org/1989003002/) that this change
doesn't flag protect the way the internal version does.  Doubly so now that this
is being rolled back.

[dahollings, 2016-05-23 21:15:18]

https://codereview.chromium.org/1961573002/diff/260001/net/quic/spdy_utils.cc
File net/quic/spdy_utils.cc (right):

https://codereview.chromium.org/1961573002/diff/260001/net/quic/spdy_utils.cc...
net/quic/spdy_utils.cc:134: "%s%c%s", iter->second.as_string().c_str(), '\0',
p.second.c_str());
birenroy@ pointed out that this could be one possible source of the bug: If more
than two headers with the same key are reconstructed, all but the first and last
may get lost due to the interaction between nullchar delimiting, null terminated
strings, and stringprintf. The current change will cause this code to get hit
from quic_chromium_client_stream, whereas before it only appears to be used in
quic_simple_server_stream and quic_spdy_client_stream.

[Ryan Hamilton, 2016-05-24 16:08:29]

https://codereview.chromium.org/1961573002/diff/260001/net/quic/spdy_utils.cc
File net/quic/spdy_utils.cc (right):

https://codereview.chromium.org/1961573002/diff/260001/net/quic/spdy_utils.cc...
net/quic/spdy_utils.cc:134: "%s%c%s", iter->second.as_string().c_str(), '\0',
p.second.c_str());
On 2016/05/23 21:15:17, dahollings wrote:
> birenroy@ pointed out that this could be one possible source of the bug: If
more
> than two headers with the same key are reconstructed, all but the first and
last
> may get lost due to the interaction between nullchar delimiting, null
terminated
> strings, and stringprintf. The current change will cause this code to get hit
> from quic_chromium_client_stream, whereas before it only appears to be used in
> quic_simple_server_stream and quic_spdy_client_stream.

Ah, interesting. That sounds potentially problematic.

[dahollings, 2016-05-25 16:03:47]

Description was changed from

==========
Avoids the "re-encode HPACK as SPDY3" step.

This change causes header data to be propagated from SpdyFramer to the
visitor as logical key/value pairs. This avoids the "re-encode HPACK as
SPDY3" step, and therefore should improve performance.

Now with QUIC support!

This CL lands server change 120879191 by birenroy.

BUG=488484

Committed: https://crrev.com/600f0c57e92bfbd0e27f889d00d4f3089c464077
Cr-Commit-Position: refs/heads/master@{#394445}
==========

to

==========
Avoids the "re-encode HPACK as SPDY3" step.

This change causes header data to be propagated from SpdyFramer to the
visitor as logical key/value pairs. This avoids the "re-encode HPACK as
SPDY3" step, and therefore should improve performance.

Now with QUIC support!

This CL lands server change 120879191 by birenroy.

BUG=488484

Committed: https://crrev.com/600f0c57e92bfbd0e27f889d00d4f3089c464077
Cr-Commit-Position: refs/heads/master@{#394445}
==========

[dahollings, 2016-05-25 17:31:26]

The bug which caused the revert should now be fixed with
https://codereview.chromium.org/2009623002. I was able to locally repro
success/failure to login to gmail with/without that patch on top of this branch.

[Ryan Hamilton, 2016-05-25 18:48:32]

The CQ bit was checked by rch@chromium.org

[Ryan Hamilton, 2016-05-25 18:48:33]

lgtm

[commit-bot: I haz the power, 2016-05-25 18:48:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1961573002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1961573002/300001

[commit-bot: I haz the power, 2016-05-25 19:21:46]

Description was changed from

==========
Avoids the "re-encode HPACK as SPDY3" step.

This change causes header data to be propagated from SpdyFramer to the
visitor as logical key/value pairs. This avoids the "re-encode HPACK as
SPDY3" step, and therefore should improve performance.

Now with QUIC support!

This CL lands server change 120879191 by birenroy.

BUG=488484

Committed: https://crrev.com/600f0c57e92bfbd0e27f889d00d4f3089c464077
Cr-Commit-Position: refs/heads/master@{#394445}
==========

to

==========
Avoids the "re-encode HPACK as SPDY3" step.

This change causes header data to be propagated from SpdyFramer to the
visitor as logical key/value pairs. This avoids the "re-encode HPACK as
SPDY3" step, and therefore should improve performance.

Now with QUIC support!

This CL lands server change 120879191 by birenroy.

BUG=488484

Committed: https://crrev.com/600f0c57e92bfbd0e27f889d00d4f3089c464077
Cr-Commit-Position: refs/heads/master@{#394445}
==========

[commit-bot: I haz the power, 2016-05-25 19:21:48]

Committed patchset #13 (id:300001)

[commit-bot: I haz the power, 2016-05-25 19:22:59]

Description was changed from

==========
Avoids the "re-encode HPACK as SPDY3" step.

This change causes header data to be propagated from SpdyFramer to the
visitor as logical key/value pairs. This avoids the "re-encode HPACK as
SPDY3" step, and therefore should improve performance.

Now with QUIC support!

This CL lands server change 120879191 by birenroy.

BUG=488484

Committed: https://crrev.com/600f0c57e92bfbd0e27f889d00d4f3089c464077
Cr-Commit-Position: refs/heads/master@{#394445}
==========

to

==========
Avoids the "re-encode HPACK as SPDY3" step.

This change causes header data to be propagated from SpdyFramer to the
visitor as logical key/value pairs. This avoids the "re-encode HPACK as
SPDY3" step, and therefore should improve performance.

Now with QUIC support!

This CL lands server change 120879191 by birenroy.

BUG=488484

Committed: https://crrev.com/600f0c57e92bfbd0e27f889d00d4f3089c464077
Cr-Commit-Position: refs/heads/master@{#394445}

Committed: https://crrev.com/af379649901296f53bb985e4f99996e8c6beb4fa
Cr-Commit-Position: refs/heads/master@{#395958}
==========

[commit-bot: I haz the power, 2016-05-25 19:23:01]

Patchset 13 (id:??) landed as
https://crrev.com/af379649901296f53bb985e4f99996e8c6beb4fa
Cr-Commit-Position: refs/heads/master@{#395958}