Avoid doing periodic prerender cleanup when there are no contents.

Avoid doing periodic prerender cleanup when there are no contents.

This eliminates about 30 idle wakeups in the tab_switching.top_10 energy benchmark.

BUG=411489
Committed: https://crrev.com/9461b64ca4b0d4bb8adac97306621ff1f62f21eb
Cr-Commit-Position: refs/heads/master@{#295311}

[tonyg, 2014-09-05 21:04:20]

tonyg@chromium.org changed reviewers:
+ mmenke@chromium.org, tburkard@chromium.org

[tonyg, 2014-09-05 21:10:26]

Hey Matt, Timo,

I noticed this periodic timer while studying our idle energy benchmarks. It's
probably not a huge deal on its own, but I'm hoping a lot of similar changes
like this will add up. It looks to me like it's not necessary at all in my
scenario since there're no web contents, but plmk if I'm missing something.

There's a pending perf tryjob for this here:
https://codereview.chromium.org/547703003/

-Tony

[mmenke, 2014-09-05 21:20:02]

On 2014/09/05 21:10:26, tonyg wrote:
> Hey Matt, Timo,
> 
> I noticed this periodic timer while studying our idle energy benchmarks. It's
> probably not a huge deal on its own, but I'm hoping a lot of similar changes
> like this will add up. It looks to me like it's not necessary at all in my
> scenario since there're no web contents, but plmk if I'm missing something.
> 
> There's a pending perf tryjob for this here:
> https://codereview.chromium.org/547703003/
> 
> -Tony

I'd argue it makes more sense to just get rid of the logic to keep around
Prerenders without WebContents.  It used to be we'd use that for rate limiting,
but now we only use it for histograms for "field trials" where prerendering is
enabled but not used, and I'd argue it's a lot of ugliness for histograms,
particularly those that haven't been looked at in ages.

[mmenke, 2014-09-05 21:20:56]

On 2014/09/05 21:20:02, mmenke wrote:
> On 2014/09/05 21:10:26, tonyg wrote:
> > Hey Matt, Timo,
> > 
> > I noticed this periodic timer while studying our idle energy benchmarks.
It's
> > probably not a huge deal on its own, but I'm hoping a lot of similar changes
> > like this will add up. It looks to me like it's not necessary at all in my
> > scenario since there're no web contents, but plmk if I'm missing something.
> > 
> > There's a pending perf tryjob for this here:
> > https://codereview.chromium.org/547703003/
> > 
> > -Tony
> 
> I'd argue it makes more sense to just get rid of the logic to keep around
> Prerenders without WebContents.  It used to be we'd use that for rate
limiting,
> but now we only use it for histograms for "field trials" where prerendering is
> enabled but not used, and I'd argue it's a lot of ugliness for histograms,
> particularly those that haven't been looked at in ages.

Also, this CL will break those histograms - another reason for just removing
that behavior.

[tonyg, 2014-09-05 21:23:29]

> I'd argue it makes more sense to just get rid of the logic to keep around
> Prerenders without WebContents.

Shall I go ahead and put together a patch to do that or is there anyone else
we'd want to check with first?

[mmenke, 2014-09-05 21:28:36]

On 2014/09/05 21:23:29, tonyg wrote:
> > I'd argue it makes more sense to just get rid of the logic to keep around
> > Prerenders without WebContents.
> 
> Shall I go ahead and put together a patch to do that or is there anyone else
> we'd want to check with first?

You should definitely wait for Timo to weigh in.  I've always thought those
histograms were too heavy-weight to keep around, but Timo has argued otherwise
in the past.

[tburkard, 2014-09-10 08:09:49]

On 2014/09/05 21:28:36, mmenke wrote:
> On 2014/09/05 21:23:29, tonyg wrote:
> > > I'd argue it makes more sense to just get rid of the logic to keep around
> > > Prerenders without WebContents.
> > 
> > Shall I go ahead and put together a patch to do that or is there anyone else
> > we'd want to check with first?
> 
> You should definitely wait for Timo to weigh in.  I've always thought those
> histograms were too heavy-weight to keep around, but Timo has argued otherwise
> in the past.

I think the issue is that we'd lose all the control group histograms. This would
basically make it impossible to evaluate the benefits of prerendering any more.

Do we have stats how often these recurring cleanups occur with non-content
prerenders?

Based on how small our control group is, I'd think it should be pretty small?

If not, we should figure out why first.

Do you have numbers on how often (as a fraction) cleanups are scheduled without
any contents?

Thanks.

[tburkard, 2014-09-10 08:10:09]

Btw, sorry about the delayed response, but I was on vacation on Monday and
Tuesday.

[mmenke, 2014-09-10 13:58:03]

On 2014/09/10 08:09:49, tburkard wrote:
> On 2014/09/05 21:28:36, mmenke wrote:
> > On 2014/09/05 21:23:29, tonyg wrote:
> > > > I'd argue it makes more sense to just get rid of the logic to keep
around
> > > > Prerenders without WebContents.
> > > 
> > > Shall I go ahead and put together a patch to do that or is there anyone
else
> > > we'd want to check with first?
> > 
> > You should definitely wait for Timo to weigh in.  I've always thought those
> > histograms were too heavy-weight to keep around, but Timo has argued
otherwise
> > in the past.
> 
> I think the issue is that we'd lose all the control group histograms. This
would
> basically make it impossible to evaluate the benefits of prerendering any
more.
> 
> Do we have stats how often these recurring cleanups occur with non-content
> prerenders?
> 
> Based on how small our control group is, I'd think it should be pretty small?
> 
> If not, we should figure out why first.
> 
> Do you have numbers on how often (as a fraction) cleanups are scheduled
without
> any contents?
> 
> Thanks.

That's not the only case where we have cleanups without any contents - the more
common case is presumably the "MATCH_COMPLETE_REPLACEMENT" case.

[tburkard, 2014-09-11 09:28:54]

On 2014/09/10 13:58:03, mmenke wrote:
> On 2014/09/10 08:09:49, tburkard wrote:
> > On 2014/09/05 21:28:36, mmenke wrote:
> > > On 2014/09/05 21:23:29, tonyg wrote:
> > > > > I'd argue it makes more sense to just get rid of the logic to keep
> around
> > > > > Prerenders without WebContents.
> > > > 
> > > > Shall I go ahead and put together a patch to do that or is there anyone
> else
> > > > we'd want to check with first?
> > > 
> > > You should definitely wait for Timo to weigh in.  I've always thought
those
> > > histograms were too heavy-weight to keep around, but Timo has argued
> otherwise
> > > in the past.
> > 
> > I think the issue is that we'd lose all the control group histograms. This
> would
> > basically make it impossible to evaluate the benefits of prerendering any
> more.
> > 
> > Do we have stats how often these recurring cleanups occur with non-content
> > prerenders?
> > 
> > Based on how small our control group is, I'd think it should be pretty
small?
> > 
> > If not, we should figure out why first.
> > 
> > Do you have numbers on how often (as a fraction) cleanups are scheduled
> without
> > any contents?
> > 
> > Thanks.
> 
> That's not the only case where we have cleanups without any contents - the
more
> common case is presumably the "MATCH_COMPLETE_REPLACEMENT" case.

Good point.

Here is one proposal which would not reduce any stats but should greatly improve
this then:
Have 1% control, 1% prerender+match_complete_replacement, and 98% prerender w/o
match complete replacement. That way, we can compare across the two 1% groups.

This should address everything while keeping our histograms pretty much
identical for analysis purposes.

Thoughts?

[tonyg, 2014-09-13 01:12:01]

Good idea. I've updated the patch to the experiment-based approach. Is this
patch enough to make that happen or do I have to change something in the
experiment configs as well?

https://codereview.chromium.org/550453002/diff/20001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_field_trial.cc (right):

https://codereview.chromium.org/550453002/diff/20001/chrome/browser/prerender...
chrome/browser/prerender/prerender_field_trial.cc:129:
dev_experiment_multi_prerender_probability = 200;
I made these up. PLMK what %ages you'd like.

[mmenke, 2014-09-15 14:10:34]

On 2014/09/13 01:12:01, tonyg wrote:
> Good idea. I've updated the patch to the experiment-based approach. Is this
> patch enough to make that happen or do I have to change something in the
> experiment configs as well?
> 
>
https://codereview.chromium.org/550453002/diff/20001/chrome/browser/prerender...
> File chrome/browser/prerender/prerender_field_trial.cc (right):
> 
>
https://codereview.chromium.org/550453002/diff/20001/chrome/browser/prerender...
> chrome/browser/prerender/prerender_field_trial.cc:129:
> dev_experiment_multi_prerender_probability = 200;
> I made these up. PLMK what %ages you'd like.

I'm just going to defer to Timo on this review.

[tburkard, 2014-09-16 12:54:21]

Looks good so far, however, I want to think about this for a few more days, if
that's ok? Or is this urgent?

Also, histograms.xml must be updated (due to expanding the enum). It's usually a
lot of copy and paste. Search for the string "Prerender5minTTL" and duplicate
that.

Likewise, PRERENDER_MODE needs to be augmented.

[tonyg, 2014-09-16 16:04:32]

This isn't particularly urgent, but I'd like to make sure it doesn't languish.
Could we shoot for landing something that fixes these periodic wakes by the end
of the week?

I'm pretty ambivalent about the approach and would be fine if we go with this
patch, another approach you suggest, or if you post something and we abandon
this patch.

Anyway, just in case, I updated histograms.xml as you suggested in the latest
version.

[tburkard, 2014-09-16 18:23:19]

Sounds good, I will wrap it up in the next day or two, so that it can be
landed by the end of the week.

On Tue, Sep 16, 2014 at 6:04 PM, <tonyg@chromium.org> wrote:

> This isn't particularly urgent, but I'd like to make sure it doesn't
> languish.
> Could we shoot for landing something that fixes these periodic wakes by
> the end
> of the week?
>
> I'm pretty ambivalent about the approach and would be fine if we go with
> this
> patch, another approach you suggest, or if you post something and we
> abandon
> this patch.
>
> Anyway, just in case, I updated histograms.xml as you suggested in the
> latest
> version.
>
> https://codereview.chromium.org/550453002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tburkard, 2014-09-17 09:23:20]

LGTM if you incorporate the updated probabilities below.

Thanks.

https://codereview.chromium.org/550453002/diff/60001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_field_trial.cc (right):

https://codereview.chromium.org/550453002/diff/60001/chrome/browser/prerender...
chrome/browser/prerender/prerender_field_trial.cc:133:
dev_experiment_match_complete_probability = 100;
It's best if the #'s are the same, for easier comparison.
I would prefer:
Enabled=200
Control=200
Multi=200
15min=100
nouse=100
MatchComplete=200

Thanks.

[tonyg, 2014-09-17 14:44:59]

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

[tonyg, 2014-09-17 14:45:54]

On 2014/09/17 09:23:20, tburkard wrote:
> LGTM if you incorporate the updated probabilities below.

Done and thank you.

Alexei, requesting your approval of histograms.xml

[Alexei Svitkine (slow), 2014-09-17 14:56:49]

LGTM I guess, but this field trial and histograms really need to be overhauled.

It's one of the last uses of the client-side FieldTrial API, rather than using
the more flexible server-side control.

And it looks like its defining a new version of each histogram depending on the
field trial group, which is completely unnecessary to do - as both the Finch
dashboard and histogram dashboard lets you look at a histogram for a specific
field trial group, so none of this extra client code complexity is necessary.

[tonyg, 2014-09-17 14:58:06]

The CQ bit was checked by tonyg@chromium.org

[commit-bot: I haz the power, 2014-09-17 14:58:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/550453002/80001

[davidben, 2014-09-17 15:03:09]

On 2014/09/17 14:56:49, Alexei Svitkine wrote:
> LGTM I guess, but this field trial and histograms really need to be
overhauled.
> 
> It's one of the last uses of the client-side FieldTrial API, rather than using
> the more flexible server-side control.
> 
> And it looks like its defining a new version of each histogram depending on
the
> field trial group, which is completely unnecessary to do - as both the Finch
> dashboard and histogram dashboard lets you look at a histogram for a specific
> field trial group, so none of this extra client code complexity is necessary.

Filed http://crbug.com/415161 so this is in the bug tracker somewhere.

[commit-bot: I haz the power, 2014-09-17 16:20:07]

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

[commit-bot: I haz the power, 2014-09-17 16:20:08]

Try jobs failed on following builders:
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tonyg, 2014-09-17 17:24:42]

The CQ bit was checked by tonyg@chromium.org

[commit-bot: I haz the power, 2014-09-17 17:25:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/550453002/100001

[commit-bot: I haz the power, 2014-09-17 18:56:12]

Committed patchset #6 (id:100001) as 3664b434f8e43c44d27539477ce4413e5f199663

[commit-bot: I haz the power, 2014-09-17 18:56:47]

Patchset 6 (id:??) landed as
https://crrev.com/9461b64ca4b0d4bb8adac97306621ff1f62f21eb
Cr-Commit-Position: refs/heads/master@{#295311}