Optimized HostPortPair::ToString and CookieMonster::GetAllCookiesForURLWithOptions.

Optimized HostPortPair::ToString and CookieMonster::GetAllCookiesForURLWithOptions.

- Removed GetFlattenedSpdyServer and replaced
its calls with host_port_pair.ToString().
- Optimized HostPortPair::ToString() by substituting StringPrintf with string concatenation (reduces the number of temporary allocations).
- Added a call to reserve() on cookies to minimize the number of grows.

BUG=

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

[Georges Khalil, 2014-12-01 16:13:16]

georgesak@chromium.org changed reviewers:
+ agl@chromium.org

[Georges Khalil, 2014-12-01 16:13:17]

PTAL.

[Ryan Sleevi, 2014-12-01 16:28:50]

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

[Ryan Sleevi, 2014-12-01 16:28:51]

https://codereview.chromium.org/745373002/diff/1/net/http/http_server_propert...
File net/http/http_server_properties_impl.cc (right):

https://codereview.chromium.org/745373002/diff/1/net/http/http_server_propert...
net/http/http_server_properties_impl.cc:138:
spdy_server.append(base::IntToString(host_port_pair.port()));
Not LGTM.

I don't like line 135 because, as a programming pattern, it requires me to
carefully reason whether .length() is safe to add +7 to it and what the
implications are if it isn't. My code review pattern matching is to hone in on
this, and any code full of surprises, especially of a "is it secure or not", is
bad.

I'm not sure why this code does append, append, StringAppendF, compared with
HostPortPair's StringPrintF,
https://code.google.com/p/chromium/codesearch#chromium/src/net/base/host_port...
, but I feel like if you're going to change this method, changing it to match
HostPortPair (with the special case of what I presume is bracketed IPv6 vs
unbracketed; if there isn't a special case, it should be using HostPortPair
ToString()) is a far easier change to reason about.

[Ryan Sleevi, 2014-12-01 16:29:41]

rsleevi@chromium.org changed reviewers:
+ rch@chromium.org
- agl@chromium.org

[Ryan Sleevi, 2014-12-01 16:29:41]

-agl (to save him review load)
+rch (who can also answer about the HostPortPair behaviour)

[Ryan Hamilton, 2014-12-01 20:14:53]

Please also update the CL description to be explicit about the changes you're
making.

https://codereview.chromium.org/745373002/diff/1/net/http/http_server_propert...
File net/http/http_server_properties_impl.cc (right):

https://codereview.chromium.org/745373002/diff/1/net/http/http_server_propert...
net/http/http_server_properties_impl.cc:138:
spdy_server.append(base::IntToString(host_port_pair.port()));
On 2014/12/01 16:28:51, Ryan Sleevi wrote:
> Not LGTM.
> 
> I don't like line 135 because, as a programming pattern, it requires me to
> carefully reason whether .length() is safe to add +7 to it and what the
> implications are if it isn't. My code review pattern matching is to hone in on
> this, and any code full of surprises, especially of a "is it secure or not",
is
> bad.
> 
> I'm not sure why this code does append, append, StringAppendF, compared with
> HostPortPair's StringPrintF,
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/base/host_port...
> , but I feel like if you're going to change this method, changing it to match
> HostPortPair (with the special case of what I presume is bracketed IPv6 vs
> unbracketed; if there isn't a special case, it should be using HostPortPair
> ToString()) is a far easier change to reason about.

I agree with everything sleevi said. I'd be inclined to simply use
host_port_pair.ToString() instead of GetFlattenedSpdyServer in the two places
that it is called.

[Georges Khalil, 2014-12-02 16:00:19]

Patchset #2 (id:20001) has been deleted

[Georges Khalil, 2014-12-02 16:00:26]

Patchset #2 (id:40001) has been deleted

[Georges Khalil, 2014-12-02 16:07:58]

PTAL.

As per both Ryan's comments, I have removed GetFlattenedSpdyServer and replaced
its calls with host_port_pair.ToString().

I have also modified host_port_pair.ToString() to replace StringPrintf with
calls to +=, which yields better performance (same as append) while, IMO,
maintaining good readability.

As far as performance goes, we ran some benchmarks and that change more than
doubles the speed of host_port_pair.ToString() (new function is ~55% faster).
Also, another thing to note is that +=':' (or append(1, ':')) is around ~25%
faster than +=":" (or append(":")). These benchmarks were done on Windows, using
MSVC2013.

[Ryan Sleevi, 2014-12-02 16:13:19]

On 2014/12/02 16:07:58, Georges Khalil wrote:
> PTAL.
> 
> As per both Ryan's comments, I have removed GetFlattenedSpdyServer and
replaced
> its calls with host_port_pair.ToString().

If this is protocol correct (and I defer to rch@), fantastic.

> 
> I have also modified host_port_pair.ToString() to replace StringPrintf with
> calls to +=, which yields better performance (same as append) while, IMO,
> maintaining good readability.

I'm don't entirely agree with this, but I fully realize readability is
subjective.

> 
> As far as performance goes, we ran some benchmarks and that change more than
> doubles the speed of host_port_pair.ToString() (new function is ~55% faster).
> Also, another thing to note is that +=':' (or append(1, ':')) is around ~25%
> faster than +=":" (or append(":")). These benchmarks were done on Windows,
using
> MSVC2013.

This is exactly the degree of microoptimization that I think is premature and
dangerous, and if it's the line of reasoning that is to be used for future
changes, I'm deeply uncomfortable with it.

I guess I can say "lgtm with result, but not with reasoning", but I want to make
it clear my objection here and that future microoptimizations like this seem...
problematic.

There's plenty of low hanging fruit of the "omgwtfbbq" nature that we should
focus on, and .append() / += chains can be quite unreadable for larger strings.

[Georges Khalil, 2014-12-02 16:21:20]

On 2014/12/02 16:13:19, Ryan Sleevi wrote:
> On 2014/12/02 16:07:58, Georges Khalil wrote:
> > PTAL.
> > 
> > As per both Ryan's comments, I have removed GetFlattenedSpdyServer and
> replaced
> > its calls with host_port_pair.ToString().
> 
> If this is protocol correct (and I defer to rch@), fantastic.
> 
> > 
> > I have also modified host_port_pair.ToString() to replace StringPrintf with
> > calls to +=, which yields better performance (same as append) while, IMO,
> > maintaining good readability.
> 
> I'm don't entirely agree with this, but I fully realize readability is
> subjective.
> 
> > 
> > As far as performance goes, we ran some benchmarks and that change more than
> > doubles the speed of host_port_pair.ToString() (new function is ~55%
faster).
> > Also, another thing to note is that +=':' (or append(1, ':')) is around ~25%
> > faster than +=":" (or append(":")). These benchmarks were done on Windows,
> using
> > MSVC2013.
> 
> This is exactly the degree of microoptimization that I think is premature and
> dangerous, and if it's the line of reasoning that is to be used for future
> changes, I'm deeply uncomfortable with it.
> 
> I guess I can say "lgtm with result, but not with reasoning", but I want to
make
> it clear my objection here and that future microoptimizations like this
seem...
> problematic.
> 
> There's plenty of low hanging fruit of the "omgwtfbbq" nature that we should
> focus on, and .append() / += chains can be quite unreadable for larger
strings.

I agree that a long chain of appends / += can get hard to read. That is why we
are looking at alternatives (StrCat and strings::Substittute for example).

In this case, I still think that readability does not suffer much and the
performance gains are considerable (over twice as fast).

[Ryan Hamilton, 2014-12-02 16:58:19]

As Both Ryan's said, please update the CL description to be much more specific
about what you're doing and why. :>

On 2014/12/02 16:13:19, Ryan Sleevi wrote:
> On 2014/12/02 16:07:58, Georges Khalil wrote:
> > PTAL.
> > 
> > As per both Ryan's comments, I have removed GetFlattenedSpdyServer and
> replaced
> > its calls with host_port_pair.ToString().
> 
> If this is protocol correct (and I defer to rch@), fantastic.

This looks correct to me.

> > I have also modified host_port_pair.ToString() to replace StringPrintf with
> > calls to +=, which yields better performance (same as append) while, IMO,
> > maintaining good readability.
> 
> I'm don't entirely agree with this, but I fully realize readability is
> subjective.

Admittedly, we're only talking about a few lines, but I agree with sleevi. The
old code is more readable to me.

> > As far as performance goes, we ran some benchmarks and that change more than
> > doubles the speed of host_port_pair.ToString() (new function is ~55%
faster).
> > Also, another thing to note is that +=':' (or append(1, ':')) is around ~25%
> > faster than +=":" (or append(":")). These benchmarks were done on Windows,
> using
> > MSVC2013.
> 
> This is exactly the degree of microoptimization that I think is premature and
> dangerous, and if it's the line of reasoning that is to be used for future
> changes, I'm deeply uncomfortable with it.
> 
> I guess I can say "lgtm with result, but not with reasoning", but I want to
make
> it clear my objection here and that future microoptimizations like this
seem...
> problematic.
> 
> There's plenty of low hanging fruit of the "omgwtfbbq" nature that we should
> focus on, and .append() / += chains can be quite unreadable for larger
strings.

I'm curious what motivated these changes. Was there some specific performance
problem which led you to try to optimize these two calls?

[Georges Khalil, 2014-12-02 17:51:10]

On 2014/12/02 16:58:19, Ryan Hamilton wrote:
> As Both Ryan's said, please update the CL description to be much more specific
> about what you're doing and why. :>

Done.

> 
> On 2014/12/02 16:13:19, Ryan Sleevi wrote:
> > On 2014/12/02 16:07:58, Georges Khalil wrote:
> > > PTAL.
> > > 
> > > As per both Ryan's comments, I have removed GetFlattenedSpdyServer and
> > replaced
> > > its calls with host_port_pair.ToString().
> > 
> > If this is protocol correct (and I defer to rch@), fantastic.
> 
> This looks correct to me.
> 
> > > I have also modified host_port_pair.ToString() to replace StringPrintf
with
> > > calls to +=, which yields better performance (same as append) while, IMO,
> > > maintaining good readability.
> > 
> > I'm don't entirely agree with this, but I fully realize readability is
> > subjective.
> 
> Admittedly, we're only talking about a few lines, but I agree with sleevi. The
> old code is more readable to me.
> 
> > > As far as performance goes, we ran some benchmarks and that change more
than
> > > doubles the speed of host_port_pair.ToString() (new function is ~55%
> faster).
> > > Also, another thing to note is that +=':' (or append(1, ':')) is around
~25%
> > > faster than +=":" (or append(":")). These benchmarks were done on Windows,
> > using
> > > MSVC2013.
> > 
> > This is exactly the degree of microoptimization that I think is premature
and
> > dangerous, and if it's the line of reasoning that is to be used for future
> > changes, I'm deeply uncomfortable with it.
> > 
> > I guess I can say "lgtm with result, but not with reasoning", but I want to
> make
> > it clear my objection here and that future microoptimizations like this
> seem...
> > problematic.
> > 
> > There's plenty of low hanging fruit of the "omgwtfbbq" nature that we should
> > focus on, and .append() / += chains can be quite unreadable for larger
> strings.
> 
> I'm curious what motivated these changes. Was there some specific performance
> problem which led you to try to optimize these two calls?

These calls were shown to make a lot of calls to the heap allocator, while
profiling. That is why we are proposing these changes, which reduces the number
of temporary string allocations and string growing.

[Ryan Hamilton, 2014-12-02 18:03:09]

Thanks! LGTM

[Georges Khalil, 2014-12-02 22:35:46]

The CQ bit was checked by georgesak@chromium.org

[commit-bot: I haz the power, 2014-12-02 22:37:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/745373002/60001

[commit-bot: I haz the power, 2014-12-02 23:52:19]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2014-12-02 23:53:17]

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