Replace the existing SpdyHeaderBlock typedef with a class.

Replace the existing SpdyHeaderBlock typedef with a class.

SpdyHeaderBlock is currently a map<string, string>. This change replaces the
typedef with a class that
* behaves like a linked_hash_map<StringPiece, StringPiece>, that is, preserves
  insertion order,
* and also manages memory containing the header names and values, resulting in
  much fewer allocations.

String construction and destruction showed up in the CPU profile under heavy
loads, and this class addresses this performance issue.

This CL includes server change 100829267 by rjshade (for
net/spdy/hpack/hpack_{en,de}coder_test.cc), which changed SpdyHeaderBlock to a
linked_hash_map, thus preserving order.  That change was not merged separately
because Chromium's linked_hash_map implementation does not support copy.

This CL also includes server changed 103293205 by birenroy, which introduces
SpdyHeaderBlock class managing large chunks of memory.

This CL also updates a large number of unittests and a small amount of
production code to account for header order being preserved.

BUG=443287
Committed: https://crrev.com/7ecc11208382bf462982af29d1680ee5eba2ded3
Cr-Commit-Position: refs/heads/master@{#351062}

[Bence, 2015-09-22 20:02:40]

Patchset #10 (id:180001) has been deleted

[Bence, 2015-09-22 20:02:45]

Patchset #9 (id:160001) has been deleted

[Bence, 2015-09-24 13:14:00]

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

[Bence, 2015-09-24 13:14:01]

Ryan: PTAL.  Thanks.

[Ryan Hamilton, 2015-09-24 17:54:06]

Couple small questions.

https://codereview.chromium.org/1357953002/diff/320001/net/spdy/spdy_framer_t...
File net/spdy/spdy_framer_test.cc (right):

https://codereview.chromium.org/1357953002/diff/320001/net/spdy/spdy_framer_t...
net/spdy/spdy_framer_test.cc:1066: EXPECT_EQ(99u, compressed_size4);
Why did this change?

https://codereview.chromium.org/1357953002/diff/320001/net/spdy/spdy_header_b...
File net/spdy/spdy_header_block.h (right):

https://codereview.chromium.org/1357953002/diff/320001/net/spdy/spdy_header_b...
net/spdy/spdy_header_block.h:23: using base::StringPiece;
I didn't think it was permitted by the style guide to use "using" in a .h file
because it contaminated the files in which it was included.

https://codereview.chromium.org/1357953002/diff/320001/net/spdy/spdy_test_uti...
File net/spdy/spdy_test_utils.h (right):

https://codereview.chromium.org/1357953002/diff/320001/net/spdy/spdy_test_uti...
net/spdy/spdy_test_utils.h:23: }
I wonder if it might just be easier to have a method like
bool CheckHeaderBlockHasValue(SpdyHeaderBlock block, StringPiece key,
StringPiece value);

Operator overloading is often a bad idea.

https://codereview.chromium.org/1357953002/diff/320001/net/tools/quic/quic_cl...
File net/tools/quic/quic_client_bin.cc (right):

https://codereview.chromium.org/1357953002/diff/320001/net/tools/quic/quic_cl...
net/tools/quic/quic_client_bin.cc:279: for (const auto& kv : header_block) {
Out of curiosity, what would the explicit type be here?

[Bence, 2015-09-24 21:14:52]

PTAL, thanks.

https://codereview.chromium.org/1357953002/diff/320001/net/spdy/spdy_framer_t...
File net/spdy/spdy_framer_test.cc (right):

https://codereview.chromium.org/1357953002/diff/320001/net/spdy/spdy_framer_t...
net/spdy/spdy_framer_test.cc:1066: EXPECT_EQ(99u, compressed_size4);
On 2015/09/24 17:54:06, Ryan Hamilton wrote:
> Why did this change?

The order of the header fields changed, therefore the raw header block changed,
resulting in a different compressed size.

https://codereview.chromium.org/1357953002/diff/320001/net/spdy/spdy_header_b...
File net/spdy/spdy_header_block.h (right):

https://codereview.chromium.org/1357953002/diff/320001/net/spdy/spdy_header_b...
net/spdy/spdy_header_block.h:23: using base::StringPiece;
On 2015/09/24 17:54:06, Ryan Hamilton wrote:
> I didn't think it was permitted by the style guide to use "using" in a .h file
> because it contaminated the files in which it was included.

Done.  You are right.

https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespaces

https://codereview.chromium.org/1357953002/diff/320001/net/spdy/spdy_test_uti...
File net/spdy/spdy_test_utils.h (right):

https://codereview.chromium.org/1357953002/diff/320001/net/spdy/spdy_test_uti...
net/spdy/spdy_test_utils.h:23: }
On 2015/09/24 17:54:06, Ryan Hamilton wrote:
> I wonder if it might just be easier to have a method like
> bool CheckHeaderBlockHasValue(SpdyHeaderBlock block, StringPiece key,
> StringPiece value);
> 
> Operator overloading is often a bad idea.

I would like to ask you to reconsider.  This operator allows smooth
EXPECT_EQ("GET", headers["method"])
statements in spdy_header_block_test, spdy_framer_test, spdy_http_utils_test,
and tools/flip_server/spdy_interface_test (see Patch Set 17).  Many of them. 
This reads much more easily than either
EXPECT_EQ("GET", static_cast<StringPiece>(headers["method"]))
or
EXPECT_TRUE(HeaderValueEquals(headers, "method", "GET"))
with the latter having the further disadvantage that on failure it does not
print the mismatching value.

Out of curiosity, why is overloading a bad idea, what are the possible pitfalls?
 Maybe there is a way to mitigate them.

By the way I was looking at testing/gtest/src/gtest.cc to see if there is any
helper comparison function that can be overloaded (like there is an overloadable
pretty print function), but it does not seem like the case.

https://codereview.chromium.org/1357953002/diff/320001/net/tools/quic/quic_cl...
File net/tools/quic/quic_client_bin.cc (right):

https://codereview.chromium.org/1357953002/diff/320001/net/tools/quic/quic_cl...
net/tools/quic/quic_client_bin.cc:279: for (const auto& kv : header_block) {
On 2015/09/24 17:54:06, Ryan Hamilton wrote:
> Out of curiosity, what would the explicit type be here?

pair<StringPiece, StringPiece>.  Same in quic_simple_client_bin.cc.  If you need
to ask, maybe it would be better do spell it out explicitly.  I'm not a big fan
of auto anyway, but this is how it got landed internally.

[Ryan Hamilton, 2015-09-24 22:25:55]

lgtm

https://codereview.chromium.org/1357953002/diff/320001/net/spdy/spdy_test_uti...
File net/spdy/spdy_test_utils.h (right):

https://codereview.chromium.org/1357953002/diff/320001/net/spdy/spdy_test_uti...
net/spdy/spdy_test_utils.h:23: }
On 2015/09/24 21:14:51, Bence wrote:
> On 2015/09/24 17:54:06, Ryan Hamilton wrote:
> > I wonder if it might just be easier to have a method like
> > bool CheckHeaderBlockHasValue(SpdyHeaderBlock block, StringPiece key,
> > StringPiece value);
> > 
> > Operator overloading is often a bad idea.
> 
> I would like to ask you to reconsider.  This operator allows smooth
> EXPECT_EQ("GET", headers["method"])
> statements in spdy_header_block_test, spdy_framer_test, spdy_http_utils_test,
> and tools/flip_server/spdy_interface_test (see Patch Set 17).  Many of them. 
> This reads much more easily than either
> EXPECT_EQ("GET", static_cast<StringPiece>(headers["method"]))
> or
> EXPECT_TRUE(HeaderValueEquals(headers, "method", "GET"))
> with the latter having the further disadvantage that on failure it does not
> print the mismatching value.

*grumble* :> Ok, fair enough. In particular your point about not printing the
mismatch convinces me.

> Out of curiosity, why is overloading a bad idea, what are the possible
pitfalls?
>  Maybe there is a way to mitigate them.
> 
> By the way I was looking at testing/gtest/src/gtest.cc to see if there is any
> helper comparison function that can be overloaded (like there is an
overloadable
> pretty print function), but it does not seem like the case.

In general:

https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

But what you've done is definitely on the sane side of the fence, I'm just
basically allergic. But your point is compelling.

https://codereview.chromium.org/1357953002/diff/320001/net/tools/quic/quic_cl...
File net/tools/quic/quic_client_bin.cc (right):

https://codereview.chromium.org/1357953002/diff/320001/net/tools/quic/quic_cl...
net/tools/quic/quic_client_bin.cc:279: for (const auto& kv : header_block) {
On 2015/09/24 21:14:51, Bence wrote:
> On 2015/09/24 17:54:06, Ryan Hamilton wrote:
> > Out of curiosity, what would the explicit type be here?
> 
> pair<StringPiece, StringPiece>.  Same in quic_simple_client_bin.cc.  If you
need
> to ask, maybe it would be better do spell it out explicitly.  I'm not a big
fan
> of auto anyway, but this is how it got landed internally.

meh. If it were me, I would spell it out, but it's not the end of the world in
this case.

[Bence, 2015-09-25 11:27:21]

The CQ bit was checked by bnc@chromium.org

[Bence, 2015-09-25 11:27:29]

The CQ bit was unchecked by bnc@chromium.org

[Bence, 2015-09-25 11:32:44]

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

[Bence, 2015-09-25 11:32:45]

dgozman: PTAL at content/browser/devtools/devtools_netlog_observer.cc.  Thank
you.

Ryan: Thanks.

[dgozman, 2015-09-25 18:10:42]

devtools lgtm

https://codereview.chromium.org/1357953002/diff/360001/net/spdy/spdy_header_b...
File net/spdy/spdy_header_block.h (right):

https://codereview.chromium.org/1357953002/diff/360001/net/spdy/spdy_header_b...
net/spdy/spdy_header_block.h:36: typedef linked_hash_map<base::StringPiece,
base::StringPiece> MapType;
Style guide suggests to use "using" instead of "typedef" for new code.

[Bence, 2015-09-25 18:36:12]

The CQ bit was checked by bnc@chromium.org

[commit-bot: I haz the power, 2015-09-25 18:36:36]

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

[Bence, 2015-09-25 18:37:46]

Thanks.

https://codereview.chromium.org/1357953002/diff/360001/net/spdy/spdy_header_b...
File net/spdy/spdy_header_block.h (right):

https://codereview.chromium.org/1357953002/diff/360001/net/spdy/spdy_header_b...
net/spdy/spdy_header_block.h:36: typedef linked_hash_map<base::StringPiece,
base::StringPiece> MapType;
On 2015/09/25 18:10:42, dgozman wrote:
> Style guide suggests to use "using" instead of "typedef" for new code.

Like in http://en.cppreference.com/w/cpp/language/type_alias?  I'm sorry, I
cannot find it either in the style guide or in the list of whitelisted Chromium
C++11 features.  Could you please point me to the appropriate part of the style
guide?  Thank you.

[dgozman, 2015-09-25 18:42:47]

On 2015/09/25 18:37:46, Bence wrote:
> Thanks.
> 
>
https://codereview.chromium.org/1357953002/diff/360001/net/spdy/spdy_header_b...
> File net/spdy/spdy_header_block.h (right):
> 
>
https://codereview.chromium.org/1357953002/diff/360001/net/spdy/spdy_header_b...
> net/spdy/spdy_header_block.h:36: typedef linked_hash_map<base::StringPiece,
> base::StringPiece> MapType;
> On 2015/09/25 18:10:42, dgozman wrote:
> > Style guide suggests to use "using" instead of "typedef" for new code.
> 
> Like in http://en.cppreference.com/w/cpp/language/type_alias  I'm sorry, I
> cannot find it either in the style guide or in the list of whitelisted
Chromium
> C++11 features.  Could you please point me to the appropriate part of the
style
> guide?  Thank you.

See first entry "Aliases" at https://chromium-cpp.appspot.com. The resolution
was "Use instead of typedef, unless the header needs to be compatible with C."

[Bence, 2015-09-25 18:44:53]

The CQ bit was unchecked by bnc@chromium.org

[Bence, 2015-09-25 18:47:46]

On 2015/09/25 18:42:47, dgozman wrote:
> On 2015/09/25 18:37:46, Bence wrote:
> > Thanks.
> > 
> >
>
https://codereview.chromium.org/1357953002/diff/360001/net/spdy/spdy_header_b...
> > File net/spdy/spdy_header_block.h (right):
> > 
> >
>
https://codereview.chromium.org/1357953002/diff/360001/net/spdy/spdy_header_b...
> > net/spdy/spdy_header_block.h:36: typedef linked_hash_map<base::StringPiece,
> > base::StringPiece> MapType;
> > On 2015/09/25 18:10:42, dgozman wrote:
> > > Style guide suggests to use "using" instead of "typedef" for new code.
> > 
> > Like in http://en.cppreference.com/w/cpp/language/type_alias  I'm sorry, I
> > cannot find it either in the style guide or in the list of whitelisted
> Chromium
> > C++11 features.  Could you please point me to the appropriate part of the
> style
> > guide?  Thank you.
> 
> See first entry "Aliases" at https://chromium-cpp.appspot.com. The resolution
> was "Use instead of typedef, unless the header needs to be compatible with C."

Sorry I missed that.  Thanks.

[Bence, 2015-09-25 21:49:44]

The CQ bit was checked by bnc@chromium.org

[Bence, 2015-09-25 21:49:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
dgozman@chromium.org, rch@chromium.org
Link to the patchset: https://codereview.chromium.org/1357953002/#ps400001
(title: "s/typedef/using/g")

[commit-bot: I haz the power, 2015-09-25 21:50:19]

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

[commit-bot: I haz the power, 2015-09-25 23:03:18]

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

[commit-bot: I haz the power, 2015-09-25 23:03:19]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Bence, 2015-09-28 12:15:13]

The CQ bit was checked by bnc@chromium.org

[Bence, 2015-09-28 12:15:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
dgozman@chromium.org, rch@chromium.org
Link to the patchset: https://codereview.chromium.org/1357953002/#ps420001
(title: "Add NET_EXPORT to fix compile error on win_chromium_compile_dbg_ng.")

[commit-bot: I haz the power, 2015-09-28 12:15:23]

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

[commit-bot: I haz the power, 2015-09-28 13:23:00]

Committed patchset #20 (id:420001)

[commit-bot: I haz the power, 2015-09-28 13:23:44]

Patchset 20 (id:??) landed as
https://crrev.com/7ecc11208382bf462982af29d1680ee5eba2ded3
Cr-Commit-Position: refs/heads/master@{#351062}