Introduce performance measurement varitation parameter for SubresourceFilter.

Introduce performance measurement varitation parameter for SubresourceFilter.

The parameter defines a fraction of page loads that should have extended
performance measurements enabled. ContentSubresourceFilterDriverFactory uses it
for each main-frame navigation as a probability for enabling
|measure_performance|, which then gets distributed to all
DocumentSubresourceFilter's corresponding to (sub-)frames of the page.

If the variation parameter is not present or malformed, the extended performance
measurement is switched off.

BUG=672519
Committed: https://crrev.com/3b758a270a56b34f25f73bee8130a5f6ec816d6e
Cr-Commit-Position: refs/heads/master@{#438543}

[pkalinnikov, 2016-12-12 14:40:51]

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

[pkalinnikov, 2016-12-12 14:40:52]

Balazs, PTAL.

Questions:
1. Are we fine with random sampling, or a more predictable scheme (e.g., take
every Nth or N out of M) would be preferable?
2. Note that the default sampling rate is 1, to be compliant with the existing
behavior and tests. Do you think it would be better to make it 0 instead?
3. I have doubts on using double. Maybe it would be better to have smth like
"PerformanceMeasurementsPer1000Loads" as an integer. What do you think?
4. I see lots of call sites for ScopedSubresourceFilterFeatureToggle. I am not
sure if I should modify them, add more tests by the side, or leave them as is.

[engedy, 2016-12-12 15:31:23]

> 1. Are we fine with random sampling, or a more predictable scheme (e.g., take
> every Nth or N out of M) would be preferable?

For every N out of M, we would need to maintain state across tabs and across
start-up/shut-down cycles to avoid bias (especially Android where the browser
might be killed very often). Random will get us unbiased results (admittedly at
the expense of slightly higher variation), so I'd go with that. 

> 2. Note that the default sampling rate is 1, to be compliant with the existing
> behavior and tests. Do you think it would be better to make it 0 instead?

Yeah, let's err on the side of being conservative.

> 3. I have doubts on using double. Maybe it would be better to have smth like
> "PerformanceMeasurementsPer1000Loads" as an integer. What do you think?

With the current implementation of GetRand(), we can get exactly 0% and 100%,
which is the important part. Also it's the natural way to define the rate and
does not require additional explanation. Do you foresee other complications?

> 4. I see lots of call sites for ScopedSubresourceFilterFeatureToggle. I am not
> sure if I should modify them, add more tests by the side, or leave them as is.

The current test for the toggle itself looks good. In addition, let's add two
test methods in SubresourceFilterDriverFactory unittests that verify that
measure_performance is true,  and false when the rate is 1, and 0, respectively.

[engedy, 2016-12-12 15:32:36]

LGTM % comments and adding some more information to the CL desciption.

https://codereview.chromium.org/2569693002/diff/20001/components/subresource_...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
(right):

https://codereview.chromium.org/2569693002/diff/20001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:168:
activation_state_ = ActivationState::DISABLED;
I would reset |measure_performance_| here as well for clarity, even though its
value is never used if |activation_state_| is DISABLED.

https://codereview.chromium.org/2569693002/diff/20001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:218:
activation_state_ = ActivationState::DISABLED;
Same here, reset |measure_performance_|.

https://codereview.chromium.org/2569693002/diff/20001/components/subresource_...
File components/subresource_filter/core/browser/subresource_filter_features.cc
(right):

https://codereview.chromium.org/2569693002/diff/20001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features.cc:81: :
1;
Let's return 0 by default (but continue to allow == 1 as well).

https://codereview.chromium.org/2569693002/diff/20001/components/subresource_...
File components/subresource_filter/core/browser/subresource_filter_features.h
(right):

https://codereview.chromium.org/2569693002/diff/20001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features.h:33:
extern const char kPerfMeasurementRateParameterName[];
nit: Abbreviations are discouraged as per the style guide unless they are very
common. Please s/Perf/Performance/g, it's only a few more characters.

https://codereview.chromium.org/2569693002/diff/20001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features.h:51: //
Returns current performance measurement rate for the SubresourceFilter
Slightly more succinct phrasing suggestion:

Returns a number in the range [0, 1], indicating the fraction of page loads that
should have extended performance measurements enabled. The rate will be 0 unless
a greater frequency is specified by variation parameters.

https://codereview.chromium.org/2569693002/diff/20001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features.h:55:
double GetCurrentPerfMeasurementRate();
Drop 'Current' for simplicity.

[pkalinnikov, 2016-12-12 18:47:29]

PTAL again.

https://codereview.chromium.org/2569693002/diff/20001/components/subresource_...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
(right):

https://codereview.chromium.org/2569693002/diff/20001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:168:
activation_state_ = ActivationState::DISABLED;
On 2016/12/12 15:32:36, engedy wrote:
> I would reset |measure_performance_| here as well for clarity, even though its
> value is never used if |activation_state_| is DISABLED.

Done.

https://codereview.chromium.org/2569693002/diff/20001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:218:
activation_state_ = ActivationState::DISABLED;
On 2016/12/12 15:32:36, engedy wrote:
> Same here, reset |measure_performance_|.

Done.

https://codereview.chromium.org/2569693002/diff/20001/components/subresource_...
File components/subresource_filter/core/browser/subresource_filter_features.cc
(right):

https://codereview.chromium.org/2569693002/diff/20001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features.cc:81: :
1;
On 2016/12/12 15:32:36, engedy wrote:
> Let's return 0 by default (but continue to allow == 1 as well).

Done.

https://codereview.chromium.org/2569693002/diff/20001/components/subresource_...
File components/subresource_filter/core/browser/subresource_filter_features.h
(right):

https://codereview.chromium.org/2569693002/diff/20001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features.h:33:
extern const char kPerfMeasurementRateParameterName[];
On 2016/12/12 15:32:36, engedy wrote:
> nit: Abbreviations are discouraged as per the style guide unless they are very
> common. Please s/Perf/Performance/g, it's only a few more characters.

Done.

https://codereview.chromium.org/2569693002/diff/20001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features.h:51: //
Returns current performance measurement rate for the SubresourceFilter
On 2016/12/12 15:32:36, engedy wrote:
> Slightly more succinct phrasing suggestion:
> 
> Returns a number in the range [0, 1], indicating the fraction of page loads
that
> should have extended performance measurements enabled. The rate will be 0
unless
> a greater frequency is specified by variation parameters.

Done.

https://codereview.chromium.org/2569693002/diff/20001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features.h:55:
double GetCurrentPerfMeasurementRate();
On 2016/12/12 15:32:36, engedy wrote:
> Drop 'Current' for simplicity.

Done.

[pkalinnikov, 2016-12-12 18:48:25]

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

[commit-bot: I haz the power, 2016-12-12 18:49:09]

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

[pkalinnikov, 2016-12-13 15:53:55]

Description was changed from

==========
Introduce performance measurement varitation parameter for SubresourceFilter.

BUG=672519
==========

to

==========
Introduce performance measurement varitation parameter for SubresourceFilter.

The parameter defines a fraction of page loads that should have extended
performance measurements enabled. ContentSubresourceFilterDriverFactory uses it
for each main-frame navigation as a probability for enabling
|measure_performance|, which is then gets distributed to all
DocumentSubresourceFilter's corresponding to (sub-)frames of the page.

BUG=672519
==========

[pkalinnikov, 2016-12-13 15:54:08]

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

[commit-bot: I haz the power, 2016-12-13 15:54:42]

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

[pkalinnikov, 2016-12-13 15:56:03]

Description was changed from

==========
Introduce performance measurement varitation parameter for SubresourceFilter.

The parameter defines a fraction of page loads that should have extended
performance measurements enabled. ContentSubresourceFilterDriverFactory uses it
for each main-frame navigation as a probability for enabling
|measure_performance|, which is then gets distributed to all
DocumentSubresourceFilter's corresponding to (sub-)frames of the page.

BUG=672519
==========

to

==========
Introduce performance measurement varitation parameter for SubresourceFilter.

The parameter defines a fraction of page loads that should have extended
performance measurements enabled. ContentSubresourceFilterDriverFactory uses it
for each main-frame navigation as a probability for enabling
|measure_performance|, which then gets distributed to all
DocumentSubresourceFilter's corresponding to (sub-)frames of the page.

If the variation parameter is not present or malformed, the extended performance
measurement is switched off.

BUG=672519
==========

[commit-bot: I haz the power, 2016-12-13 16:55:48]

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

[commit-bot: I haz the power, 2016-12-13 16:55:49]

Dry run: This issue passed the CQ dry run.

[engedy, 2016-12-14 17:39:39]

On 2016/12/12 18:47:29, pkalinnikov wrote:
> PTAL again.
> 
>
https://codereview.chromium.org/2569693002/diff/20001/components/subresource_...
> File
>
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
> (right):
> 
>
https://codereview.chromium.org/2569693002/diff/20001/components/subresource_...
>
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:168:
> activation_state_ = ActivationState::DISABLED;
> On 2016/12/12 15:32:36, engedy wrote:
> > I would reset |measure_performance_| here as well for clarity, even though
its
> > value is never used if |activation_state_| is DISABLED.
> 
> Done.
> 
>
https://codereview.chromium.org/2569693002/diff/20001/components/subresource_...
>
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:218:
> activation_state_ = ActivationState::DISABLED;
> On 2016/12/12 15:32:36, engedy wrote:
> > Same here, reset |measure_performance_|.
> 
> Done.
> 
>
https://codereview.chromium.org/2569693002/diff/20001/components/subresource_...
> File components/subresource_filter/core/browser/subresource_filter_features.cc
> (right):
> 
>
https://codereview.chromium.org/2569693002/diff/20001/components/subresource_...
> components/subresource_filter/core/browser/subresource_filter_features.cc:81:
:
> 1;
> On 2016/12/12 15:32:36, engedy wrote:
> > Let's return 0 by default (but continue to allow == 1 as well).
> 
> Done.
> 
>
https://codereview.chromium.org/2569693002/diff/20001/components/subresource_...
> File components/subresource_filter/core/browser/subresource_filter_features.h
> (right):
> 
>
https://codereview.chromium.org/2569693002/diff/20001/components/subresource_...
> components/subresource_filter/core/browser/subresource_filter_features.h:33:
> extern const char kPerfMeasurementRateParameterName[];
> On 2016/12/12 15:32:36, engedy wrote:
> > nit: Abbreviations are discouraged as per the style guide unless they are
very
> > common. Please s/Perf/Performance/g, it's only a few more characters.
> 
> Done.
> 
>
https://codereview.chromium.org/2569693002/diff/20001/components/subresource_...
> components/subresource_filter/core/browser/subresource_filter_features.h:51:
//
> Returns current performance measurement rate for the SubresourceFilter
> On 2016/12/12 15:32:36, engedy wrote:
> > Slightly more succinct phrasing suggestion:
> > 
> > Returns a number in the range [0, 1], indicating the fraction of page loads
> that
> > should have extended performance measurements enabled. The rate will be 0
> unless
> > a greater frequency is specified by variation parameters.
> 
> Done.
> 
>
https://codereview.chromium.org/2569693002/diff/20001/components/subresource_...
> components/subresource_filter/core/browser/subresource_filter_features.h:55:
> double GetCurrentPerfMeasurementRate();
> On 2016/12/12 15:32:36, engedy wrote:
> > Drop 'Current' for simplicity.
> 
> Done.

[engedy, 2016-12-14 17:39:58]

LGTM, thanks!

[pkalinnikov, 2016-12-14 17:41:26]

The CQ bit was checked by pkalinnikov@chromium.org

[commit-bot: I haz the power, 2016-12-14 17:41:52]

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-12-14 18:35:44]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481737286053200,
"parent_rev": "b6875d9c3c3c7c5465719e27df4f81f12280cbd5", "commit_rev":
"c1f5357fa6e3e6324f1c680ad9019fe3454e9b39"}

[commit-bot: I haz the power, 2016-12-14 18:36:12]

Description was changed from

==========
Introduce performance measurement varitation parameter for SubresourceFilter.

The parameter defines a fraction of page loads that should have extended
performance measurements enabled. ContentSubresourceFilterDriverFactory uses it
for each main-frame navigation as a probability for enabling
|measure_performance|, which then gets distributed to all
DocumentSubresourceFilter's corresponding to (sub-)frames of the page.

If the variation parameter is not present or malformed, the extended performance
measurement is switched off.

BUG=672519
==========

to

==========
Introduce performance measurement varitation parameter for SubresourceFilter.

The parameter defines a fraction of page loads that should have extended
performance measurements enabled. ContentSubresourceFilterDriverFactory uses it
for each main-frame navigation as a probability for enabling
|measure_performance|, which then gets distributed to all
DocumentSubresourceFilter's corresponding to (sub-)frames of the page.

If the variation parameter is not present or malformed, the extended performance
measurement is switched off.

BUG=672519

Review-Url: https://codereview.chromium.org/2569693002
==========

[commit-bot: I haz the power, 2016-12-14 18:36:13]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-14 18:37:42]

Description was changed from

==========
Introduce performance measurement varitation parameter for SubresourceFilter.

The parameter defines a fraction of page loads that should have extended
performance measurements enabled. ContentSubresourceFilterDriverFactory uses it
for each main-frame navigation as a probability for enabling
|measure_performance|, which then gets distributed to all
DocumentSubresourceFilter's corresponding to (sub-)frames of the page.

If the variation parameter is not present or malformed, the extended performance
measurement is switched off.

BUG=672519

Review-Url: https://codereview.chromium.org/2569693002
==========

to

==========
Introduce performance measurement varitation parameter for SubresourceFilter.

The parameter defines a fraction of page loads that should have extended
performance measurements enabled. ContentSubresourceFilterDriverFactory uses it
for each main-frame navigation as a probability for enabling
|measure_performance|, which then gets distributed to all
DocumentSubresourceFilter's corresponding to (sub-)frames of the page.

If the variation parameter is not present or malformed, the extended performance
measurement is switched off.

BUG=672519

Committed: https://crrev.com/3b758a270a56b34f25f73bee8130a5f6ec816d6e
Cr-Commit-Position: refs/heads/master@{#438543}
==========

[commit-bot: I haz the power, 2016-12-14 18:37:43]

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