[subresource_filter] Index activation rules separately.

[subresource_filter] Index activation rules separately.

This CL adds a separate index for activation rules. This drastically increases
speed of IndexedRuleset::ShouldDisableFilteringForDocument, because currently
there are only about 20 rules with activation options, as opposed to tens of
thousands URL rules total in the ruleset. Ultimately, this change makes
computing activation states for subdocuments very fast.

Memory footprint added by the new index is less than a kilobyte.

BUG=708458
Review-Url: https://codereview.chromium.org/2797133006
Cr-Commit-Position: refs/heads/master@{#462825}
Committed: https://chromium.googlesource.com/chromium/src/+/986188a907c91d38938e24604f7b4397fdaaff75

[pkalinnikov, 2017-04-06 11:49:04]

Description was changed from

==========
[subresource_filter] Index activation rules separately.

BUG=708458
==========

to

==========
[subresource_filter] Index activation rules separately.

This CL adds a separate index for activation rules. This drastically increases
speed of IndexedRuleset::ShouldDisableFilteringForDocument, because currently
there are only about 20 rules with activation options, as opposed to tens of
thousands URL rules total in the ruleset. Ultimately, this change makes
computing activation states for subdocuments very fast.

Memory footprint added by the new index is less than a kilobyte.

BUG=708458
==========

[pkalinnikov, 2017-04-06 11:49:59]

pkalinnikov@chromium.org changed reviewers:
+ csharrison@chromium.org, engedy@chromium.org

[pkalinnikov, 2017-04-06 11:49:59]

PTAL.

[pkalinnikov, 2017-04-06 11:50:54]

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

[commit-bot: I haz the power, 2017-04-06 11:51:07]

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, 2017-04-06 12:12:45]

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

[commit-bot: I haz the power, 2017-04-06 12:12:46]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[pkalinnikov, 2017-04-06 12:16:50]

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

[commit-bot: I haz the power, 2017-04-06 12:17:09]

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

[pkalinnikov, 2017-04-06 12:58:53]

Note: "indexed_ruleset_unittest.cc" already covers this change in the
OneRuleWithActivationTypes test.

[commit-bot: I haz the power, 2017-04-06 13:03:24]

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

[commit-bot: I haz the power, 2017-04-06 13:03:25]

Dry run: This issue passed the CQ dry run.

[engedy, 2017-04-06 14:21:30]

Looks great, LGTM % minor comments.

I find it amazing that a conceptually simple -- but in fact quite fundamental --
change like this translates to such a simple and painless change in the code.
Testament to great code health!

https://codereview.chromium.org/2797133006/diff/40001/components/subresource_...
File components/subresource_filter/core/common/indexed_ruleset.cc (right):

https://codereview.chromium.org/2797133006/diff/40001/components/subresource_...
components/subresource_filter/core/common/indexed_ruleset.cc:221: auto
add_rule_to_index = [&rule, rule_offset](MutableUrlPatternIndex* index) {
nit: Have you considered sticking to the old approach and not using a lambda
here?

https://codereview.chromium.org/2797133006/diff/40001/components/subresource_...
File components/subresource_filter/core/common/indexed_ruleset_unittest.cc
(right):

https://codereview.chromium.org/2797133006/diff/40001/components/subresource_...
components/subresource_filter/core/common/indexed_ruleset_unittest.cc:676:
TEST_F(SubresourceFilterIndexedRulesetTest, BlacklistAndActivationType) {
Do we have a test for a rule with non-empty element_types and activation_types
at the same time? Essentially a version of this with only one rule. If not, we
should add one.

[pkalinnikov, 2017-04-07 09:06:48]

Thanks! Balazs, one question for you.

https://codereview.chromium.org/2797133006/diff/40001/components/subresource_...
File components/subresource_filter/core/common/indexed_ruleset.cc (right):

https://codereview.chromium.org/2797133006/diff/40001/components/subresource_...
components/subresource_filter/core/common/indexed_ruleset.cc:221: auto
add_rule_to_index = [&rule, rule_offset](MutableUrlPatternIndex* index) {
On 2017/04/06 14:21:30, engedy wrote:
> nit: Have you considered sticking to the old approach and not using a lambda
> here?

A whitelist rule can be added to |whitelist_| and |activation_| at the same
time. So, with the old approach I would need an array of 2 pointers to
MutableUrlPatternIndex, which is quite ugly.

The alternative is to define a static version of this lambda either on the class
level (and benefit from using all the typedefs in its signature), or on the
level of this file (but the signature would become long because of flatbuffers'
types, plus the MutableUrlPatternIndex is a private type).

WDYT?

https://codereview.chromium.org/2797133006/diff/40001/components/subresource_...
File components/subresource_filter/core/common/indexed_ruleset_unittest.cc
(right):

https://codereview.chromium.org/2797133006/diff/40001/components/subresource_...
components/subresource_filter/core/common/indexed_ruleset_unittest.cc:676:
TEST_F(SubresourceFilterIndexedRulesetTest, BlacklistAndActivationType) {
On 2017/04/06 14:21:30, engedy wrote:
> Do we have a test for a rule with non-empty element_types and activation_types
> at the same time? Essentially a version of this with only one rule. If not, we
> should add one.

Done.

[pkalinnikov, 2017-04-07 09:06:56]

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

[commit-bot: I haz the power, 2017-04-07 09:07:18]

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

[engedy, 2017-04-07 09:21:43]

https://codereview.chromium.org/2797133006/diff/40001/components/subresource_...
File components/subresource_filter/core/common/indexed_ruleset.cc (right):

https://codereview.chromium.org/2797133006/diff/40001/components/subresource_...
components/subresource_filter/core/common/indexed_ruleset.cc:221: auto
add_rule_to_index = [&rule, rule_offset](MutableUrlPatternIndex* index) {
On 2017/04/07 09:06:48, pkalinnikov wrote:
> On 2017/04/06 14:21:30, engedy wrote:
> > nit: Have you considered sticking to the old approach and not using a lambda
> > here?
> 
> A whitelist rule can be added to |whitelist_| and |activation_| at the same
> time. So, with the old approach I would need an array of 2 pointers to
> MutableUrlPatternIndex, which is quite ugly.
> 
> The alternative is to define a static version of this lambda either on the
class
> level (and benefit from using all the typedefs in its signature), or on the
> level of this file (but the signature would become long because of
flatbuffers'
> types, plus the MutableUrlPatternIndex is a private type).
> 
> WDYT?

Oh yes, I see that now. Current solution looks good to me.

[commit-bot: I haz the power, 2017-04-07 09:59:57]

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

[commit-bot: I haz the power, 2017-04-07 09:59:58]

Dry run: This issue passed the CQ dry run.

[pkalinnikov, 2017-04-07 10:00:23]

The CQ bit was checked by pkalinnikov@chromium.org

[pkalinnikov, 2017-04-07 10:00:23]

The patchset sent to the CQ was uploaded after l-g-t-m from engedy@chromium.org
Link to the patchset: https://codereview.chromium.org/2797133006/#ps60001
(title: "Add unittest.")

[commit-bot: I haz the power, 2017-04-07 10:00:38]

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, 2017-04-07 10:04:41]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491559223352020,
"parent_rev": "b7a72005ab58990243283267c49c423ac6e8d3ff", "commit_rev":
"986188a907c91d38938e24604f7b4397fdaaff75"}

[commit-bot: I haz the power, 2017-04-07 10:05:29]

Description was changed from

==========
[subresource_filter] Index activation rules separately.

This CL adds a separate index for activation rules. This drastically increases
speed of IndexedRuleset::ShouldDisableFilteringForDocument, because currently
there are only about 20 rules with activation options, as opposed to tens of
thousands URL rules total in the ruleset. Ultimately, this change makes
computing activation states for subdocuments very fast.

Memory footprint added by the new index is less than a kilobyte.

BUG=708458
==========

to

==========
[subresource_filter] Index activation rules separately.

This CL adds a separate index for activation rules. This drastically increases
speed of IndexedRuleset::ShouldDisableFilteringForDocument, because currently
there are only about 20 rules with activation options, as opposed to tens of
thousands URL rules total in the ruleset. Ultimately, this change makes
computing activation states for subdocuments very fast.

Memory footprint added by the new index is less than a kilobyte.

BUG=708458

Review-Url: https://codereview.chromium.org/2797133006
Cr-Commit-Position: refs/heads/master@{#462825}
Committed:
https://chromium.googlesource.com/chromium/src/+/986188a907c91d38938e24604f7b...
==========

[commit-bot: I haz the power, 2017-04-07 10:05:30]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/986188a907c91d38938e24604f7b...

[Charlie Harrison, 2017-04-07 12:09:57]

lgtm/2