Further reduction of feature_compiler.py generated code

Further reduction of feature_compiler.py generated code

Use raw pointers instead of unique pointers to reduce size. See linked
bug for size details.

BUG=631416
Committed: https://crrev.com/2026cea7fa1b2146d7113dc31f2a2969d9ec58e3
Cr-Commit-Position: refs/heads/master@{#408788}

[scottmg, 2016-07-29 18:28:22]

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

[scottmg, 2016-07-29 18:28:40]

Description was changed from

==========
Further reduction of feature_compiler.py generated code

BUG=631416
==========

to

==========
Further reduction of feature_compiler.py generated code

Use raw pointers instead of unique pointers to reduce size.

BUG=631416
==========

[scottmg, 2016-07-29 18:28:55]

Description was changed from

==========
Further reduction of feature_compiler.py generated code

Use raw pointers instead of unique pointers to reduce size.

BUG=631416
==========

to

==========
Further reduction of feature_compiler.py generated code

Use raw pointers instead of unique pointers to reduce size. See linked bug for
size details.

BUG=631416
==========

[scottmg, 2016-07-29 18:29:30]

Description was changed from

==========
Further reduction of feature_compiler.py generated code

Use raw pointers instead of unique pointers to reduce size. See linked bug for
size details.

BUG=631416
==========

to

==========
Further reduction of feature_compiler.py generated code

Use raw pointers instead of unique pointers to reduce size. See linked
bug for size details.

BUG=631416
==========

[scottmg, 2016-07-29 18:30:28]

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

[commit-bot: I haz the power, 2016-07-29 18:31:06]

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-29 18:41:01]

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

[commit-bot: I haz the power, 2016-07-29 18:41:02]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Devlin, 2016-07-29 18:50:15]

https://codereview.chromium.org/2193693003/diff/20001/extensions/common/featu...
File extensions/common/features/complex_feature.cc (right):

https://codereview.chromium.org/2193693003/diff/20001/extensions/common/featu...
extensions/common/features/complex_feature.cc:33: STLDeleteElements(&features_);
This makes me a bit sad.

Since we control the construction of the FeatureList and can avoid the
wrapping/unwrapping of all the unique_ptrs a bit more, is using raw ptrs still a
huge win over unique_ptrs here?

(I also notice that FeatureProvider's map still uses unique_ptrs - I'd think the
two containers would be similar.  So, conversely, if this *is* a big win, should
we do the same to the map?)

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

https://codereview.chromium.org/2193693003/diff/20001/extensions/common/featu...
extensions/common/features/complex_feature.h:28: explicit
ComplexFeature(FeatureList* features);
I think there are a few testing call sites that still need to be updated here.

[scottmg, 2016-07-29 20:05:04]

https://codereview.chromium.org/2193693003/diff/20001/extensions/common/featu...
File extensions/common/features/complex_feature.cc (right):

https://codereview.chromium.org/2193693003/diff/20001/extensions/common/featu...
extensions/common/features/complex_feature.cc:33: STLDeleteElements(&features_);
On 2016/07/29 18:50:14, Devlin wrote:
> This makes me a bit sad.
> 
> Since we control the construction of the FeatureList and can avoid the
> wrapping/unwrapping of all the unique_ptrs a bit more, is using raw ptrs still
a
> huge win over unique_ptrs here?
> 
> (I also notice that FeatureProvider's map still uses unique_ptrs - I'd think
the
> two containers would be similar.  So, conversely, if this *is* a big win,
should
> we do the same to the map?)

Is something like this what you had in mind?

I'm not sure what you mean with FeatureProvider. The reason FeatureList was
changed was to avoid all the unique_ptr wrap/unwrap in the generated code. In
the actual implementation of ComplexFeature it's irrelevant.

[Devlin, 2016-07-29 20:13:49]

This looks okay, so if these other suggestions don't pan out, this is probably
fine.

https://codereview.chromium.org/2193693003/diff/40001/extensions/common/featu...
File extensions/common/features/base_feature_provider.cc (right):

https://codereview.chromium.org/2193693003/diff/40001/extensions/common/featu...
extensions/common/features/base_feature_provider.cc:70:
features_[name.as_string()] = std::unique_ptr<Feature>(feature);
For the record, this is what I was talking about with FeatureProvider - we still
have features_ has an STL container of unique_ptrs, but just pass it in by raw
ptr and then add it to the container as a unique_ptr.  With the complex feature,
we already have the container, so maybe we could avoid the wrap/unwrap just
through the generated code.  See other comment.

https://codereview.chromium.org/2193693003/diff/40001/extensions/common/featu...
File extensions/common/features/complex_feature.cc (right):

https://codereview.chromium.org/2193693003/diff/40001/extensions/common/featu...
extensions/common/features/complex_feature.cc:12:
features_.push_back(std::unique_ptr<Feature>(f));
What if we did something in the generated code like:
std::vector<std::unique_ptr<Feature>> features;
{
  SimpleFeature* feature = new SimpleFeature();
  ...
  features.push_back(std::unique_ptr<Feature>(f));
}
new ComplexFeature(std::move(features));

(or ComplexFeature could take a std::vector<unique_ptr<Feature>>* and we could
call swap, or some other variation.)  That should avoid the wrap/unwrap, because
we're only wrapping them when they are placed in the vector, right?

Overall this is probably okay, but it seems intuitively like it would be faster
if we didn't have to construct two vectors.

[scottmg, 2016-07-29 20:24:53]

https://codereview.chromium.org/2193693003/diff/40001/extensions/common/featu...
File extensions/common/features/complex_feature.cc (right):

https://codereview.chromium.org/2193693003/diff/40001/extensions/common/featu...
extensions/common/features/complex_feature.cc:12:
features_.push_back(std::unique_ptr<Feature>(f));
On 2016/07/29 20:13:49, Devlin wrote:
> What if we did something in the generated code like:
> std::vector<std::unique_ptr<Feature>> features;
> {
>   SimpleFeature* feature = new SimpleFeature();
>   ...
>   features.push_back(std::unique_ptr<Feature>(f));
> }
> new ComplexFeature(std::move(features));
> 
> (or ComplexFeature could take a std::vector<unique_ptr<Feature>>* and we could
> call swap, or some other variation.)  That should avoid the wrap/unwrap,
because
> we're only wrapping them when they are placed in the vector, right?
> 
> Overall this is probably okay, but it seems intuitively like it would be
faster
> if we didn't have to construct two vectors.

Sorry, I'm probably missing something, but the point is to avoid having N calls
to unique_ptr::unique_ptr() in the generated code, rather than only 1 in
ComplexFeature, so I don't see how that would help.

[scottmg, 2016-07-29 20:41:09]

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

[commit-bot: I haz the power, 2016-07-29 20:41:37]

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-29 21:15:02]

lgtm with some completely optional nits.

https://codereview.chromium.org/2193693003/diff/40001/extensions/common/featu...
File extensions/common/features/complex_feature.cc (right):

https://codereview.chromium.org/2193693003/diff/40001/extensions/common/featu...
extensions/common/features/complex_feature.cc:12:
features_.push_back(std::unique_ptr<Feature>(f));
On 2016/07/29 20:24:53, scottmg (OOO until August) wrote:
> On 2016/07/29 20:13:49, Devlin wrote:
> > What if we did something in the generated code like:
> > std::vector<std::unique_ptr<Feature>> features;
> > {
> >   SimpleFeature* feature = new SimpleFeature();
> >   ...
> >   features.push_back(std::unique_ptr<Feature>(f));
> > }
> > new ComplexFeature(std::move(features));
> > 
> > (or ComplexFeature could take a std::vector<unique_ptr<Feature>>* and we
could
> > call swap, or some other variation.)  That should avoid the wrap/unwrap,
> because
> > we're only wrapping them when they are placed in the vector, right?
> > 
> > Overall this is probably okay, but it seems intuitively like it would be
> faster
> > if we didn't have to construct two vectors.
> 
> Sorry, I'm probably missing something, but the point is to avoid having N
calls
> to unique_ptr::unique_ptr() in the generated code, rather than only 1 in
> ComplexFeature, so I don't see how that would help.
> 

Ah, right, sorry.  I misunderstood that the concern was the amount of moving
that had to happen (i.e., create a unique_ptr, then move it into the container
vs just construct it directly in the container) rather than the number of calls
to the ctor at all.

https://codereview.chromium.org/2193693003/diff/60001/extensions/common/featu...
File extensions/common/features/complex_feature.h (right):

https://codereview.chromium.org/2193693003/diff/60001/extensions/common/featu...
extensions/common/features/complex_feature.h:27: explicit ComplexFeature(const
std::vector<Feature*>& features);
nit: I think it's misleading to take ownership of stuff stored in a const&.  I
wonder if this would be cleaner if we took a std::vector<Feature*>* and cleared
it once all features were taken.  WDYT?

https://codereview.chromium.org/2193693003/diff/60001/tools/json_schema_compi...
File tools/json_schema_compiler/feature_compiler.py (right):

https://codereview.chromium.org/2193693003/diff/60001/tools/json_schema_compi...
tools/json_schema_compiler/feature_compiler.py:360: c.Append('%s* feature(new
%s());' % (feature_class, feature_class))
optional nit: Usually we use assignment rather than construction for POD.  I
think this would generate slightly prettier code if it were '%s* feature = new
%s();'.  At the same time, generated code doesn't need to be pretty, so I'm fine
with ignoring this.

[commit-bot: I haz the power, 2016-07-29 21:23:08]

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

[commit-bot: I haz the power, 2016-07-29 21:23:09]

Dry run: This issue passed the CQ dry run.

[scottmg, 2016-07-29 21:28:45]

https://codereview.chromium.org/2193693003/diff/60001/extensions/common/featu...
File extensions/common/features/complex_feature.h (right):

https://codereview.chromium.org/2193693003/diff/60001/extensions/common/featu...
extensions/common/features/complex_feature.h:27: explicit ComplexFeature(const
std::vector<Feature*>& features);
On 2016/07/29 21:15:02, Devlin wrote:
> nit: I think it's misleading to take ownership of stuff stored in a const&.  I
> wonder if this would be cleaner if we took a std::vector<Feature*>* and
cleared
> it once all features were taken.  WDYT?

Shrug, sure. Done.

https://codereview.chromium.org/2193693003/diff/60001/tools/json_schema_compi...
File tools/json_schema_compiler/feature_compiler.py (right):

https://codereview.chromium.org/2193693003/diff/60001/tools/json_schema_compi...
tools/json_schema_compiler/feature_compiler.py:360: c.Append('%s* feature(new
%s());' % (feature_class, feature_class))
On 2016/07/29 21:15:02, Devlin wrote:
> optional nit: Usually we use assignment rather than construction for POD.  I
> think this would generate slightly prettier code if it were '%s* feature = new
> %s();'.  At the same time, generated code doesn't need to be pretty, so I'm
fine
> with ignoring this.

Done.

[scottmg, 2016-07-29 21:29:17]

The CQ bit was checked by scottmg@chromium.org

[scottmg, 2016-07-29 21:29:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2193693003/#ps80001
(title: "nits")

[commit-bot: I haz the power, 2016-07-29 21:29:35]

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-29 22:41:02]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-29 22:43:36]

Description was changed from

==========
Further reduction of feature_compiler.py generated code

Use raw pointers instead of unique pointers to reduce size. See linked
bug for size details.

BUG=631416
==========

to

==========
Further reduction of feature_compiler.py generated code

Use raw pointers instead of unique pointers to reduce size. See linked
bug for size details.

BUG=631416

Committed: https://crrev.com/2026cea7fa1b2146d7113dc31f2a2969d9ec58e3
Cr-Commit-Position: refs/heads/master@{#408788}
==========

[commit-bot: I haz the power, 2016-07-29 22:43:37]

Patchset 5 (id:??) landed as
https://crrev.com/2026cea7fa1b2146d7113dc31f2a2969d9ec58e3
Cr-Commit-Position: refs/heads/master@{#408788}

[tapted, 2016-08-01 06:02:07]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2201653003/ by tapted@chromium.org.

The reason for reverting is: Suspect for widespread Win7 NaCl failures starting
with
https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%281%29/bui...

Wasn't r408782, r408787 or r408781.