Remove EmptyRuleIterators with nullptrs.

Remove EmptyRuleIterators with nullptrs.

Check for nullptrs and avoid adding them to vectors.

Committed: https://crrev.com/9edb4f54d7ac549e7e2a83273a8e75caa223d051
Cr-Commit-Position: refs/heads/master@{#417455}

[Lei Zhang, 2016-09-07 16:43:22]

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

[commit-bot: I haz the power, 2016-09-07 16:43:37]

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-09-07 17:51:48]

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

[commit-bot: I haz the power, 2016-09-07 17:51:52]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2016-09-07 18:29:41]

Description was changed from

==========
Remove EmptyRuleIterators with nullptrs.

Check for nullptrs and avoid adding them to vectors.
==========

to

==========
Remove EmptyRuleIterators with nullptrs.

Check for nullptrs and avoid adding them to vectors.
==========

[Lei Zhang, 2016-09-07 18:29:41]

thestig@chromium.org changed reviewers:
+ bauerb@chromium.org, markusheintz@chromium.org

[Lei Zhang, 2016-09-07 18:37:30]

From running a debug build with some logging and using the browser casually, I
see ContentSettingsStore::GetRuleIterator() getting called numerous times.

Most of the time it:
- constructs an EmptyRuleIterator
- put it in a vector of iterators of size 1
- only to erase it in the ConcatenationIterator ctor

With this CL GetRuleIterator():
- never construct an EmptyRuleIterator
- the vector of iterators stay empty
- ConcatenationIterator's ctor sees an empty vector and carries on

See https://goto.google.com/cwpperf3 for the callgraph.

[Lei Zhang, 2016-09-07 18:49:32]

On 2016/09/07 18:37:30, Lei Zhang wrote:
> See https://goto.google.com/cwpperf3 for the callgraph.

And for those playing at home, the graph looks like:
- GetRuleIterator() as a whole consumes 17.5 Bogocycles:
-- 5.6 Bogocycles in ConcatenationIterator calling vector::erase()
-- 5.3 Bogocycles allocating RuleIterators
-- 3.2 Bogocycles in GetRuleIterator() itself
-- 2.4 Bogocycles doing vector::push_back()
-- 1.0 Bogocycles for everything else

I expect this to shave off ~13 Bogocycles.

[Bernhard Bauer, 2016-09-08 09:30:31]

Neat! LGTM, but could we add a comment to methods that return
unique_ptr<RuleIterator> that nullptr is equivalent to an empty iterator?

https://codereview.chromium.org/2318223002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/content_settings/content_settings_store.cc
(right):

https://codereview.chromium.org/2318223002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/content_settings/content_settings_store.cc:93: new
ConcatenationIterator(std::move(iterators), auto_lock.release()));
You could shortcut here as well and just return nullptr if |iterators| is empty.

https://codereview.chromium.org/2318223002/diff/1/components/content_settings...
File
components/content_settings/core/browser/content_settings_default_provider.cc
(right):

https://codereview.chromium.org/2318223002/diff/1/components/content_settings...
components/content_settings/core/browser/content_settings_default_provider.cc:270:
return std::unique_ptr<RuleIterator>(
Can you use base::WrapUnique()?

[Lei Zhang, 2016-09-08 21:03:15]

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

[commit-bot: I haz the power, 2016-09-08 21:03:44]

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

[Lei Zhang, 2016-09-08 21:04:30]

https://codereview.chromium.org/2318223002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/content_settings/content_settings_store.cc
(right):

https://codereview.chromium.org/2318223002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/content_settings/content_settings_store.cc:93: new
ConcatenationIterator(std::move(iterators), auto_lock.release()));
On 2016/09/08 09:30:31, Bernhard Bauer wrote:
> You could shortcut here as well and just return nullptr if |iterators| is
empty.

Done.

https://codereview.chromium.org/2318223002/diff/1/components/content_settings...
File
components/content_settings/core/browser/content_settings_default_provider.cc
(right):

https://codereview.chromium.org/2318223002/diff/1/components/content_settings...
components/content_settings/core/browser/content_settings_default_provider.cc:270:
return std::unique_ptr<RuleIterator>(
On 2016/09/08 09:30:31, Bernhard Bauer wrote:
> Can you use base::WrapUnique()?

Went straight to MakeUnique.

[commit-bot: I haz the power, 2016-09-08 22:04:45]

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

[commit-bot: I haz the power, 2016-09-08 22:04:46]

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

[Lei Zhang, 2016-09-08 22:53:41]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2016-09-08 22:53:41]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2318223002/#ps60001
(title: "Fix failing unit test")

[commit-bot: I haz the power, 2016-09-08 22:54:18]

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-09-09 00:39:53]

Description was changed from

==========
Remove EmptyRuleIterators with nullptrs.

Check for nullptrs and avoid adding them to vectors.
==========

to

==========
Remove EmptyRuleIterators with nullptrs.

Check for nullptrs and avoid adding them to vectors.
==========

[commit-bot: I haz the power, 2016-09-09 00:39:54]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-09 00:42:06]

Description was changed from

==========
Remove EmptyRuleIterators with nullptrs.

Check for nullptrs and avoid adding them to vectors.
==========

to

==========
Remove EmptyRuleIterators with nullptrs.

Check for nullptrs and avoid adding them to vectors.

Committed: https://crrev.com/9edb4f54d7ac549e7e2a83273a8e75caa223d051
Cr-Commit-Position: refs/heads/master@{#417455}
==========

[commit-bot: I haz the power, 2016-09-09 00:42:09]

Patchset 4 (id:??) landed as
https://crrev.com/9edb4f54d7ac549e7e2a83273a8e75caa223d051
Cr-Commit-Position: refs/heads/master@{#417455}