FBTF: Move a bunch of code to the headers and remove includes.

FBTF: Move a bunch of code to the headers and remove includes.

BUG=none
TEST=compiles

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=60208

[Elliot Glaysher, 2010-09-21 20:46:20]

Lots of random little things. Some code moving, some #include removal (histogram
showed up again in a header that didn't need it), some adding default
constructors.

[Randy Smith (Not in Mondays), 2010-09-21 21:07:58]

On 2010/09/21 20:46:20, Elliot Glaysher wrote:
> Lots of random little things. Some code moving, some #include removal
(histogram
> showed up again in a header that didn't need it), some adding default
> constructors.

What are your goals, specifically with regard to cookie_monster.h and
cookie_monster.cc?  Your changes don't look like they'll break functionality,
but CanonicalCookies are constructed on some performance critical paths.  Could
you move the histogram inclusions but leave the inline construction alone?

[Elliot Glaysher, 2010-09-21 21:51:35]

On 2010/09/21 21:07:58, rdsmith wrote:
> What are your goals, specifically with regard to cookie_monster.h and
> cookie_monster.cc?

1) Style: We're already not supposed to have larger methods inlined and the
style guide explicitly calls out ctors/dtors for being deceptive in how large
they are. Almost none of our inlining is deliberate. (See the whole files where
we have non-templated (complex) classes defined entirely in their headers)
1b) You are correct that the practical impact here is small, but I've spent the
last quarter tracking down cases where it isn't. By doing style cleanup
everywhere, I'm hoping that new code written won't automatically/unthinkingly
try to inline ctors/dtors in the places where this does matter.
2) Shrink binary size of .o/.a on disk due to duplicate symbols (we sometimes
fail to link in 32-bit address space)
3) Shave inconsequential amounts off compile time (due to code generation)
4) These methods (probably) won't be inlined anyway due to their size. The dtor
will make 4 function calls min, and maybe 7 depending on whether the compiler
will try to inline some of the member destructors.

> Your changes don't look like they'll break functionality,
> but CanonicalCookies are constructed on some performance critical paths.

I really doubt that this will have an effect on runtime performance. The effects
of inlining are entirely overrated while their downsides are almost never
considered.

[Randy Smith (Not in Mondays), 2010-09-21 21:55:57]

On 2010/09/21 21:51:35, Elliot Glaysher wrote:
> On 2010/09/21 21:07:58, rdsmith wrote:
> > What are your goals, specifically with regard to cookie_monster.h and
> > cookie_monster.cc?
> 
> 1) Style: We're already not supposed to have larger methods inlined and the
> style guide explicitly calls out ctors/dtors for being deceptive in how large
> they are. Almost none of our inlining is deliberate. (See the whole files
where
> we have non-templated (complex) classes defined entirely in their headers)
> 1b) You are correct that the practical impact here is small, but I've spent
the
> last quarter tracking down cases where it isn't. By doing style cleanup
> everywhere, I'm hoping that new code written won't automatically/unthinkingly
> try to inline ctors/dtors in the places where this does matter.
> 2) Shrink binary size of .o/.a on disk due to duplicate symbols (we sometimes
> fail to link in 32-bit address space)
> 3) Shave inconsequential amounts off compile time (due to code generation)
> 4) These methods (probably) won't be inlined anyway due to their size. The
dtor
> will make 4 function calls min, and maybe 7 depending on whether the compiler
> will try to inline some of the member destructors.
> 
> > Your changes don't look like they'll break functionality,
> > but CanonicalCookies are constructed on some performance critical paths.
> 
> I really doubt that this will have an effect on runtime performance. The
effects
> of inlining are entirely overrated while their downsides are almost never
> considered.

All completely fair; I was more in the "This change is probably ok, but makes me
slightly uncomfortable, and you might be willing to just skip the parts that
make me uncomfortable, so I thought I'd ask."  So I'll fall back to practical
request: Could you run net_perftests --gtest_filter=CookieMonsterTest.* and make
sure there are no regressions from this change?  If not, you have my blessing.

[Randy Smith (Not in Mondays), 2010-09-21 21:58:03]

On 2010/09/21 21:51:35, Elliot Glaysher wrote:
> On 2010/09/21 21:07:58, rdsmith wrote:
> > What are your goals, specifically with regard to cookie_monster.h and
> > cookie_monster.cc?
> 
> 1) Style: We're already not supposed to have larger methods inlined and the
> style guide explicitly calls out ctors/dtors for being deceptive in how large
> they are. Almost none of our inlining is deliberate. (See the whole files
where
> we have non-templated (complex) classes defined entirely in their headers)
> 1b) You are correct that the practical impact here is small, but I've spent
the
> last quarter tracking down cases where it isn't. By doing style cleanup
> everywhere, I'm hoping that new code written won't automatically/unthinkingly
> try to inline ctors/dtors in the places where this does matter.
> 2) Shrink binary size of .o/.a on disk due to duplicate symbols (we sometimes
> fail to link in 32-bit address space)
> 3) Shave inconsequential amounts off compile time (due to code generation)
> 4) These methods (probably) won't be inlined anyway due to their size. The
dtor
> will make 4 function calls min, and maybe 7 depending on whether the compiler
> will try to inline some of the member destructors.
> 
> > Your changes don't look like they'll break functionality,
> > but CanonicalCookies are constructed on some performance critical paths.
> 
> I really doubt that this will have an effect on runtime performance. The
effects
> of inlining are entirely overrated while their downsides are almost never
> considered.

All completely fair.  I was more in the "these changes make me slightly
uncomfortable and they're a small part of what he's trying to do; maybe the
simplest thing is to get him to drop them" mode.  I'll switch over to the more
practical mode: Could you run net_perftests --gtest_filter=CookieMonsterTest.*
before/after your changes and confirm that there isn't any performance
degradation?  If not, you have my blessing.

[Randy Smith (Not in Mondays), 2010-09-21 22:01:59]

On 2010/09/21 21:58:03, rdsmith wrote:
> All completely fair.  I was more in the "these changes make me slightly
> uncomfortable and they're a small part of what he's trying to do; maybe the
> simplest thing is to get him to drop them" mode.  I'll switch over to the more
> practical mode: Could you run net_perftests --gtest_filter=CookieMonsterTest.*
> before/after your changes and confirm that there isn't any performance
> degradation?  If not, you have my blessing.
Sorry, that was ambiguous.  "If there are no performance degradations, you have
my blessing."

[Elliot Glaysher, 2010-09-21 22:35:49]

On 2010/09/21 21:58:03, rdsmith wrote:
> All completely fair.  I was more in the "these changes make me slightly
> uncomfortable and they're a small part of what he's trying to do; maybe the
> simplest thing is to get him to drop them" mode.

Apologies on my part, then. I've had a surprising amount of pushback on these
series of patches and have had to explain linkage, ctor/dtor synthesizing and
other aspects of C++ way too many times.

>  I'll switch over to the more
> practical mode: Could you run net_perftests --gtest_filter=CookieMonsterTest.*
> before/after your changes and confirm that there isn't any performance
> degradation?  If not, you have my blessing.

In a series of 5 runs, both binaries ran in the rough ballpark of 6500~7200ms. I
don't think this has any affect.

-- Elliot

[Randy Smith (Not in Mondays), 2010-09-22 17:35:40]

On 2010/09/21 22:35:49, Elliot Glaysher wrote:
> On 2010/09/21 21:58:03, rdsmith wrote:
> > All completely fair.  I was more in the "these changes make me slightly
> > uncomfortable and they're a small part of what he's trying to do; maybe the
> > simplest thing is to get him to drop them" mode.
> 
> Apologies on my part, then. I've had a surprising amount of pushback on these
> series of patches and have had to explain linkage, ctor/dtor synthesizing and
> other aspects of C++ way too many times.
> 
> >  I'll switch over to the more
> > practical mode: Could you run net_perftests
--gtest_filter=CookieMonsterTest.*
> > before/after your changes and confirm that there isn't any performance
> > degradation?  If not, you have my blessing.
> 
> In a series of 5 runs, both binaries ran in the rough ballpark of 6500~7200ms.
I
> don't think this has any affect.
> 
> -- Elliot

Elliot: Could you send me the output of the two sets of runs?  I'm actually
interested in how the individual tests vary in time; the time gap you mention
could be worrisome, if it's all around a single test.

[Elliot Glaysher, 2010-09-22 18:24:34]

On 2010/09/22 17:35:40, rdsmith wrote:
> On 2010/09/21 22:35:49, Elliot Glaysher wrote:
> > On 2010/09/21 21:58:03, rdsmith wrote:
> > > All completely fair.  I was more in the "these changes make me slightly
> > > uncomfortable and they're a small part of what he's trying to do; maybe
the
> > > simplest thing is to get him to drop them" mode.
> > 
> > Apologies on my part, then. I've had a surprising amount of pushback on
these
> > series of patches and have had to explain linkage, ctor/dtor synthesizing
and
> > other aspects of C++ way too many times.
> > 
> > >  I'll switch over to the more
> > > practical mode: Could you run net_perftests
> --gtest_filter=CookieMonsterTest.*
> > > before/after your changes and confirm that there isn't any performance
> > > degradation?  If not, you have my blessing.
> > 
> > In a series of 5 runs, both binaries ran in the rough ballpark of
6500~7200ms.
> I
> > don't think this has any affect.
> > 
> > -- Elliot
> 
> Elliot: Could you send me the output of the two sets of runs?  I'm actually
> interested in how the individual tests vary in time; the time gap you mention
> could be worrisome, if it's all around a single test.

http://www.corp.google.com/~erg/cm_all.tar.gz : 9 trials both before/after.

[Randy Smith (Not in Mondays), 2010-09-22 19:09:29]

On 2010/09/22 18:24:34, Elliot Glaysher wrote:
> On 2010/09/22 17:35:40, rdsmith wrote:
> > On 2010/09/21 22:35:49, Elliot Glaysher wrote:
> > > On 2010/09/21 21:58:03, rdsmith wrote:
> > > > All completely fair.  I was more in the "these changes make me slightly
> > > > uncomfortable and they're a small part of what he's trying to do; maybe
> the
> > > > simplest thing is to get him to drop them" mode.
> > > 
> > > Apologies on my part, then. I've had a surprising amount of pushback on
> these
> > > series of patches and have had to explain linkage, ctor/dtor synthesizing
> and
> > > other aspects of C++ way too many times.
> > > 
> > > >  I'll switch over to the more
> > > > practical mode: Could you run net_perftests
> > --gtest_filter=CookieMonsterTest.*
> > > > before/after your changes and confirm that there isn't any performance
> > > > degradation?  If not, you have my blessing.
> > > 
> > > In a series of 5 runs, both binaries ran in the rough ballpark of
> 6500~7200ms.
> > I
> > > don't think this has any affect.
> > > 
> > > -- Elliot
> > 
> > Elliot: Could you send me the output of the two sets of runs?  I'm actually
> > interested in how the individual tests vary in time; the time gap you
mention
> > could be worrisome, if it's all around a single test.
> 
> http://www.corp.google.com/%7Eerg/cm_all.tar.gz : 9 trials both before/after.

I hope you share my amusement that if anything, your changes have improved
performance (not a lot, and it's not clear whether significantly or not, but the
delta is positive).  LGTM.