|
|
DescriptionFour 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
Patch Set 1 #Patch Set 2 : Long line fixed #Patch Set 3 : Testing new pages in tough_scheduleing_cases on try-bot #Patch Set 4 : Update to run-perf-test.cfg #Patch Set 5 : Removal of incorrect WPR file reference #
Total comments: 2
Patch Set 6 : White space fixup #
Total comments: 1
Messages
Total messages: 32 (0 generated)
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.
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!
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?
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.
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.
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.
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...
<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.
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!
I think we can land this and figure out the metric we want separately. Thus: lgtm.
The CQ bit was checked by picksi@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/picksi@chromium.org/372243003/1
The CQ bit was checked by picksi@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/picksi@chromium.org/372243003/20001
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...)
Message was sent while issue was closed.
Change committed as 282740
Message was sent while issue was closed.
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/....
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.
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.
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.
The CQ bit was checked by picksi@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/picksi@chromium.org/372243003/100001
Message was sent while issue was closed.
Change committed as 284060
Message was sent while issue was closed.
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?). .
Message was sent while issue was closed.
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!
Message was sent while issue was closed.
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?
Message was sent while issue was closed.
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?
Message was sent while issue was closed.
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!).
Message was sent while issue was closed.
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?
Message was sent while issue was closed.
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?
Message was sent while issue was closed.
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.
Message was sent while issue was closed.
I've added theses pages (except io9) to key_mobiles_sites under a different cl (here: https://codereview.chromium.org/525493003/). |