Four new pages added to tough_scheduling_cases

Four new pages added to tough_scheduling_cases

BUG=391005
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282740

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284060

[picksi, 2014-07-08 13:08:55]

Please review this code to add new pages to the touch_scheduling_cases to the
page-set.

There is an issue with the pbs page (related to synthetic scrolling) which is
preventing record_wpr from working, dominikg is looking at this.

I'll add this page in once record_wpr is fixed.

[jdduke (slow), 2014-07-08 15:20:53]

On 2014/07/08 13:08:55, picksi wrote:
> Please review this code to add new pages to the touch_scheduling_cases to the
> page-set.
> 
> There is an issue with the pbs page (related to synthetic scrolling) which is
> preventing record_wpr from working, dominikg is looking at this.
> 
> I'll add this page in once record_wpr is fixed.

I've never actually added a page to the test suites (so I'm probably not the
best reviewer here), but in any case thanks for doing this!

[jdduke (slow), 2014-07-08 18:27:01]

miletus@: These tests stress scheduling around page load.  Is there anything we
should be doing to have the telemetry tests inject the synthetic gestures at the
appropriate time?

[Yufeng Shen (Slow to review), 2014-07-08 21:59:46]

On 2014/07/08 18:27:01, jdduke wrote:
> miletus@: These tests stress scheduling around page load.  Is there anything
we
> should be doing to have the telemetry tests inject the synthetic gestures at
the
> appropriate time?

I think we inject synthetic input events after the page finishes loading ?
(Dominik to confirm)
It is tricky to define what's the "appropriate time" if we want to inject events
while page is loading and the synthetic event injection code needs to be
changed to support that.

The good news is that, the following 3 pages 

'http://www.latimes.com',
'http://ftw.usatoday.com/2014/05/spelling-bee-rules-shenanigans',
'http://m.espn.go.com/nhl/rankings'

are still doing busy main thread work even after telemetry considers the page is
loaded
and start to inject synthetic gestures. So from the trace I can see tons of
TouchMove
are piled in the browser waiting for ack from renderer, resulting large input
latency
for the first few GestureScrollUpdate.

(page 'http://io9.com/the-10-most-shocking-characters-that-george-r-r-martin
-1579481155/+tinaamini',  does not have this behavior)

So disregard of how we and telemetry interpret when does the page finish
loading, the
scroll latency for the first few scroll events can still be useful as a
benchmark when
we try to improve the first frame input response time.

The other thing is that even though the first few scroll events have large
latency number,
but the mean_input_event_latency remains reasonable low. So we will need a
separate metric, e.g.
maybe the average of the first 3 input events' latency, to reflect what we are
actually trying
to improve in these cases.

[Yufeng Shen (Slow to review), 2014-07-08 21:59:46]

On 2014/07/08 18:27:01, jdduke wrote:
> miletus@: These tests stress scheduling around page load.  Is there anything
we
> should be doing to have the telemetry tests inject the synthetic gestures at
the
> appropriate time?

I think we inject synthetic input events after the page finishes loading ?
(Dominik to confirm)
It is tricky to define what's the "appropriate time" if we want to inject events
while page is loading and the synthetic event injection code needs to be
changed to support that.

The good news is that, the following 3 pages 

'http://www.latimes.com',
'http://ftw.usatoday.com/2014/05/spelling-bee-rules-shenanigans',
'http://m.espn.go.com/nhl/rankings'

are still doing busy main thread work even after telemetry considers the page is
loaded
and start to inject synthetic gestures. So from the trace I can see tons of
TouchMove
are piled in the browser waiting for ack from renderer, resulting large input
latency
for the first few GestureScrollUpdate.

(page 'http://io9.com/the-10-most-shocking-characters-that-george-r-r-martin
-1579481155/+tinaamini',  does not have this behavior)

So disregard of how we and telemetry interpret when does the page finish
loading, the
scroll latency for the first few scroll events can still be useful as a
benchmark when
we try to improve the first frame input response time.

The other thing is that even though the first few scroll events have large
latency number,
but the mean_input_event_latency remains reasonable low. So we will need a
separate metric, e.g.
maybe the average of the first 3 input events' latency, to reflect what we are
actually trying
to improve in these cases.

[jdduke (slow), 2014-07-08 22:23:30]

On 2014/07/08 21:59:46, Yufeng Shen wrote:
> The other thing is that even though the first few scroll events have large
> latency number,
> but the mean_input_event_latency remains reasonable low. So we will need a
> separate metric, e.g.
> maybe the average of the first 3 input events' latency, to reflect what we are
> actually trying
> to improve in these cases.

Sigh, yeah, perhaps a max_input_event_latency.... or a
95_percentile_input_event_latency.

[Dominik Grewe, 2014-07-09 09:20:10]

On 2014/07/08 21:59:46, Yufeng Shen wrote:
> On 2014/07/08 18:27:01, jdduke wrote:
> > miletus@: These tests stress scheduling around page load.  Is there anything
> we
> > should be doing to have the telemetry tests inject the synthetic gestures at
> the
> > appropriate time?
> 
> I think we inject synthetic input events after the page finishes loading ?
> (Dominik to confirm)

Yes, Telemetry waits until the page has loaded (see [1] for details) before
running any page actions.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...

[picksi, 2014-07-09 10:40:00]

<snip>maybe the average of the first 3 input events' latency, to reflect what we
are actually trying to improve in these cases</snip>

I'll add a metric to track Average/Max/95th Percentile etc. (...whatever seems
to most accurately highlight the issue). I'll also look into the the delay
before scrolling begins, as it doesn't really reflect user behavior and is the
area that we are hoping to make major gains.

[picksi, 2014-07-09 19:54:41]

After thinking about this some more, I'm going to measure how long the
input-latency takes to fall below (or near) the average latency. As we sort out
task prioritisation (and fix other blocking issues) the time to average will
come down. In a perfect world the initial input latency will be the average
latency (16ms?) immediately, and our work will be done!

[Sami, 2014-07-11 14:38:09]

I think we can land this and figure out the metric we want separately. Thus:
lgtm.

[picksi, 2014-07-11 15:04:42]

The CQ bit was checked by picksi@google.com

[commit-bot: I haz the power, 2014-07-11 15:05:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/picksi@chromium.org/372243003/1

[picksi, 2014-07-11 17:44:44]

The CQ bit was checked by picksi@google.com

[commit-bot: I haz the power, 2014-07-11 17:46:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/picksi@chromium.org/372243003/20001

[commit-bot: I haz the power, 2014-07-11 23:13:21]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)

[commit-bot: I haz the power, 2014-07-11 23:55:09]

Change committed as 282740

[tonyg, 2014-07-12 23:02:23]

A revert of this CL has been created in
https://codereview.chromium.org/388053002/ by tonyg@chromium.org.

The reason for reverting is: Causes scheduler.tough_scheduling_cases to fail on
all android bots. Example:
http://chromegw/i/chromium.perf/builders/Android%20Nexus4%20Perf/builds/1081/....

[picksi, 2014-07-15 11:31:11]

Revert of reverted code + removal of reference to an unused WPR file. Assuming
initial failure was down to perf-bots being in a bad status as local builds are
fine.

[Sami, 2014-07-16 10:32:01]

lgtm. Given that this works locally, let's give it another run through the bots
and revert again if there still are issues.

https://codereview.chromium.org/372243003/diff/80001/tools/perf/page_sets/tou...
File tools/perf/page_sets/tough_scheduling_cases.py (right):

https://codereview.chromium.org/372243003/diff/80001/tools/perf/page_sets/tou...
tools/perf/page_sets/tough_scheduling_cases.py:471: 'http://www.latimes.com',
nit: This should use four spaces of indentation.

[picksi, 2014-07-18 09:23:07]

Tabs fixed. Re-committing this now.

https://codereview.chromium.org/372243003/diff/80001/tools/perf/page_sets/tou...
File tools/perf/page_sets/tough_scheduling_cases.py (right):

https://codereview.chromium.org/372243003/diff/80001/tools/perf/page_sets/tou...
tools/perf/page_sets/tough_scheduling_cases.py:471: 'http://www.latimes.com',
On 2014/07/16 10:32:01, Sami wrote:
> nit: This should use four spaces of indentation.

Done.

[picksi, 2014-07-18 09:23:32]

The CQ bit was checked by picksi@google.com

[commit-bot: I haz the power, 2014-07-18 09:26:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/picksi@chromium.org/372243003/100001

[commit-bot: I haz the power, 2014-07-18 11:31:02]

Change committed as 284060

[tonyg, 2014-07-19 20:33:44]

A revert of this CL has been created in
https://codereview.chromium.org/406783002/ by tonyg@chromium.org.

The reason for reverting is: All new pages failing on all android bots.

Example:
http://chromegw/i/chromium.perf/builders/Android%20Nexus4%20Perf/builds/1116/...

We shouldn't try to reland without some kind of unittest coverage (perhaps the
smoke test?).

.

[picksi, 2014-07-22 13:52:47]

There is already *some* unittest coverage for tough_scheduling_cases. The
availability of archived data, credentials and types is checked as part of
page_set_unittests (which calls into PageSetSmokeTest). This gets executed by
run_tests in /tools/perf. I'm not sure if these tests are run as part of the
commit try-bot tests, but they are limited and don't actually run the
performance test.

I assume that, Tony, you're suggesting I add
"tough_scheduling_cases_unittest.py" which would essentially run the perf but
throw away the results (the issue breaking the bots seems to be missing data
being returned at the end of a run, so the unit tests probably can't short-cut
actually running the metrics?). Does there currently exist code from another
unit-test that launches a perf-test? I'm not at all sure how I'd weld the
unit-test running framework in with the perf-test running framework!

[Sami, 2014-07-22 14:02:13]

On 2014/07/22 13:52:47, picksi wrote:
> There is already *some* unittest coverage for tough_scheduling_cases. The
> availability of archived data, credentials and types is checked as part of
> page_set_unittests (which calls into PageSetSmokeTest). This gets executed by
> run_tests in /tools/perf. I'm not sure if these tests are run as part of the
> commit try-bot tests, but they are limited and don't actually run the
> performance test.

I may be wrong, but it seems like the problem here isn't the measurement itself
but instead the new pages that we're adding. Maybe we could try adding those
pages to PageSetSmokeTest -- at least temporarily -- and see how they fare on
the try bots?

[picksi, 2014-07-22 14:23:34]

The additions of the 4 new pages to tough_scheduling_cases automatically adds
them to PageSetSmokeTest, so they would have been present for the commit
try-bots on my commit; or are you thinking of a non-commit related try-bot?

Having said all that, when I run the PageSetSmokeTest locally it only *Warns*
about missing pages (there are about 20 missing, ironically the 4 I added are
OK) e.g:

WARNING:root:Skipping
/usr/local/google/code/clankium/src/tools/perf/page_sets/tough_pepper_cases.py:
no archive data file

There is a comment at the regarding these warnings saying:
# TODO: Eventually these should be fatal.

Shall I record and commit all the missing archived pages and then make this
check fatal, as a first step? Or might there be something odd about my local
configuration that is making these pages appear to be missing?

[picksi, 2014-07-22 17:04:35]

These pages are also missing on the bots, scroll to the bottom of the following
log to see the warnings:

http://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/14...

This log is from a revision of the code which had my new pages in, and it isn't
warning about their absence:

http://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/14...

... All of which leads me to believe that the issue has nothing to do with
missing pages (which would be an easy fix!).

[jdduke (slow), 2014-08-21 19:26:51]

On 2014/07/22 17:04:35, picksi wrote:
> These pages are also missing on the bots, scroll to the bottom of the
following
> log to see the warnings:
> 
>
http://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/14...
> 
> This log is from a revision of the code which had my new pages in, and it
isn't
> warning about their absence:
> 
>
http://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/14...
> 
> ... All of which leads me to believe that the issue has nothing to do with
> missing pages (which would be an easy fix!).

What needs to happen for us to reland these pages?

[picksi1, 2014-08-22 12:36:02]

Sami reminded me about these missing pages yesterday. I'm Sheriffing or on
holiday until next Friday, so I probably won't get a chance to look at this
until then. I don't know why this is failing on the bots; The try-bots were in a
bad state when I was last trying to run these remotely and I couldn't get them
to run at all; hopefully things are a bit better now & I'll be able to run these
remotely, which will make it possible to repro & fix the issue (it's one of
these works-on-my-machine issues). It might give us some insight if someone else
was to get this patch on their machine and see if it works somewhere other than
my PC?

[jdduke (slow), 2014-08-26 15:46:20]

I tested each page set locally, and they seemed to run just fine.  The usatoday,
espn and latimes pages are *fantastic* examples of poor initial scrolling
response (>300ms).  The io9 case isn't quite as compelling.

Given the popularity of these pages, I wonder if these pages would be
appropriate in the key_mobile_sites bin?

https://codereview.chromium.org/372243003/diff/100001/tools/perf/page_sets/da...
File tools/perf/page_sets/data/tough_scheduling_cases.json (right):

https://codereview.chromium.org/372243003/diff/100001/tools/perf/page_sets/da...
tools/perf/page_sets/data/tough_scheduling_cases.json:6:
"http://io9.com/the-10-most-shocking-characters-that-george-r-r-martin-1579481155/+tinaamini",
We might get rid of the io9 case; it doesn't look very interesting in practice
any longer.

[picksi1, 2014-09-01 14:56:33]

I've added theses pages (except io9) to key_mobiles_sites under a different cl
(here: https://codereview.chromium.org/525493003/).