Reduce size of generated extension FeatureProviders.

Reduce size of generated extension FeatureProviders.

The generated feature provider code was generating a lot of std::string
and std::vector objects from string literals and initializer lists.
Unfortunately, this results in inlining a lot of STL code: on an
official Linux build, extensions::APIFeatureProvider::APIFeatureProvider
was over 200KB of code. After this change, it's still larger than
expected, at just under 67KB, but it's much better than before. In all,
this saves about 275KB of binary size in an official Linux GN build.

BUG=631915
Committed: https://crrev.com/d8b8acff3d2d57e66b833a5632e3a993eb405277
Cr-Commit-Position: refs/heads/master@{#408350}

[dcheng, 2016-07-27 09:47:17]

Description was changed from

==========
Reduce size of generated extension FeatureProviders.

The generated feature provider code was generating a lot of std::string
and std::vector objects from string literals and initializer lists.
Unfortunately, this results in inlining a lot of STL code: on an
official Linux build, extensions::APIFeatureProvider::APIFeatureProvider
was over 200KB of code. After this change, it's <much smaller>.

BUG=631915
==========

to

==========
Reduce size of generated extension FeatureProviders.

The generated feature provider code was generating a lot of std::string
and std::vector objects from string literals and initializer lists.
Unfortunately, this results in inlining a lot of STL code: on an
official Linux build, extensions::APIFeatureProvider::APIFeatureProvider
was over 200KB of code. After this change, it's still larger than
expected, at just under 67KB, but it's much better than before.

BUG=631915
==========

[dcheng, 2016-07-27 09:47:17]

dcheng@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[dcheng, 2016-07-27 09:48:50]

FWIW, the other substantial savings will probably come from not instantiating a
std::unique_ptr<Feature> for each feature: clang generates a bunch of cleanup
code for each stack instance. I'll explore that in a followup CL.

https://codereview.chromium.org/2186893002/diff/1/extensions/common/features/...
File extensions/common/features/simple_feature.h (right):

https://codereview.chromium.org/2186893002/diff/1/extensions/common/features/...
extensions/common/features/simple_feature.h:114: void
set_command_line_switch(std::string&& command_line_switch);
This is the last thing that passes by rvalue: what do you think of just making
this consistent with set_name and passing by StringPiece?

[dcheng, 2016-07-27 09:49:00]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-27 09:49:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-27 10:43:54]

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

[commit-bot: I haz the power, 2016-07-27 10:43:54]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-07-27 13:05:13]

Description was changed from

==========
Reduce size of generated extension FeatureProviders.

The generated feature provider code was generating a lot of std::string
and std::vector objects from string literals and initializer lists.
Unfortunately, this results in inlining a lot of STL code: on an
official Linux build, extensions::APIFeatureProvider::APIFeatureProvider
was over 200KB of code. After this change, it's still larger than
expected, at just under 67KB, but it's much better than before.

BUG=631915
==========

to

==========
Reduce size of generated extension FeatureProviders.

The generated feature provider code was generating a lot of std::string
and std::vector objects from string literals and initializer lists.
Unfortunately, this results in inlining a lot of STL code: on an
official Linux build, extensions::APIFeatureProvider::APIFeatureProvider
was over 200KB of code. After this change, it's still larger than
expected, at just under 67KB, but it's much better than before. In all,
this saves about 275KB of binary size in an official Linux GN build.

BUG=631915
==========

[Devlin, 2016-07-27 15:36:49]

I'd like to test this locally when I get into the office (about an hour) to see
how this performance stacks up.  I went through a few different iterations, and
assigning directly with rvalue refs was the fastest I found, so I'm curious if
the push-backs will be slower.

https://codereview.chromium.org/2186893002/diff/1/extensions/common/features/...
File extensions/common/features/simple_feature.cc (right):

https://codereview.chromium.org/2186893002/diff/1/extensions/common/features/...
extensions/common/features/simple_feature.cc:672: blacklist_.clear();
These should only be called at feature construction, so all these parameters
*should* be empty.  I'd prefer to replace this with a DCHECK(blacklist_.empty())
(and similar) so that we reduce release complexity further.

https://codereview.chromium.org/2186893002/diff/1/extensions/common/features/...
File extensions/common/features/simple_feature.h (right):

https://codereview.chromium.org/2186893002/diff/1/extensions/common/features/...
extensions/common/features/simple_feature.h:109: // when we pass by value?
Perhaps codegen is still worse.
Passing by rvalue reference reduced binary size and had the intentional side
effect of discouraging use of these functions when it would cause a copy.

[dcheng, 2016-07-27 16:07:37]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[dcheng, 2016-07-27 16:07:48]

https://codereview.chromium.org/2186893002/diff/1/extensions/common/features/...
File extensions/common/features/simple_feature.cc (right):

https://codereview.chromium.org/2186893002/diff/1/extensions/common/features/...
extensions/common/features/simple_feature.cc:672: blacklist_.clear();
On 2016/07/27 15:36:49, Devlin wrote:
> These should only be called at feature construction, so all these parameters
> *should* be empty.  I'd prefer to replace this with a
DCHECK(blacklist_.empty())
> (and similar) so that we reduce release complexity further.

The size impact of this is pretty negligible, maybe a few bytes. Changing the
behavior of just these setters also makes the different setters behave
inconsistently. I'd rather not do that (and it's also somewhat surprising). In
any case, it seems out of scope of this change: I'm trying to avoid any behavior
changes in this CL.

As for efficiency, I just changed this to use .assign(). Strictly speaking, it's
slightly less efficient than the original version: if we had an
initializer_list<std::string> here, the constructor can be slightly more
efficient by reserving all the space upfront and then constructing the items
into place. I'm not sure it will make a difference in practice, given the
typical sizes of these vectors though.

[commit-bot: I haz the power, 2016-07-27 16:07:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Devlin, 2016-07-27 16:43:59]

lgtm.  From relatively unscientific local testing, this seems about as efficient
as it was before.

Thanks!

https://codereview.chromium.org/2186893002/diff/1/extensions/common/features/...
File extensions/common/features/simple_feature.h (right):

https://codereview.chromium.org/2186893002/diff/1/extensions/common/features/...
extensions/common/features/simple_feature.h:114: void
set_command_line_switch(std::string&& command_line_switch);
On 2016/07/27 09:48:50, dcheng wrote:
> This is the last thing that passes by rvalue: what do you think of just making
> this consistent with set_name and passing by StringPiece?

Sure.  Any reason not to?

https://codereview.chromium.org/2186893002/diff/20001/extensions/common/featu...
File extensions/common/features/simple_feature.h (right):

https://codereview.chromium.org/2186893002/diff/20001/extensions/common/featu...
extensions/common/features/simple_feature.h:108: // TODO(dcheng): Not sure why
this needs a style exception, we can still move
No longer does, right?  Unless initializer_lists are a style exception?

[Devlin, 2016-07-27 16:46:23]

https://codereview.chromium.org/2186893002/diff/20001/extensions/common/featu...
File extensions/common/features/base_feature_provider.h (right):

https://codereview.chromium.org/2186893002/diff/20001/extensions/common/featu...
extensions/common/features/base_feature_provider.h:42: void
AddFeature(base::StringPiece name, std::unique_ptr<Feature> feature);
For my own edification, what's the motivation behind this one?

[dcheng, 2016-07-27 17:02:36]

PTAL

https://codereview.chromium.org/2186893002/diff/1/extensions/common/features/...
File extensions/common/features/simple_feature.h (right):

https://codereview.chromium.org/2186893002/diff/1/extensions/common/features/...
extensions/common/features/simple_feature.h:114: void
set_command_line_switch(std::string&& command_line_switch);
On 2016/07/27 16:43:59, Devlin wrote:
> On 2016/07/27 09:48:50, dcheng wrote:
> > This is the last thing that passes by rvalue: what do you think of just
making
> > this consistent with set_name and passing by StringPiece?
> 
> Sure.  Any reason not to?

Done.

https://codereview.chromium.org/2186893002/diff/20001/extensions/common/featu...
File extensions/common/features/base_feature_provider.h (right):

https://codereview.chromium.org/2186893002/diff/20001/extensions/common/featu...
extensions/common/features/base_feature_provider.h:42: void
AddFeature(base::StringPiece name, std::unique_ptr<Feature> feature);
On 2016/07/27 16:46:23, Devlin wrote:
> For my own edification, what's the motivation behind this one?

1. map::operator[] generates a bunch of inline code
2. the string literal to std::string conversion also generates a bunch of inline
code

Previously, we paid the cost of this every time we called feature_["literal"] =
std::move(feature);

Now, we only pay the cost once (in the definition of AddFeature). We still have
to pay a few instructions to set up this actual function call, but it still
works out to a pretty significant savings.

https://codereview.chromium.org/2186893002/diff/20001/extensions/common/featu...
File extensions/common/features/simple_feature.h (right):

https://codereview.chromium.org/2186893002/diff/20001/extensions/common/featu...
extensions/common/features/simple_feature.h:108: // TODO(dcheng): Not sure why
this needs a style exception, we can still move
On 2016/07/27 16:43:59, Devlin wrote:
> No longer does, right?  Unless initializer_lists are a style exception?

I've updated this comment.

[Devlin, 2016-07-27 17:04:59]

s lgtm.

https://codereview.chromium.org/2186893002/diff/20001/extensions/common/featu...
File extensions/common/features/base_feature_provider.h (right):

https://codereview.chromium.org/2186893002/diff/20001/extensions/common/featu...
extensions/common/features/base_feature_provider.h:42: void
AddFeature(base::StringPiece name, std::unique_ptr<Feature> feature);
On 2016/07/27 17:02:36, dcheng wrote:
> On 2016/07/27 16:46:23, Devlin wrote:
> > For my own edification, what's the motivation behind this one?
> 
> 1. map::operator[] generates a bunch of inline code
> 2. the string literal to std::string conversion also generates a bunch of
inline
> code
> 
> Previously, we paid the cost of this every time we called feature_["literal"]
=
> std::move(feature);
> 
> Now, we only pay the cost once (in the definition of AddFeature). We still
have
> to pay a few instructions to set up this actual function call, but it still
> works out to a pretty significant savings.

Ah, makes sense.  Thanks!

[dcheng, 2016-07-28 06:28:14]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2016-07-28 06:28:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-28 07:04:19]

Description was changed from

==========
Reduce size of generated extension FeatureProviders.

The generated feature provider code was generating a lot of std::string
and std::vector objects from string literals and initializer lists.
Unfortunately, this results in inlining a lot of STL code: on an
official Linux build, extensions::APIFeatureProvider::APIFeatureProvider
was over 200KB of code. After this change, it's still larger than
expected, at just under 67KB, but it's much better than before. In all,
this saves about 275KB of binary size in an official Linux GN build.

BUG=631915
==========

to

==========
Reduce size of generated extension FeatureProviders.

The generated feature provider code was generating a lot of std::string
and std::vector objects from string literals and initializer lists.
Unfortunately, this results in inlining a lot of STL code: on an
official Linux build, extensions::APIFeatureProvider::APIFeatureProvider
was over 200KB of code. After this change, it's still larger than
expected, at just under 67KB, but it's much better than before. In all,
this saves about 275KB of binary size in an official Linux GN build.

BUG=631915
==========

[commit-bot: I haz the power, 2016-07-28 07:04:19]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-28 07:07:33]

Description was changed from

==========
Reduce size of generated extension FeatureProviders.

The generated feature provider code was generating a lot of std::string
and std::vector objects from string literals and initializer lists.
Unfortunately, this results in inlining a lot of STL code: on an
official Linux build, extensions::APIFeatureProvider::APIFeatureProvider
was over 200KB of code. After this change, it's still larger than
expected, at just under 67KB, but it's much better than before. In all,
this saves about 275KB of binary size in an official Linux GN build.

BUG=631915
==========

to

==========
Reduce size of generated extension FeatureProviders.

The generated feature provider code was generating a lot of std::string
and std::vector objects from string literals and initializer lists.
Unfortunately, this results in inlining a lot of STL code: on an
official Linux build, extensions::APIFeatureProvider::APIFeatureProvider
was over 200KB of code. After this change, it's still larger than
expected, at just under 67KB, but it's much better than before. In all,
this saves about 275KB of binary size in an official Linux GN build.

BUG=631915

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

[commit-bot: I haz the power, 2016-07-28 07:07:34]

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