Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
871913004:
New page set added to include pathologically bad and janky sites. (Closed)
DescriptionNew page set added to include pathologically bad and janky sites.
BUG=447508
Committed: https://crrev.com/276f72cca888834c3f680711deb9b7fc03cec301
Cr-Commit-Position: refs/heads/master@{#315751}
Committed: https://crrev.com/b8b4244017c1c69aac2e5877a20265ad16e6a208
Cr-Commit-Position: refs/heads/master@{#315973}
Patch Set 1 #
Total comments: 4
Patch Set 2 : ESPN moved out of tough_scheduling_cases #
Total comments: 9
Patch Set 3 : Forbes removed due to cookie based pain #Patch Set 4 : Added page set to smoothness #Patch Set 5 : re-uploading json and sha1 to match uploaded wpr file #Patch Set 6 : new WPR uploaded #
Messages
Total messages: 28 (7 generated)
picksi@chromium.org changed reviewers: + alexclarke@chromium.org, jdduke@chromium.org, rmcilroy@chromium.org, skyostil@chromium.org
Here is the page set of janky sites as we discussed (some time ago now!), and the related WPR json files. I've made this usable by the task_execution_time metric so that we can track these pages on the perf dashboards to see if they have any slow tasks in common. gpylint asked me to make some changes to the task_execution_time file, I've added comments to explain these. PTAL! https://codereview.chromium.org/871913004/diff/1/tools/perf/benchmarks/task_e... File tools/perf/benchmarks/task_execution_time.py (right): https://codereview.chromium.org/871913004/diff/1/tools/perf/benchmarks/task_e... tools/perf/benchmarks/task_execution_time.py:4: gpylint suggested these imports should be re-ordered: https://codereview.chromium.org/871913004/diff/1/tools/perf/benchmarks/task_e... tools/perf/benchmarks/task_execution_time.py:12: gpylint wants blanks lines after the class definition too. https://codereview.chromium.org/871913004/diff/1/tools/perf/page_sets/patholo... File tools/perf/page_sets/pathological_mobile_sites.py (right): https://codereview.chromium.org/871913004/diff/1/tools/perf/page_sets/patholo... tools/perf/page_sets/pathological_mobile_sites.py:37: 'http://www.latimes.com', gpylint didn't allow me to add a '+' between the two halves of the next string:
On 2015/01/23 14:43:46, picksi1 wrote: > Here is the page set of janky sites as we discussed (some time ago now!), and > the related WPR json files. > > I've made this usable by the task_execution_time metric so that we can track > these pages on the perf dashboards to see if they have any slow tasks in common. > > gpylint asked me to make some changes to the task_execution_time file, I've > added comments to explain these. > > PTAL! > > https://codereview.chromium.org/871913004/diff/1/tools/perf/benchmarks/task_e... > File tools/perf/benchmarks/task_execution_time.py (right): > > https://codereview.chromium.org/871913004/diff/1/tools/perf/benchmarks/task_e... > tools/perf/benchmarks/task_execution_time.py:4: > gpylint suggested these imports should be re-ordered: > > https://codereview.chromium.org/871913004/diff/1/tools/perf/benchmarks/task_e... > tools/perf/benchmarks/task_execution_time.py:12: > gpylint wants blanks lines after the class definition too. > > https://codereview.chromium.org/871913004/diff/1/tools/perf/page_sets/patholo... > File tools/perf/page_sets/pathological_mobile_sites.py (right): > > https://codereview.chromium.org/871913004/diff/1/tools/perf/page_sets/patholo... > tools/perf/page_sets/pathological_mobile_sites.py:37: 'http://www.latimes.com', > gpylint didn't allow me to add a '+' between the two halves of the next string: Thanks Simon. Just a note before we land, all but one of the Android perf bots are completely hosed, and have been for nearly a week: http://build.chromium.org/p/chromium.perf/waterfall?show_events=true&failures.... We should probably defer landing this change until they're back online, or at least consistently succeeding :(.
https://codereview.chromium.org/871913004/diff/1/tools/perf/page_sets/data/pa... File tools/perf/page_sets/data/pathological_mobile_sites.json (right): https://codereview.chromium.org/871913004/diff/1/tools/perf/page_sets/data/pa... tools/perf/page_sets/data/pathological_mobile_sites.json:5: "http://m.espn.go.com/nhl/rankings", Should we remove the nhl rankings from tough_scheduling_cases.json? It looks a little out of place there.
I've moved the ESPN page out of tough_scheduling_cases.py; Jared you mention moving it out of the .json file - is this a typo or did you really want the json tidied up? Removing ESPN, I noticed that gpylint was unhappy with tough_scheduling_cases.py so I did some white-space massaging.
On 2015/01/26 16:30:21, picksi1 wrote: > I've moved the ESPN page out of tough_scheduling_cases.py; Jared you mention > moving it out of the .json file - is this a typo or did you really want the json > tidied up? > > Removing ESPN, I noticed that gpylint was unhappy with tough_scheduling_cases.py > so I did some white-space massaging. Oh, whatever the standard behavior is here when shuffling around sites is fine I think.
Looks great. Couple of suggestions below. https://codereview.chromium.org/871913004/diff/20001/tools/perf/page_sets/pat... File tools/perf/page_sets/pathological_mobile_sites.py (right): https://codereview.chromium.org/871913004/diff/20001/tools/perf/page_sets/pat... tools/perf/page_sets/pathological_mobile_sites.py:39: 'much-really-cost-live-city-like-seattle/#the-rundown', Please indent this line or use braces to make it more obvious that it's a continuation of the previous one. https://codereview.chromium.org/871913004/diff/20001/tools/perf/page_sets/tou... File tools/perf/page_sets/tough_scheduling_cases.py (right): https://codereview.chromium.org/871913004/diff/20001/tools/perf/page_sets/tou... tools/perf/page_sets/tough_scheduling_cases.py:479: 'http://www.latimes.com', Could you move latimes to the new page set too? It's pretty pathological, and that way tough_scheduling_cases will only consist of synthetic cases which should make it less noisy.
+vogelheim on cc since he was interested in looking at these janky pages.
https://codereview.chromium.org/871913004/diff/20001/tools/perf/page_sets/pat... File tools/perf/page_sets/pathological_mobile_sites.py (right): https://codereview.chromium.org/871913004/diff/20001/tools/perf/page_sets/pat... tools/perf/page_sets/pathological_mobile_sites.py:23: class PathologicalMobileSitesPageSet(page_set_module.PageSet): Do we want to add this as a smoothness test?
https://codereview.chromium.org/871913004/diff/20001/tools/perf/page_sets/dat... File tools/perf/page_sets/data/pathological_mobile_sites_000.wpr.sha1 (right): https://codereview.chromium.org/871913004/diff/20001/tools/perf/page_sets/dat... tools/perf/page_sets/data/pathological_mobile_sites_000.wpr.sha1:1: 9b72eb199764027f10708a9f9dfa7d54b27d299b Is there an accompanying .wpr file we need here? Sorry, not sure how this works, just ran into this when trying to test locally.
https://codereview.chromium.org/871913004/diff/20001/tools/perf/page_sets/dat... File tools/perf/page_sets/data/pathological_mobile_sites_000.wpr.sha1 (right): https://codereview.chromium.org/871913004/diff/20001/tools/perf/page_sets/dat... tools/perf/page_sets/data/pathological_mobile_sites_000.wpr.sha1:1: 9b72eb199764027f10708a9f9dfa7d54b27d299b On 2015/01/28 17:50:22, jdduke wrote: > Is there an accompanying .wpr file we need here? Sorry, not sure how this works, > just ran into this when trying to test locally. Hmm, nevermind, looks like this works differently...
picksi@google.com changed reviewers: + picksi@google.com
... Several weeks later. I've removed Forbes from the list. The page was checking a cookie and redirecting to a welcome (i.e. advert) page. When the tests are running cookies seem to vanish (even tapping on the link manually simply re-loads the welcome page). I messed around with various solutions but have timed out on this and want to get it up and running so we can collect data. I'll look into adding Forbes again as a separate CL. @Jared, please see my question about smoothness. https://codereview.chromium.org/871913004/diff/20001/tools/perf/page_sets/pat... File tools/perf/page_sets/pathological_mobile_sites.py (right): https://codereview.chromium.org/871913004/diff/20001/tools/perf/page_sets/pat... tools/perf/page_sets/pathological_mobile_sites.py:23: class PathologicalMobileSitesPageSet(page_set_module.PageSet): Not sure what you want here. Do I need to add something somewhere else to make this a "smoothness test", or are you suggesting I change is_smooth to False? Happy to do whatever you are after! https://codereview.chromium.org/871913004/diff/20001/tools/perf/page_sets/pat... tools/perf/page_sets/pathological_mobile_sites.py:39: 'much-really-cost-live-city-like-seattle/#the-rundown', On 2015/01/27 15:30:37, Sami wrote: > Please indent this line or use braces to make it more obvious that it's a > continuation of the previous one. Done. https://codereview.chromium.org/871913004/diff/20001/tools/perf/page_sets/tou... File tools/perf/page_sets/tough_scheduling_cases.py (right): https://codereview.chromium.org/871913004/diff/20001/tools/perf/page_sets/tou... tools/perf/page_sets/tough_scheduling_cases.py:479: 'http://www.latimes.com', On 2015/01/27 15:30:37, Sami wrote: > Could you move latimes to the new page set too? It's pretty pathological, and > that way tough_scheduling_cases will only consist of synthetic cases which > should make it less noisy. Done.
https://codereview.chromium.org/871913004/diff/20001/tools/perf/page_sets/pat... File tools/perf/page_sets/pathological_mobile_sites.py (right): https://codereview.chromium.org/871913004/diff/20001/tools/perf/page_sets/pat... tools/perf/page_sets/pathological_mobile_sites.py:23: class PathologicalMobileSitesPageSet(page_set_module.PageSet): On 2015/02/10 18:10:46, picksi wrote: > Not sure what you want here. Do I need to add something somewhere else to make > this a "smoothness test", or are you suggesting I change is_smooth to False? > Happy to do whatever you are after! Sorry, I meant add it to the smoothness suite of tests in "smoothness.py", much as you added it to the "task_execution_time.py" suite. We're as concerned about latency and jank here as we are about the task execution times, right?
Thanks Simon, lgtm!
New patchsets have been uploaded after l-g-t-m from skyostil@chromium.org
The CQ bit was checked by picksi@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/871913004/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/276f72cca888834c3f680711deb9b7fc03cec301 Cr-Commit-Position: refs/heads/master@{#315751}
The CQ bit was checked by picksi@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/871913004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...) ios_rel_device_ninja_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by picksi@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/871913004/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/b8b4244017c1c69aac2e5877a20265ad16e6a208 Cr-Commit-Position: refs/heads/master@{#315973} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
