Add "light speed" experiment code for SafeBrowsing service.

Add "light speed" experiment code for SafeBrowsing service.

This allows turning off safe-browsing experimentally for
a percentage of users on Canary and seeing its impact on
start up performance metrics.

BUG=450037
Committed: https://crrev.com/0cad7260a2e5f8a3a26db8ac57921d06138c277e
Cr-Commit-Position: refs/heads/master@{#316338}

[Alexei Svitkine (slow), 2015-02-12 16:50:57]

asvitkine@chromium.org changed reviewers:
+ gab@chromium.org

[gab, 2015-02-12 16:57:00]

lg, few questions below

https://codereview.chromium.org/924543002/diff/1/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/database_manager.cc (right):

https://codereview.chromium.org/924543002/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/database_manager.cc:655: // Remove when
experimentation is complete. http://crbug.com/416219
Wrong bug #, should be 450037, right?

https://codereview.chromium.org/924543002/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/database_manager.cc:656: if
(!variations::GetVariationParamValue("LightSpeed", "DisableSB").empty())
So the experiment name is "LightSpeed" and it has a "DisableSB" group?

Shouldn't the experiment name be more specific to SB?

[Alexei Svitkine (slow), 2015-02-12 17:26:46]

Patchset #2 (id:20001) has been deleted

[Alexei Svitkine (slow), 2015-02-12 17:27:12]

https://codereview.chromium.org/924543002/diff/1/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/database_manager.cc (right):

https://codereview.chromium.org/924543002/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/database_manager.cc:655: // Remove when
experimentation is complete. http://crbug.com/416219
On 2015/02/12 16:57:00, gab wrote:
> Wrong bug #, should be 450037, right?

Done.

https://codereview.chromium.org/924543002/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/database_manager.cc:656: if
(!variations::GetVariationParamValue("LightSpeed", "DisableSB").empty())
On 2015/02/12 16:57:00, gab wrote:
> So the experiment name is "LightSpeed" and it has a "DisableSB" group?
> 
> Shouldn't the experiment name be more specific to SB?

This is using the parameters support in the variations framework.

The idea is a group can define parameters, so in this case, this is asking
whether the currently active group has a param "DisableSB". go/variations-params
for more details.

I figured we can use a single field trial for all of the "light speed"
experiments we're planning to do, with different params affecting different
settings.

This lets us not step on each others' toes when doing parallel experimentation
and also gives us flexibility of doing e.g. combination of different params.
WDYT?

[gab, 2015-02-12 17:35:42]

https://codereview.chromium.org/924543002/diff/1/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/database_manager.cc (right):

https://codereview.chromium.org/924543002/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/database_manager.cc:656: if
(!variations::GetVariationParamValue("LightSpeed", "DisableSB").empty())
On 2015/02/12 17:27:12, Alexei Svitkine wrote:
> On 2015/02/12 16:57:00, gab wrote:
> > So the experiment name is "LightSpeed" and it has a "DisableSB" group?
> > 
> > Shouldn't the experiment name be more specific to SB?
> 
> This is using the parameters support in the variations framework.
> 
> The idea is a group can define parameters, so in this case, this is asking
> whether the currently active group has a param "DisableSB".
go/variations-params
> for more details.
> 
> I figured we can use a single field trial for all of the "light speed"
> experiments we're planning to do, with different params affecting different
> settings.
> 
> This lets us not step on each others' toes when doing parallel experimentation
> and also gives us flexibility of doing e.g. combination of different params.
> WDYT?

Oh very cool! SGTM

The one thing I'm worried about combining experiments (or otherwise having to
split 25/25/25/25 for two experiments) is that as we dilute the population into
smaller buckets, we'll lose statistical significance (and confidence intervals
may grow too big to prove anything).

Where as I was thinking that if we had two experiments E/F at 50/50, it's okay
for them to overlap as each group will get half of the effect of the other
experiment and it otherwise won't affect the significance of its individual
result (and we could theoretically dremel the combined impacts of {E}, {E,F},
{F}, {} though we wouldn't have those on the dashboards nor be able to easily
create special groups for a higher percentage of the population).

Lastly, do we get confidence intervals with params on the dashboard? (I assume
we do as it's just a group like any other, but just making sure as those will be
key)

[Alexei Svitkine (slow), 2015-02-12 17:47:56]

https://codereview.chromium.org/924543002/diff/1/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/database_manager.cc (right):

https://codereview.chromium.org/924543002/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/database_manager.cc:656: if
(!variations::GetVariationParamValue("LightSpeed", "DisableSB").empty())
On 2015/02/12 17:35:42, gab wrote:
> On 2015/02/12 17:27:12, Alexei Svitkine wrote:
> > On 2015/02/12 16:57:00, gab wrote:
> > > So the experiment name is "LightSpeed" and it has a "DisableSB" group?
> > > 
> > > Shouldn't the experiment name be more specific to SB?
> > 
> > This is using the parameters support in the variations framework.
> > 
> > The idea is a group can define parameters, so in this case, this is asking
> > whether the currently active group has a param "DisableSB".
> go/variations-params
> > for more details.
> > 
> > I figured we can use a single field trial for all of the "light speed"
> > experiments we're planning to do, with different params affecting different
> > settings.
> > 
> > This lets us not step on each others' toes when doing parallel
experimentation
> > and also gives us flexibility of doing e.g. combination of different params.
> > WDYT?
> 
> Oh very cool! SGTM
> 
> The one thing I'm worried about combining experiments (or otherwise having to
> split 25/25/25/25 for two experiments) is that as we dilute the population
into
> smaller buckets, we'll lose statistical significance (and confidence intervals
> may grow too big to prove anything).
> 
> Where as I was thinking that if we had two experiments E/F at 50/50, it's okay
> for them to overlap as each group will get half of the effect of the other
> experiment and it otherwise won't affect the significance of its individual
> result (and we could theoretically dremel the combined impacts of {E}, {E,F},
> {F}, {} though we wouldn't have those on the dashboards nor be able to easily
> create special groups for a higher percentage of the population).
> 
> Lastly, do we get confidence intervals with params on the dashboard? (I assume
> we do as it's just a group like any other, but just making sure as those will
be
> key)

Right, it depends how many we want to run in parallel - indeed if too many, then
we will get smaller user populations. But we can see if that's actually a
problem and can always still do experiments in parallel if needed.

And yes, we get all the same confidence intervals with params on the dashboard -
as they're attached to regular groups.

[gab, 2015-02-12 18:26:58]

lgtm, thanks

https://codereview.chromium.org/924543002/diff/1/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/database_manager.cc (right):

https://codereview.chromium.org/924543002/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/database_manager.cc:656: if
(!variations::GetVariationParamValue("LightSpeed", "DisableSB").empty())
On 2015/02/12 17:47:56, Alexei Svitkine wrote:
> On 2015/02/12 17:35:42, gab wrote:
> > On 2015/02/12 17:27:12, Alexei Svitkine wrote:
> > > On 2015/02/12 16:57:00, gab wrote:
> > > > So the experiment name is "LightSpeed" and it has a "DisableSB" group?
> > > > 
> > > > Shouldn't the experiment name be more specific to SB?
> > > 
> > > This is using the parameters support in the variations framework.
> > > 
> > > The idea is a group can define parameters, so in this case, this is asking
> > > whether the currently active group has a param "DisableSB".
> > go/variations-params
> > > for more details.
> > > 
> > > I figured we can use a single field trial for all of the "light speed"
> > > experiments we're planning to do, with different params affecting
different
> > > settings.
> > > 
> > > This lets us not step on each others' toes when doing parallel
> experimentation
> > > and also gives us flexibility of doing e.g. combination of different
params.
> > > WDYT?
> > 
> > Oh very cool! SGTM
> > 
> > The one thing I'm worried about combining experiments (or otherwise having
to
> > split 25/25/25/25 for two experiments) is that as we dilute the population
> into
> > smaller buckets, we'll lose statistical significance (and confidence
intervals
> > may grow too big to prove anything).
> > 
> > Where as I was thinking that if we had two experiments E/F at 50/50, it's
okay
> > for them to overlap as each group will get half of the effect of the other
> > experiment and it otherwise won't affect the significance of its individual
> > result (and we could theoretically dremel the combined impacts of {E},
{E,F},
> > {F}, {} though we wouldn't have those on the dashboards nor be able to
easily
> > create special groups for a higher percentage of the population).
> > 
> > Lastly, do we get confidence intervals with params on the dashboard? (I
assume
> > we do as it's just a group like any other, but just making sure as those
will
> be
> > key)
> 
> Right, it depends how many we want to run in parallel - indeed if too many,
then
> we will get smaller user populations. But we can see if that's actually a
> problem and can always still do experiments in parallel if needed.
> 
> And yes, we get all the same confidence intervals with params on the dashboard
-
> as they're attached to regular groups.

Okay SGTM.

[Alexei Svitkine (slow), 2015-02-13 16:50:51]

New patchsets have been uploaded after l-g-t-m from gab@chromium.org

[Alexei Svitkine (slow), 2015-02-13 16:53:45]

asvitkine@chromium.org changed reviewers:
+ mattm@chromium.org

[Alexei Svitkine (slow), 2015-02-13 16:53:45]

+mattm for safe_browsing OWNERS

gab@, PTAL. I re-worked this a little bit. I found in my testing that with the
previous version, some DCHECKs(enabled_); were being triggered.

Instead of adding early returns on enabled_, which could potentially reduce
DCHECK coverage for the non-lightspeed code paths, I added early returns to
those functions based on the same field trial lightspeed check.

With this patch set, I haven't noticed any issues running a debug build locally,
so I think this should be good to try for the canary experiment.

[gab, 2015-02-13 18:09:24]

On 2015/02/13 16:53:45, Alexei Svitkine wrote:
> +mattm for safe_browsing OWNERS
> 
> gab@, PTAL. I re-worked this a little bit. I found in my testing that with the
> previous version, some DCHECKs(enabled_); were being triggered.
> 
> Instead of adding early returns on enabled_, which could potentially reduce
> DCHECK coverage for the non-lightspeed code paths, I added early returns to
> those functions based on the same field trial lightspeed check.
> 
> With this patch set, I haven't noticed any issues running a debug build
locally,
> so I think this should be good to try for the canary experiment.

Patch set 3 lgtm, another option would to disable what kicks off the update in
SafeBrowsingProtocolManager(), but since this is only a rough experiment for
Canary, it's probably okay to keep update payloads without processing them for a
few days (this may be a little more traffic than when SafeBrowsing is on though
as over time you'll get bigger and bigger diffs from the server without ever
adjusting the local state -- but, again, probably fine on Canary for a few
days).

[gab, 2015-02-13 18:09:35]

https://codereview.chromium.org/924543002/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/database_manager.cc (right):

https://codereview.chromium.org/924543002/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/database_manager.cc:174: static bool disable_sb =
DCHECK_CURRENTLY_ON(BrowserThread::IO);

since a static variable isn't thread-safe.

[Alexei Svitkine (slow), 2015-02-13 18:45:57]

New patchsets have been uploaded after l-g-t-m from gab@chromium.org

[Alexei Svitkine (slow), 2015-02-13 18:47:34]

Patchset #4 (id:80001) has been deleted

[Alexei Svitkine (slow), 2015-02-13 19:00:08]

https://codereview.chromium.org/924543002/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/database_manager.cc (right):

https://codereview.chromium.org/924543002/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/database_manager.cc:174: static bool disable_sb =
On 2015/02/13 18:09:35, gab wrote:
> DCHECK_CURRENTLY_ON(BrowserThread::IO);
> 
> since a static variable isn't thread-safe.

Done.

[mattm, 2015-02-13 22:16:46]

Seems like it would be much simpler to put in  SafeBrowsingService::RefreshState
and just force enable=false.

Can variations change during execution, or are they only changed when you
restart?

[gab, 2015-02-13 22:27:28]

On 2015/02/13 22:16:46, mattm wrote:
> Seems like it would be much simpler to put in 
SafeBrowsingService::RefreshState
> and just force enable=false.
> 
> Can variations change during execution, or are they only changed when you
> restart?

Would that prevent the database from being loaded though? (given we want to test
the impact on startup, that's key)

[mattm, 2015-02-13 22:32:13]

On 2015/02/13 22:27:28, gab wrote:
> On 2015/02/13 22:16:46, mattm wrote:
> > Seems like it would be much simpler to put in 
> SafeBrowsingService::RefreshState
> > and just force enable=false.
> > 
> > Can variations change during execution, or are they only changed when you
> > restart?
> 
> Would that prevent the database from being loaded though? (given we want to
test
> the impact on startup, that's key)

Yeah, it should.

[Alexei Svitkine (slow), 2015-02-13 22:51:35]

Indeed that looks cleaner. Updated CL coming momentarily.

[gab, 2015-02-13 22:58:21]

https://codereview.chromium.org/924543002/diff/100001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/database_manager.cc (right):

https://codereview.chromium.org/924543002/diff/100001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/database_manager.cc:176:
!variations::GetVariationParamValue("LightSpeed", "DisableSB").empty();
Hmmm actually, isn't this boolean logic backwards? i.e. either this bool should
be named |enable_sb| or their shouldn't be a '!'?

I think you want to drop the '!' and keep the rest.

[Alexei Svitkine (slow), 2015-02-13 22:58:40]

Done. PTAL.

[gab, 2015-02-13 23:00:09]

https://codereview.chromium.org/924543002/diff/100001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/database_manager.cc (right):

https://codereview.chromium.org/924543002/diff/100001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/database_manager.cc:176:
!variations::GetVariationParamValue("LightSpeed", "DisableSB").empty();
On 2015/02/13 22:58:21, gab wrote:
> Hmmm actually, isn't this boolean logic backwards? i.e. either this bool
should
> be named |enable_sb| or their shouldn't be a '!'?
> 
> I think you want to drop the '!' and keep the rest.

Oh nvm, it's !empty(), i.e. disabled == not not disabled...! Or enabled = not
not not disabled :-)!

Ugh..!

[gab, 2015-02-13 23:01:36]

lgtm

https://codereview.chromium.org/924543002/diff/100001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/database_manager.cc (right):

https://codereview.chromium.org/924543002/diff/100001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/database_manager.cc:176:
!variations::GetVariationParamValue("LightSpeed", "DisableSB").empty();
On 2015/02/13 23:00:09, gab wrote:
> On 2015/02/13 22:58:21, gab wrote:
> > Hmmm actually, isn't this boolean logic backwards? i.e. either this bool
> should
> > be named |enable_sb| or their shouldn't be a '!'?
> > 
> > I think you want to drop the '!' and keep the rest.
> 
> Oh nvm, it's !empty(), i.e. disabled == not not disabled...! Or enabled = not
> not not disabled :-)!
> 
> Ugh..!

Would be nice to have a boolean method on that API rather than have to check
!empty(), just saying :-)!

[Alexei Svitkine (slow), 2015-02-13 23:03:03]

On 2015/02/13 23:01:36, gab wrote:
> lgtm
> 
>
https://codereview.chromium.org/924543002/diff/100001/chrome/browser/safe_bro...
> File chrome/browser/safe_browsing/database_manager.cc (right):
> 
>
https://codereview.chromium.org/924543002/diff/100001/chrome/browser/safe_bro...
> chrome/browser/safe_browsing/database_manager.cc:176:
> !variations::GetVariationParamValue("LightSpeed", "DisableSB").empty();
> On 2015/02/13 23:00:09, gab wrote:
> > On 2015/02/13 22:58:21, gab wrote:
> > > Hmmm actually, isn't this boolean logic backwards? i.e. either this bool
> > should
> > > be named |enable_sb| or their shouldn't be a '!'?
> > > 
> > > I think you want to drop the '!' and keep the rest.
> > 
> > Oh nvm, it's !empty(), i.e. disabled == not not disabled...! Or enabled =
not
> > not not disabled :-)!
> > 
> > Ugh..!
> 
> Would be nice to have a boolean method on that API rather than have to check
> !empty(), just saying :-)!

Yep, I agree.

[mattm, 2015-02-13 23:06:39]

lgtm

[Alexei Svitkine (slow), 2015-02-13 23:11:28]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2015-02-13 23:13:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/924543002/120001

[commit-bot: I haz the power, 2015-02-14 00:07:58]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2015-02-14 00:08:27]

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