Create a --disable-top-sites flag for use by telemetry tests

Create a --disable-top-sites flag for use by telemetry tests

BUG=588745
Committed: https://crrev.com/f7d11512831495b87ab1fa9cc3bcd92b432d29fc
Cr-Commit-Position: refs/heads/master@{#402758}

[Marc Treib, 2016-03-17 13:13:11]

treib@chromium.org changed reviewers:
+ sky@chromium.org

[Marc Treib, 2016-03-17 13:13:11]

PTAL! Context on the bug.

[sky, 2016-03-17 21:03:25]

I'm not opposed to adding this switch, but I want to understand why we want to.
If TopSites is impacting performance should that be fixed rather than disabling
it when we run perf testing?

[Marc Treib, 2016-03-18 09:15:48]

On 2016/03/17 21:03:25, sky wrote:
> I'm not opposed to adding this switch, but I want to understand why we want
to.
> If TopSites is impacting performance should that be fixed rather than
disabling
> it when we run perf testing?

From what I understand, there are some tests during which a TopSites update is
triggered *sometimes*. Clearly that introduces noise, especially since the thing
being tested takes less time than TopSites. IMO just disabling TopSites for
those kinds of tests isn't unreasonable (though TBH, I'm somewhat surprised that
there aren't 100 other things that trigger "some of the time" and mess up the
performance data...)

[sky, 2016-03-18 17:12:55]

Is the expectation top sites is disabled for *all* telemetry tests, or
just some? Topsites is hardly new, why is causing more noise now?

  -Scott

On Fri, Mar 18, 2016 at 2:15 AM,  <treib@chromium.org> wrote:
> On 2016/03/17 21:03:25, sky wrote:
>> I'm not opposed to adding this switch, but I want to understand why we
>> want
> to.
>> If TopSites is impacting performance should that be fixed rather than
> disabling
>> it when we run perf testing?
>
> From what I understand, there are some tests during which a TopSites update
> is
> triggered *sometimes*. Clearly that introduces noise, especially since the
> thing
> being tested takes less time than TopSites. IMO just disabling TopSites for
> those kinds of tests isn't unreasonable (though TBH, I'm somewhat surprised
> that
> there aren't 100 other things that trigger "some of the time" and mess up
> the
> performance data...)
>
> https://codereview.chromium.org/1809603005/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Marc Treib, 2016-03-18 17:22:35]

I don't know - only some, I'd expect. I'll ask on the bug, hopefully the people
who ran into this have some insights.
Thanks!

On 2016/03/18 17:12:55, sky wrote:
> Is the expectation top sites is disabled for *all* telemetry tests, or
> just some? Topsites is hardly new, why is causing more noise now?
> 
>   -Scott
> 
> On Fri, Mar 18, 2016 at 2:15 AM,  <mailto:treib@chromium.org> wrote:
> > On 2016/03/17 21:03:25, sky wrote:
> >> I'm not opposed to adding this switch, but I want to understand why we
> >> want
> > to.
> >> If TopSites is impacting performance should that be fixed rather than
> > disabling
> >> it when we run perf testing?
> >
> > From what I understand, there are some tests during which a TopSites update
> > is
> > triggered *sometimes*. Clearly that introduces noise, especially since the
> > thing
> > being tested takes less time than TopSites. IMO just disabling TopSites for
> > those kinds of tests isn't unreasonable (though TBH, I'm somewhat surprised
> > that
> > there aren't 100 other things that trigger "some of the time" and mess up
> > the
> > performance data...)
> >
> > https://codereview.chromium.org/1809603005/

[sky, 2016-03-18 17:30:11]

Can you outline which sets of test it would run on?

I'm of the opinion we shouldn't need to disable top sites for tests
that are meant to measure overall performance. If this test doesn't do
that, them ok with adding and using the flag. OTOH if we have such a
switch it's easy to misuse and not end up measuring overall
performance.

  -Scott

On Fri, Mar 18, 2016 at 10:22 AM,  <treib@chromium.org> wrote:
> I don't know - only some, I'd expect. I'll ask on the bug, hopefully the
> people
> who ran into this have some insights.
> Thanks!
>
> On 2016/03/18 17:12:55, sky wrote:
>> Is the expectation top sites is disabled for *all* telemetry tests, or
>> just some? Topsites is hardly new, why is causing more noise now?
>>
>> -Scott
>>
>> On Fri, Mar 18, 2016 at 2:15 AM, <mailto:treib@chromium.org> wrote:
>> > On 2016/03/17 21:03:25, sky wrote:
>> >> I'm not opposed to adding this switch, but I want to understand why we
>> >> want
>> > to.
>> >> If TopSites is impacting performance should that be fixed rather than
>> > disabling
>> >> it when we run perf testing?
>> >
>> > From what I understand, there are some tests during which a TopSites
>> > update
>> > is
>> > triggered *sometimes*. Clearly that introduces noise, especially since
>> > the
>> > thing
>> > being tested takes less time than TopSites. IMO just disabling TopSites
>> > for
>> > those kinds of tests isn't unreasonable (though TBH, I'm somewhat
>> > surprised
>> > that
>> > there aren't 100 other things that trigger "some of the time" and mess
>> > up
>> > the
>> > performance data...)
>> >
>> > https://codereview.chromium.org/1809603005/
>
>
> https://codereview.chromium.org/1809603005/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[nednguyen, 2016-03-21 15:52:08]

nednguyen@google.com changed reviewers:
+ nednguyen@google.com, vmiura@chromium.org

[Marc Treib, 2016-06-17 12:44:18]

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

[commit-bot: I haz the power, 2016-06-17 12:44:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1809603005/20001

[Marc Treib, 2016-06-17 12:49:54]

Reviving this old CL, as there has finally been an update on the bug. CIL

On 2016/03/18 17:30:11, sky wrote:
> Can you outline which sets of test it would run on?

See https://bugs.chromium.org/p/chromium/issues/detail?id=588745#c40 - tl;dr,
this is for some performance tests that measure animation timings. The measured
times are on the order of milliseconds, so a "random" TopSites updates that also
takes a few milliseconds completely messes up the measurements.

> I'm of the opinion we shouldn't need to disable top sites for tests
> that are meant to measure overall performance. If this test doesn't do
> that, them ok with adding and using the flag. OTOH if we have such a
> switch it's easy to misuse and not end up measuring overall
> performance.

Agreed. Disabling stuff for perf tests is clearly always suboptimal, but for
this kind of test, it seems acceptable to me (and I don't see any better
alternative).

[commit-bot: I haz the power, 2016-06-17 13:23:59]

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

[commit-bot: I haz the power, 2016-06-17 13:24:04]

Dry run: This issue passed the CQ dry run.

[sky, 2016-06-17 15:11:38]

If this test doesn't need to measure chrome performance, but rendering
performance did you consider using content shell? That way it won't be
influenced by any extra processing done in chrome.

  -Scott

On Fri, Jun 17, 2016 at 5:49 AM,  <treib@chromium.org> wrote:
> Reviving this old CL, as there has finally been an update on the bug. CIL
>
> On 2016/03/18 17:30:11, sky wrote:
>> Can you outline which sets of test it would run on?
>
> See https://bugs.chromium.org/p/chromium/issues/detail?id=588745#c40 -
> tl;dr,
> this is for some performance tests that measure animation timings. The
> measured
> times are on the order of milliseconds, so a "random" TopSites updates that
> also
> takes a few milliseconds completely messes up the measurements.
>
>> I'm of the opinion we shouldn't need to disable top sites for tests
>> that are meant to measure overall performance. If this test doesn't do
>> that, them ok with adding and using the flag. OTOH if we have such a
>> switch it's easy to misuse and not end up measuring overall
>> performance.
>
> Agreed. Disabling stuff for perf tests is clearly always suboptimal, but for
> this kind of test, it seems acceptable to me (and I don't see any better
> alternative).
>
> https://codereview.chromium.org/1809603005/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[vmiura, 2016-06-17 16:26:54]

Description was changed from

==========
Create a --disable-top-sites flag for use by telemetry tests

BUG=588745
==========

to

==========
Create a --disable-top-sites flag for use by telemetry tests

BUG=588745
==========

[vmiura, 2016-06-17 16:55:29]

On 2016/06/17 15:11:38, sky wrote:
> If this test doesn't need to measure chrome performance, but rendering
> performance did you consider using content shell? That way it won't be
> influenced by any extra processing done in chrome.

This is an option, modulo telemetry & perf waterfall support.

I'm not sure switching away from testing chrome_public is the best direction. 
This benchmark tests CPU time per-frame during scrolling and animations, so it's
not ideal for measuring top_sites update.  That said it's often insightful to
see these "unexpected" tasks or changes.  Usually we ping the team, check if
it's anything to worry about and move on.  It's good we noticed this issue.

Outcomes that would be good to get out of this bug:
 - Remove the noise for this task if possible, either disable it or change the
benchmark to better categorize these periodic tasks.
 - Track the issue.  Ensure top_sites has perf tests, UMA stats on top_sites
update if we don't have them alreay.
 - Get scheduler team thinking about how we can avoid running background browser
tasks while rendering is in the "animation" phase.

[vmiura, 2016-06-17 17:18:57]

On 2016/06/17 16:55:29, vmiura wrote:
> On 2016/06/17 15:11:38, sky wrote:
> > If this test doesn't need to measure chrome performance, but rendering
> > performance did you consider using content shell? That way it won't be
> > influenced by any extra processing done in chrome.
> 
> This is an option, modulo telemetry & perf waterfall support.
> 
> I'm not sure switching away from testing chrome_public is the best direction. 
> This benchmark tests CPU time per-frame during scrolling and animations, so
it's
> not ideal for measuring top_sites update.  That said it's often insightful to
> see these "unexpected" tasks or changes.  Usually we ping the team, check if
> it's anything to worry about and move on.  It's good we noticed this issue.
> 
> Outcomes that would be good to get out of this bug:
>  - Remove the noise for this task if possible, either disable it or change the
> benchmark to better categorize these periodic tasks.
>  - Track the issue.  Ensure top_sites has perf tests, UMA stats on top_sites
> update if we don't have them alreay.
>  - Get scheduler team thinking about how we can avoid running background
browser
> tasks while rendering is in the "animation" phase.

Filed some issues for those:

 crbug.com/621125
 crbug.com/621128

[sky, 2016-06-17 17:56:47]

So, where does that leave this patch? Disabling random features does
not sit well to me for the reasons mentioned earlier.

  -Scott

On Fri, Jun 17, 2016 at 10:18 AM,  <vmiura@chromium.org> wrote:
> On 2016/06/17 16:55:29, vmiura wrote:
>> On 2016/06/17 15:11:38, sky wrote:
>> > If this test doesn't need to measure chrome performance, but rendering
>> > performance did you consider using content shell? That way it won't be
>> > influenced by any extra processing done in chrome.
>>
>> This is an option, modulo telemetry & perf waterfall support.
>>
>> I'm not sure switching away from testing chrome_public is the best
>> direction.
>> This benchmark tests CPU time per-frame during scrolling and animations,
>> so
> it's
>> not ideal for measuring top_sites update. That said it's often insightful
>> to
>> see these "unexpected" tasks or changes. Usually we ping the team, check
>> if
>> it's anything to worry about and move on. It's good we noticed this issue.
>>
>> Outcomes that would be good to get out of this bug:
>> - Remove the noise for this task if possible, either disable it or change
>> the
>> benchmark to better categorize these periodic tasks.
>> - Track the issue. Ensure top_sites has perf tests, UMA stats on top_sites
>> update if we don't have them alreay.
>> - Get scheduler team thinking about how we can avoid running background
> browser
>> tasks while rendering is in the "animation" phase.
>
> Filed some issues for those:
>
> crbug.com/621125
> crbug.com/621128
>
> https://codereview.chromium.org/1809603005/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[vmiura, 2016-06-17 18:54:48]

On 2016/06/17 17:56:47, sky wrote:
> So, where does that leave this patch? Disabling random features does
> not sit well to me for the reasons mentioned earlier.

Disabling things is not ideal, but removing noise and flakes from tests is
important.  This is not getting the urgency it should have. Test flakes need
quick revert / disable, and file issue on the owner of the system to fix it.

[sky, 2016-06-17 19:51:27]

Top sites code has not changed significantly in a long time.

  -Scott

On Fri, Jun 17, 2016 at 11:54 AM,  <vmiura@chromium.org> wrote:
> On 2016/06/17 17:56:47, sky wrote:
>> So, where does that leave this patch? Disabling random features does
>> not sit well to me for the reasons mentioned earlier.
>
> Disabling things is not ideal, but removing noise and flakes from tests is
> important. This is not getting the urgency it should have. Test flakes need
> quick revert / disable, and file issue on the owner of the system to fix it.
>
> https://codereview.chromium.org/1809603005/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[vmiura, 2016-06-17 22:51:38]

On 2016/06/17 19:51:27, sky wrote:
> Top sites code has not changed significantly in a long time.
Agreed, TopSites hasn't changed, it is only showing up due to shifts in timing. 
The update runs at an interval between 0 and 5min on Android, based on some
heuristic.  It occurs while a random test is measuring.

To answer original concerns:

* Why do we want to disable TopSites instead of fixing the performance?
 - TopSites is taking enough time to cause jank, so let's investigate.
 - In the mean time, fixing determinism in this test suite needs some
intervention, whether turning off or adding other levers to TopSites.

* We shouldn't disable TopSites on tests that are meant to measure overall
performance.
 - It's the ideal, but there are many trade-offs to improve the signal-to-noise
ratio in these tests (e.g clear data caches, lock CPU frequency, wait for low
CPU temperature, wait for page load quiescence).
 - From the folks who use and maintain these perf tests, it seems OK to disable
TopSites with a note pointing to the scheduling Issue.

Thanks

[sky, 2016-06-17 23:21:43]

Ok, you convinced me (and jam@ convinced me too). I'm wondering if there is a
more central way to disable top sites. Say making TopSitesFactory::GetInstance
returning null?

[Marc Treib, 2016-06-20 09:10:32]

On 2016/06/17 23:21:43, sky wrote:
> Ok, you convinced me (and jam@ convinced me too). I'm wondering if there is a
> more central way to disable top sites. Say making TopSitesFactory::GetInstance
> returning null?

There's some places that assume TopSites can't be null (for non-incognito
profiles), e.g. MostVisitedSites:
https://cs.chromium.org/chromium/src/components/ntp_tiles/most_visited_sites....
Maybe those should be changed?

[sky, 2016-06-20 15:21:33]

Seems like that class shouldn't be created either if you're disabling topsites.

On Mon, Jun 20, 2016 at 2:10 AM,  <treib@chromium.org> wrote:
> On 2016/06/17 23:21:43, sky wrote:
>> Ok, you convinced me (and jam@ convinced me too). I'm wondering if there
>> is a
>> more central way to disable top sites. Say making
>> TopSitesFactory::GetInstance
>> returning null?
>
> There's some places that assume TopSites can't be null (for non-incognito
> profiles), e.g. MostVisitedSites:
>
https://cs.chromium.org/chromium/src/components/ntp_tiles/most_visited_sites....
> Maybe those should be changed?
>
> https://codereview.chromium.org/1809603005/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Marc Treib, 2016-06-20 15:41:01]

MostVisitedSites is essentially a mixer, combining suggestions from various
sources, TopSites being one of them. So it should still be created even if
TopSites is inactive.
Anyway, that was just an example. My point was that so far, TopSites
couldn't be null. If we change that, then potentially lots of places need
to be updated to cope with that. Though to be fair, it looks like most
(all?) other call sites already check for it being null.

On Mon, Jun 20, 2016 at 5:21 PM Scott Violet <sky@chromium.org> wrote:

> Seems like that class shouldn't be created either if you're disabling
> topsites.
>
> On Mon, Jun 20, 2016 at 2:10 AM,  <treib@chromium.org> wrote:
> > On 2016/06/17 23:21:43, sky wrote:
> >> Ok, you convinced me (and jam@ convinced me too). I'm wondering if
> there
> >> is a
> >> more central way to disable top sites. Say making
> >> TopSitesFactory::GetInstance
> >> returning null?
> >
> > There's some places that assume TopSites can't be null (for non-incognito
> > profiles), e.g. MostVisitedSites:
> >
>
https://cs.chromium.org/chromium/src/components/ntp_tiles/most_visited_sites....
> > Maybe those should be changed?
> >
> > https://codereview.chromium.org/1809603005/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2016-06-20 16:45:55]

I think most should check given topsites is null when incognito.

On Mon, Jun 20, 2016 at 8:40 AM, Marc Treib <treib@chromium.org> wrote:
> MostVisitedSites is essentially a mixer, combining suggestions from various
> sources, TopSites being one of them. So it should still be created even if
> TopSites is inactive.
> Anyway, that was just an example. My point was that so far, TopSites
> couldn't be null. If we change that, then potentially lots of places need to
> be updated to cope with that. Though to be fair, it looks like most (all?)
> other call sites already check for it being null.
>
> On Mon, Jun 20, 2016 at 5:21 PM Scott Violet <sky@chromium.org> wrote:
>>
>> Seems like that class shouldn't be created either if you're disabling
>> topsites.
>>
>> On Mon, Jun 20, 2016 at 2:10 AM,  <treib@chromium.org> wrote:
>> > On 2016/06/17 23:21:43, sky wrote:
>> >> Ok, you convinced me (and jam@ convinced me too). I'm wondering if
>> >> there
>> >> is a
>> >> more central way to disable top sites. Say making
>> >> TopSitesFactory::GetInstance
>> >> returning null?
>> >
>> > There's some places that assume TopSites can't be null (for
>> > non-incognito
>> > profiles), e.g. MostVisitedSites:
>> >
>> >
https://cs.chromium.org/chromium/src/components/ntp_tiles/most_visited_sites....
>> > Maybe those should be changed?
>> >
>> > https://codereview.chromium.org/1809603005/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Marc Treib, 2016-06-28 13:36:06]

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

[commit-bot: I haz the power, 2016-06-28 13:36:23]

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-06-28 14:24:55]

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

[commit-bot: I haz the power, 2016-06-28 14:24:56]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2016-06-28 16:15:49]

On 2016/06/20 16:45:55, sky wrote:
> I think most should check given topsites is null when incognito.

Alright, done. I added null checks in MostVisitedSites; nothing else seems to
break. PTAL again!

[sky, 2016-06-28 19:08:28]

LGTM

[nednguyen, 2016-06-28 19:10:55]

On 2016/06/28 19:08:28, sky wrote:
> LGTM

non owners lgtm

[Marc Treib, 2016-06-29 09:01:48]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2016-06-29 09:02:13]

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-06-29 09:06:34]

Description was changed from

==========
Create a --disable-top-sites flag for use by telemetry tests

BUG=588745
==========

to

==========
Create a --disable-top-sites flag for use by telemetry tests

BUG=588745
==========

[commit-bot: I haz the power, 2016-06-29 09:06:35]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-29 09:08:15]

Description was changed from

==========
Create a --disable-top-sites flag for use by telemetry tests

BUG=588745
==========

to

==========
Create a --disable-top-sites flag for use by telemetry tests

BUG=588745

Committed: https://crrev.com/f7d11512831495b87ab1fa9cc3bcd92b432d29fc
Cr-Commit-Position: refs/heads/master@{#402758}
==========

[commit-bot: I haz the power, 2016-06-29 09:08:17]

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