Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2168743004:
[system health] Add media browsing stories. (Closed)
|
Created:
4 years, 5 months ago by Hannes Payer (out of office) Modified:
4 years, 4 months ago Reviewers:
Primiano Tucci (use gerrit), nednguyen, petrcermak, ulan, charliea (OOO until 10-5), sullivan CC:
chromium-reviews, telemetry-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[system health] Add media browsing stories.
The workloads emulate real user behavior and is mainly used to benchmark memory consumption.
Each media browsing story performs the following workload:
1. Load a page showing a media item
2. Click on the next link to go to the next media item
3. etc.
The following websites were selected:
- Facebook
- YouTube
- Imgur
BUG=chromium:589726
Committed: https://crrev.com/2b3bbf6088bf4b53cb1cd3ad63e038c2a699113f
Cr-Commit-Position: refs/heads/master@{#410622}
Patch Set 1 #
Total comments: 16
Patch Set 2 : comments addressed #
Total comments: 10
Patch Set 3 : comments #Patch Set 4 : comment #
Total comments: 13
Patch Set 5 : comments, getting close to shipping #
Total comments: 24
Patch Set 6 : more comments #
Total comments: 11
Patch Set 7 : more comments #Patch Set 8 : remove login facebook #Patch Set 9 : keep facebook separate #Patch Set 10 : merge #Patch Set 11 : disable facebook on desktop #Messages
Total messages: 49 (16 generated)
Description was changed from ========== [system health] Added media browsing story BUG= ========== to ========== [system health] Add media browsing stories. The workloads emulate real user behavior and is mainly used to benchmark memory consumption. Each media browsing story performs the following workload: 1. Load a page showing a media item 2. Click on the next link to go to the next media item 3. etc. The following websites were selected: - Facebook - Youtube - Imgur - Flickr BUG= ==========
Hi Hannes, For the context, Charlie is leading the effort of defining the set of media stories for power benchmarking. I think you should sync up with him of the list of media stories we want to cover. I think list we currently have here is overly focused on photo cases, and I am not sure whether they are the main media case that can cause perf problems. - Facebook (photos) - Youtube (video) - Imgur (photos) - Flickr (photos) https://codereview.chromium.org/2168743004/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2168743004/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/browsing_stories.py:298: mobile_facebook_login.LoginAccount(action_runner, 'facebook3', Can you not use mobile_facebook_login here but: 1) Rename mobile_facebook_login to facebook_login 2) Change mobile_facebook_login.LoginAccount to facebook_login.LoginWithMobileSite & facebook_login.LoginWithDesktopSite?
On 2016/07/21 17:32:00, nednguyen wrote: > Hi Hannes, > For the context, Charlie is leading the effort of defining the set of media > stories for power benchmarking. I think you should sync up with him of the list > of media stories we want to cover. I think list we currently have here is overly > focused on photo cases, and I am not sure whether they are the main media case > that can cause perf problems. > > - Facebook (photos) > - Youtube (video) > - Imgur (photos) > - Flickr (photos) > > https://codereview.chromium.org/2168743004/diff/1/tools/perf/page_sets/system... > File tools/perf/page_sets/system_health/browsing_stories.py (right): > > https://codereview.chromium.org/2168743004/diff/1/tools/perf/page_sets/system... > tools/perf/page_sets/system_health/browsing_stories.py:298: > mobile_facebook_login.LoginAccount(action_runner, 'facebook3', > Can you not use mobile_facebook_login here but: > 1) Rename mobile_facebook_login to facebook_login > 2) Change mobile_facebook_login.LoginAccount to > facebook_login.LoginWithMobileSite & facebook_login.LoginWithDesktopSite? Hi Ned, thanks for the suggestion. Can you add Charlie as a reviewer? Explanation why I added these benchmarks: Facebook: Alexa top 3 Youtube: Alexa top 2 Imgur: Alexa top 43 and memory heavy for v8 Flickr: We can remove this one if we want to have a smaller set of benchmarks. I am open for suggestions. And we can always add new stories incrementally.
nednguyen@google.com changed reviewers: + charliea@chromium.org
On 2016/07/21 19:04:59, Hannes Payer wrote: > On 2016/07/21 17:32:00, nednguyen wrote: > > Hi Hannes, > > For the context, Charlie is leading the effort of defining the set of media > > stories for power benchmarking. I think you should sync up with him of the > list > > of media stories we want to cover. I think list we currently have here is > overly > > focused on photo cases, and I am not sure whether they are the main media case > > that can cause perf problems. > > > > - Facebook (photos) > > - Youtube (video) > > - Imgur (photos) > > - Flickr (photos) > > > > > https://codereview.chromium.org/2168743004/diff/1/tools/perf/page_sets/system... > > File tools/perf/page_sets/system_health/browsing_stories.py (right): > > > > > https://codereview.chromium.org/2168743004/diff/1/tools/perf/page_sets/system... > > tools/perf/page_sets/system_health/browsing_stories.py:298: > > mobile_facebook_login.LoginAccount(action_runner, 'facebook3', > > Can you not use mobile_facebook_login here but: > > 1) Rename mobile_facebook_login to facebook_login > > 2) Change mobile_facebook_login.LoginAccount to > > facebook_login.LoginWithMobileSite & facebook_login.LoginWithDesktopSite? > > Hi Ned, > thanks for the suggestion. Can you add Charlie as a reviewer? > Explanation why I added these benchmarks: > Facebook: Alexa top 3 > Youtube: Alexa top 2 > Imgur: Alexa top 43 and memory heavy for v8 > Flickr: We can remove this one if we want to have a smaller set of benchmarks. > I am open for suggestions. And we can always add new stories incrementally. I think just picking top x pages from Alexa is not a good strategy in general. The reason is that we also want system health stories to also cover as many of different Chrome components & have a wide range of performance characteristics. For example, assuming we just pick top 10 pages from Alexa, and they are all lightweight pages with similar web behaviors. Then say any regression on any page also creates a similar regression on other 9 pages, we should just save our bot cycle time by keep only 1 page and drop the other 9 :-) So in term of media browsing, I would expect we have cover over: video game music play webrtc... We don't have to go about implementing all of these in 1 patch but at least we should agree on which are the media set we will want to cover.
Looks good overall. Here's just a couple of code style comments. I agree that you should check this with Charlie. Thanks, Petr https://codereview.chromium.org/2168743004/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2168743004/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/browsing_stories.py:42: class _NewsBrowsingStory(_BrowsingStory): I suggest you move the _NewsBrowsingStory and _MediaBrowsingStory subclasses to the relevant sections of this file (under the "######..." comments). https://codereview.chromium.org/2168743004/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/browsing_stories.py:76: nit: add extra blank line (I added justification in a comment below) https://codereview.chromium.org/2168743004/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/browsing_stories.py:92: self._NavigateToItem(action_runner, self._ItemSelector(i)) Why do you define the _ItemSelector method when you always ignore the argument? I would suggest defining an ITEM_SELECTOR_INDEX class constant instead. Even better, you use non-zero indices in only two places. Is there any chance you could somehow push the index into the selector (so that you could always pass `0` here)? If it would make the selectors too complicated, I suggest you set ITEM_SELECTOR_INDEX = 0 as default and override it only when necessary. https://codereview.chromium.org/2168743004/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/browsing_stories.py:203: nit: There should be two spaces between top-level definitions (one extra blank line here and one after the comment). That means there should also be an extra blank line between each two class definitions below. https://codereview.chromium.org/2168743004/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/browsing_stories.py:210: URL = ("https://www.flickr.com/photos/albertdros/27815579924/in/" s/"/'/g everywhere
On 2016/07/21 19:34:50, nednguyen wrote: > On 2016/07/21 19:04:59, Hannes Payer wrote: > > On 2016/07/21 17:32:00, nednguyen wrote: > > > Hi Hannes, > > > For the context, Charlie is leading the effort of defining the set of media > > > stories for power benchmarking. I think you should sync up with him of the > > list > > > of media stories we want to cover. I think list we currently have here is > > overly > > > focused on photo cases, and I am not sure whether they are the main media > case > > > that can cause perf problems. > > > > > > - Facebook (photos) > > > - Youtube (video) > > > - Imgur (photos) > > > - Flickr (photos) > > > > > > > > > https://codereview.chromium.org/2168743004/diff/1/tools/perf/page_sets/system... > > > File tools/perf/page_sets/system_health/browsing_stories.py (right): > > > > > > > > > https://codereview.chromium.org/2168743004/diff/1/tools/perf/page_sets/system... > > > tools/perf/page_sets/system_health/browsing_stories.py:298: > > > mobile_facebook_login.LoginAccount(action_runner, 'facebook3', > > > Can you not use mobile_facebook_login here but: > > > 1) Rename mobile_facebook_login to facebook_login > > > 2) Change mobile_facebook_login.LoginAccount to > > > facebook_login.LoginWithMobileSite & facebook_login.LoginWithDesktopSite? > > > > Hi Ned, > > thanks for the suggestion. Can you add Charlie as a reviewer? > > Explanation why I added these benchmarks: > > Facebook: Alexa top 3 > > Youtube: Alexa top 2 > > Imgur: Alexa top 43 and memory heavy for v8 > > Flickr: We can remove this one if we want to have a smaller set of benchmarks. > > I am open for suggestions. And we can always add new stories incrementally. > > I think just picking top x pages from Alexa is not a good strategy in general. > The reason is that we also want system health stories to also cover as many of > different Chrome components & have a wide range of performance characteristics. > > For example, assuming we just pick top 10 pages from Alexa, and they are all > lightweight pages with similar web behaviors. Then say any regression on any > page also creates a similar regression on other 9 pages, we should just save our > bot cycle time by keep only 1 page and drop the other 9 :-) > > So in term of media browsing, I would expect we have cover over: > video > game > music play > webrtc... > > We don't have to go about implementing all of these in 1 patch but at least we > should agree on which are the media set we will want to cover. Absolutely, that's what we did on the V8 side and this is why we picked these pages. Alexa was just the start seed. Flickr and Imgur are somewhat similar. I am happy to remove Flickr. Youtube and facebook are quiet different. Charlie, WDYT about the selected pages?
On 2016/07/22 at 07:14:22, hpayer wrote: > On 2016/07/21 19:34:50, nednguyen wrote: > > On 2016/07/21 19:04:59, Hannes Payer wrote: > > > On 2016/07/21 17:32:00, nednguyen wrote: > > > > Hi Hannes, > > > > For the context, Charlie is leading the effort of defining the set of media > > > > stories for power benchmarking. I think you should sync up with him of the > > > list > > > > of media stories we want to cover. I think list we currently have here is > > > overly > > > > focused on photo cases, and I am not sure whether they are the main media > > case > > > > that can cause perf problems. > > > > > > > > - Facebook (photos) > > > > - Youtube (video) > > > > - Imgur (photos) > > > > - Flickr (photos) > > > > > > > > > > > > > https://codereview.chromium.org/2168743004/diff/1/tools/perf/page_sets/system... > > > > File tools/perf/page_sets/system_health/browsing_stories.py (right): > > > > > > > > > > > > > https://codereview.chromium.org/2168743004/diff/1/tools/perf/page_sets/system... > > > > tools/perf/page_sets/system_health/browsing_stories.py:298: > > > > mobile_facebook_login.LoginAccount(action_runner, 'facebook3', > > > > Can you not use mobile_facebook_login here but: > > > > 1) Rename mobile_facebook_login to facebook_login > > > > 2) Change mobile_facebook_login.LoginAccount to > > > > facebook_login.LoginWithMobileSite & facebook_login.LoginWithDesktopSite? > > > > > > Hi Ned, > > > thanks for the suggestion. Can you add Charlie as a reviewer? > > > Explanation why I added these benchmarks: > > > Facebook: Alexa top 3 > > > Youtube: Alexa top 2 > > > Imgur: Alexa top 43 and memory heavy for v8 > > > Flickr: We can remove this one if we want to have a smaller set of benchmarks. > > > I am open for suggestions. And we can always add new stories incrementally. > > > > I think just picking top x pages from Alexa is not a good strategy in general. > > The reason is that we also want system health stories to also cover as many of > > different Chrome components & have a wide range of performance characteristics. > > > > For example, assuming we just pick top 10 pages from Alexa, and they are all > > lightweight pages with similar web behaviors. Then say any regression on any > > page also creates a similar regression on other 9 pages, we should just save our > > bot cycle time by keep only 1 page and drop the other 9 :-) > > > > So in term of media browsing, I would expect we have cover over: > > video > > game > > music play > > webrtc... > > > > We don't have to go about implementing all of these in 1 patch but at least we > > should agree on which are the media set we will want to cover. > > Absolutely, that's what we did on the V8 side and this is why we picked these > pages. Alexa was just the start seed. Flickr and Imgur are somewhat similar. I am happy > to remove Flickr. Youtube and facebook are quiet different. > > Charlie, WDYT about the selected pages? I think the suggestion of ditching one of Flickr and Imgur is a good one. The rest seem good to me. I think that, in the future, we'll want a few other types of video in here (e.g. from an encrypted source like Netflix in addition to just YouTube), but this is a reasonable start to the set.
On 2016/07/22 at 07:14:22, hpayer wrote: > On 2016/07/21 19:34:50, nednguyen wrote: > > On 2016/07/21 19:04:59, Hannes Payer wrote: > > > On 2016/07/21 17:32:00, nednguyen wrote: > > > > Hi Hannes, > > > > For the context, Charlie is leading the effort of defining the set of media > > > > stories for power benchmarking. I think you should sync up with him of the > > > list > > > > of media stories we want to cover. I think list we currently have here is > > > overly > > > > focused on photo cases, and I am not sure whether they are the main media > > case > > > > that can cause perf problems. > > > > > > > > - Facebook (photos) > > > > - Youtube (video) > > > > - Imgur (photos) > > > > - Flickr (photos) > > > > > > > > > > > > > https://codereview.chromium.org/2168743004/diff/1/tools/perf/page_sets/system... > > > > File tools/perf/page_sets/system_health/browsing_stories.py (right): > > > > > > > > > > > > > https://codereview.chromium.org/2168743004/diff/1/tools/perf/page_sets/system... > > > > tools/perf/page_sets/system_health/browsing_stories.py:298: > > > > mobile_facebook_login.LoginAccount(action_runner, 'facebook3', > > > > Can you not use mobile_facebook_login here but: > > > > 1) Rename mobile_facebook_login to facebook_login > > > > 2) Change mobile_facebook_login.LoginAccount to > > > > facebook_login.LoginWithMobileSite & facebook_login.LoginWithDesktopSite? > > > > > > Hi Ned, > > > thanks for the suggestion. Can you add Charlie as a reviewer? > > > Explanation why I added these benchmarks: > > > Facebook: Alexa top 3 > > > Youtube: Alexa top 2 > > > Imgur: Alexa top 43 and memory heavy for v8 > > > Flickr: We can remove this one if we want to have a smaller set of benchmarks. > > > I am open for suggestions. And we can always add new stories incrementally. > > > > I think just picking top x pages from Alexa is not a good strategy in general. > > The reason is that we also want system health stories to also cover as many of > > different Chrome components & have a wide range of performance characteristics. > > > > For example, assuming we just pick top 10 pages from Alexa, and they are all > > lightweight pages with similar web behaviors. Then say any regression on any > > page also creates a similar regression on other 9 pages, we should just save our > > bot cycle time by keep only 1 page and drop the other 9 :-) > > > > So in term of media browsing, I would expect we have cover over: > > video > > game > > music play > > webrtc... > > > > We don't have to go about implementing all of these in 1 patch but at least we > > should agree on which are the media set we will want to cover. > > Absolutely, that's what we did on the V8 side and this is why we picked these > pages. Alexa was just the start seed. Flickr and Imgur are somewhat similar. I am happy > to remove Flickr. Youtube and facebook are quiet different. > > Charlie, WDYT about the selected pages? I think the suggestion of ditching one of Flickr and Imgur is a good one. The rest seem good to me. I think that, in the future, we'll want a few other types of video in here (e.g. from an encrypted source like Netflix in addition to just YouTube), but this is a reasonable start to the set.
After syncing with Charlie I removed Flickr. PTAL https://codereview.chromium.org/2168743004/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2168743004/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/browsing_stories.py:42: class _NewsBrowsingStory(_BrowsingStory): On 2016/07/21 19:38:42, petrcermak (OOO until 31 Jul) wrote: > I suggest you move the _NewsBrowsingStory and _MediaBrowsingStory subclasses to > the relevant sections of this file (under the "######..." comments). Done. https://codereview.chromium.org/2168743004/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/browsing_stories.py:76: On 2016/07/21 19:38:42, petrcermak (OOO until 31 Jul) wrote: > nit: add extra blank line (I added justification in a comment below) Done. https://codereview.chromium.org/2168743004/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/browsing_stories.py:92: self._NavigateToItem(action_runner, self._ItemSelector(i)) On 2016/07/21 19:38:42, petrcermak (OOO until 31 Jul) wrote: > Why do you define the _ItemSelector method when you always ignore the argument? > I would suggest defining an ITEM_SELECTOR_INDEX class constant instead. > > Even better, you use non-zero indices in only two places. Is there any chance > you could somehow push the index into the selector (so that you could always > pass `0` here)? If it would make the selectors too complicated, I suggest you > set ITEM_SELECTOR_INDEX = 0 as default and override it only when necessary. Done. That is was leftover code experimenting with different selector strategies. https://codereview.chromium.org/2168743004/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/browsing_stories.py:203: On 2016/07/21 19:38:42, petrcermak (OOO until 31 Jul) wrote: > nit: There should be two spaces between top-level definitions (one extra blank > line here and one after the comment). That means there should also be an extra > blank line between each two class definitions below. Done. https://codereview.chromium.org/2168743004/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/browsing_stories.py:210: URL = ("https://www.flickr.com/photos/albertdros/27815579924/in/" On 2016/07/21 19:38:42, petrcermak (OOO until 31 Jul) wrote: > s/"/'/g everywhere Done. https://codereview.chromium.org/2168743004/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/browsing_stories.py:298: mobile_facebook_login.LoginAccount(action_runner, 'facebook3', On 2016/07/21 17:32:00, nednguyen wrote: > Can you not use mobile_facebook_login here but: > 1) Rename mobile_facebook_login to facebook_login > 2) Change mobile_facebook_login.LoginAccount to > facebook_login.LoginWithMobileSite & facebook_login.LoginWithDesktopSite? I experimented with that. The Desktop version gets stuck most of the time because facebook login wants cookies enabled. I do not run into this login problem using the mobile site, which after login transitions to non-mobile facebook. Can I use the mobile login procedure? WDYT?
Description was changed from ========== [system health] Add media browsing stories. The workloads emulate real user behavior and is mainly used to benchmark memory consumption. Each media browsing story performs the following workload: 1. Load a page showing a media item 2. Click on the next link to go to the next media item 3. etc. The following websites were selected: - Facebook - Youtube - Imgur - Flickr BUG= ========== to ========== [system health] Add media browsing stories. The workloads emulate real user behavior and is mainly used to benchmark memory consumption. Each media browsing story performs the following workload: 1. Load a page showing a media item 2. Click on the next link to go to the next media item 3. etc. The following websites were selected: - Facebook - Youtube - Imgur BUG= ==========
Since both imgur & facebook are both about viewing photos, should we consider remove one? Or if you switch facebook to the multi-videos scrolling cases, I think that would be great. https://codereview.chromium.org/2168743004/diff/20001/tools/perf/page_sets/lo... File tools/perf/page_sets/login_helpers/facebook_login.py (right): https://codereview.chromium.org/2168743004/diff/20001/tools/perf/page_sets/lo... tools/perf/page_sets/login_helpers/facebook_login.py:41: """Logs in into mobile Facebook account. s/mobile/desktop https://codereview.chromium.org/2168743004/diff/20001/tools/perf/page_sets/lo... tools/perf/page_sets/login_helpers/facebook_login.py:60: 'https://m.facebook.com/login?continue=https%3A%2F%2Fm.facebook.com') nits: change this url to the desktop one https://codereview.chromium.org/2168743004/diff/20001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2168743004/diff/20001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:286: URL = ('https://facebook.com/photo.php?fbid=10154398154450513&' nits: you can do URL = ( 'https://facebook.com/photo.php?fbid=10154398154450513&id=255110695512&set=a.406278500512.172778.255110695512&source=54&refid=13') https://codereview.chromium.org/2168743004/diff/20001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:300: '10154267233981772/?type=3&theater') Same here
I would prefer keeping the facebook photo browsing story. It shows interesting memory behavior in v8. https://codereview.chromium.org/2168743004/diff/20001/tools/perf/page_sets/lo... File tools/perf/page_sets/login_helpers/facebook_login.py (right): https://codereview.chromium.org/2168743004/diff/20001/tools/perf/page_sets/lo... tools/perf/page_sets/login_helpers/facebook_login.py:41: """Logs in into mobile Facebook account. On 2016/07/26 18:33:43, nednguyen wrote: > s/mobile/desktop Done. https://codereview.chromium.org/2168743004/diff/20001/tools/perf/page_sets/lo... tools/perf/page_sets/login_helpers/facebook_login.py:60: 'https://m.facebook.com/login?continue=https%3A%2F%2Fm.facebook.com') On 2016/07/26 18:33:43, nednguyen wrote: > nits: change this url to the desktop one The desktop login page gets stuck most of the time because facebook login wants cookies enabled and rejects login. I do not run into this login problem using the mobile site, which after login transitions to non-mobile facebook. Can I use the mobile login procedure? WDYT? https://codereview.chromium.org/2168743004/diff/20001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2168743004/diff/20001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:286: URL = ('https://facebook.com/photo.php?fbid=10154398154450513&' On 2016/07/26 18:33:43, nednguyen wrote: > nits: you can do > > URL = ( > > 'https://facebook.com/photo.php?fbid=10154398154450513&id=255110695512&set=a.406278500512.172778.255110695512&source=54&refid=13') Done. https://codereview.chromium.org/2168743004/diff/20001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:300: '10154267233981772/?type=3&theater') On 2016/07/26 18:33:43, nednguyen wrote: > Same here Done.
lgtm but please wait for Charlie's approval https://codereview.chromium.org/2168743004/diff/20001/tools/perf/page_sets/lo... File tools/perf/page_sets/login_helpers/facebook_login.py (right): https://codereview.chromium.org/2168743004/diff/20001/tools/perf/page_sets/lo... tools/perf/page_sets/login_helpers/facebook_login.py:60: 'https://m.facebook.com/login?continue=https%3A%2F%2Fm.facebook.com') On 2016/07/27 09:04:33, Hannes Payer wrote: > On 2016/07/26 18:33:43, nednguyen wrote: > > nits: change this url to the desktop one > > The desktop login page gets stuck most of the time because facebook login wants > cookies enabled and rejects login. I do not run into this login problem using > the mobile site, which after login transitions to non-mobile facebook. Can I use > the mobile login procedure? WDYT? ok, but can you add some documentation explaining the reason here.
Charlie, looking good from your side?
https://codereview.chromium.org/2168743004/diff/20001/tools/perf/page_sets/lo... File tools/perf/page_sets/login_helpers/facebook_login.py (right): https://codereview.chromium.org/2168743004/diff/20001/tools/perf/page_sets/lo... tools/perf/page_sets/login_helpers/facebook_login.py:60: 'https://m.facebook.com/login?continue=https%3A%2F%2Fm.facebook.com') On 2016/07/27 13:58:43, nednguyen wrote: > On 2016/07/27 09:04:33, Hannes Payer wrote: > > On 2016/07/26 18:33:43, nednguyen wrote: > > > nits: change this url to the desktop one > > > > The desktop login page gets stuck most of the time because facebook login > wants > > cookies enabled and rejects login. I do not run into this login problem using > > the mobile site, which after login transitions to non-mobile facebook. Can I > use > > the mobile login procedure? WDYT? > > ok, but can you add some documentation explaining the reason here. Done.
https://codereview.chromium.org/2168743004/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2168743004/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/browsing_stories.py:248: class YoutubeMobileStory(_MediaBrowsingStory): I think it's worth noting that there are known issues with web page replay (which is used to play back these stories) and YouTube: I don't think that we've had any luck getting things to work on more than on platform yet. https://codereview.chromium.org/2168743004/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/browsing_stories.py:254: ITEM_VIEW_TIME_IN_SECONDS = 2 maybe petrcermak@chromium.org has an opinion here, but it might be nice to have a comment about why two seconds was chosen here. This is the sort of magical constant that can easily get lost to history if it's not documented. https://codereview.chromium.org/2168743004/diff/60001/tools/perf/page_sets/lo... File tools/perf/page_sets/login_helpers/facebook_login.py (right): https://codereview.chromium.org/2168743004/diff/60001/tools/perf/page_sets/lo... tools/perf/page_sets/login_helpers/facebook_login.py:59: # Currently we use the mobile login page also on Desktop because the s/"we use the mobile login page also on Desktop"/"we use the mobile login page (even on desktop)" https://codereview.chromium.org/2168743004/diff/60001/tools/perf/page_sets/lo... tools/perf/page_sets/login_helpers/facebook_login.py:59: # Currently we use the mobile login page also on Desktop because the s/Desktop/desktop https://codereview.chromium.org/2168743004/diff/60001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2168743004/diff/60001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:280: ITEM_VIEW_TIME_IN_SECONDS = 2 IMO this (and the corresponding mobile story time) should be longer: when I ran these locally (on a fiber connection), there was barely enough time for the video to load before we moved on to the next story. What about increasing the time to something like 10 seconds, but cutting the number of pages to something like 1/3 of what it currently is? That seems like it would more accurately reproduce how people use YouTube.
lgtm w/ some comments (in addition to the above ones) If the BUG= line is empty, it should be removed. However, it looks like this should be associated with https://bugs.chromium.org/p/chromium/issues/detail?id=627377 (BUG=627377) Some of these stories crashed on Chrome ToT for me: I had to use stable instead of ToT for them to work. I assume this isn't our problem, but I thought it was worth noting.
Looks good overall. Here are a few comments. Thanks, Petr https://codereview.chromium.org/2168743004/diff/60001/tools/perf/page_sets/lo... File tools/perf/page_sets/login_helpers/facebook_login.py (right): https://codereview.chromium.org/2168743004/diff/60001/tools/perf/page_sets/lo... tools/perf/page_sets/login_helpers/facebook_login.py:60: # Desktop version requires enabled cookies and rejects the login. In that case, why the code duplication? You can just call LoginWithMobileSite. def LoginWithDesktopSite( action_runner, credential, credentials_path=login_utils.DEFAULT_CREDENTIAL_PATH): # Currently we ... return LoginWithMobileSite(action_runner, credential, credentials_path) https://codereview.chromium.org/2168743004/diff/60001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2168743004/diff/60001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:20: ITEMS_TO_VISIT = 4 This field is currently not used anywhere in the base class. Either move it to _NewsBrowsingStory (_MediaBrowsingStory overrides it anyway), or add an abstract _ViewItem method: class _BrowsingStory: def _DidLoadDocument(self, action_runner): for i in xrange(self.ITEMS_TO_VISIT): self._ViewItem(action_runner, i) def _ViewItem(self, action_runner, index): raise NotImplementedError class _NewsBrowsingStory: def _ViewItem(self, action_runner, index): ... class _MediaBrowsingStory: def _ViewItem(self, action_runner, index): ... https://codereview.chromium.org/2168743004/diff/60001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:219: # Media browsing stories. Can you please update the corresponding loading stories (which you might need to add) to have the same URLs (+logins if applicable) as the ones you use here? https://codereview.chromium.org/2168743004/diff/60001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:224: """ Abstract base class for media user stories nit: remove space before "Abstract" https://codereview.chromium.org/2168743004/diff/60001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:308: self.credentials_path) nit: invalid indentation. The two basic options are: # Line break when needed + vertical alignment. facebook_login.LoginWithDesktopSite(action_runner, 'facebook3', self.credentials_path) # Immediate line break + 4 space indent. facebook_login.LoginWithDesktopSite( action_runner, 'facebook3', self.credentials_path)
https://codereview.chromium.org/2168743004/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2168743004/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/browsing_stories.py:248: class YoutubeMobileStory(_MediaBrowsingStory): On 2016/07/29 20:10:01, charliea wrote: > I think it's worth noting that there are known issues with web page replay > (which is used to play back these stories) and YouTube: I don't think that we've > had any luck getting things to work on more than on platform yet. Acknowledged. https://codereview.chromium.org/2168743004/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/browsing_stories.py:254: ITEM_VIEW_TIME_IN_SECONDS = 2 On 2016/07/29 20:10:01, charliea wrote: > maybe mailto:petrcermak@chromium.org has an opinion here, but it might be nice to have > a comment about why two seconds was chosen here. This is the sort of magical > constant that can easily get lost to history if it's not documented. I will change it for all to two seconds. https://codereview.chromium.org/2168743004/diff/60001/tools/perf/page_sets/lo... File tools/perf/page_sets/login_helpers/facebook_login.py (right): https://codereview.chromium.org/2168743004/diff/60001/tools/perf/page_sets/lo... tools/perf/page_sets/login_helpers/facebook_login.py:60: # Desktop version requires enabled cookies and rejects the login. On 2016/08/01 11:22:30, petrcermak wrote: > In that case, why the code duplication? You can just call LoginWithMobileSite. > > def LoginWithDesktopSite( > action_runner, credential, > credentials_path=login_utils.DEFAULT_CREDENTIAL_PATH): > # Currently we ... > return LoginWithMobileSite(action_runner, credential, credentials_path) Right on. Done. https://codereview.chromium.org/2168743004/diff/60001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2168743004/diff/60001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:20: ITEMS_TO_VISIT = 4 On 2016/08/01 11:22:30, petrcermak wrote: > This field is currently not used anywhere in the base class. Either move it to > _NewsBrowsingStory (_MediaBrowsingStory overrides it anyway), or add an abstract > _ViewItem method: > > class _BrowsingStory: > > def _DidLoadDocument(self, action_runner): > for i in xrange(self.ITEMS_TO_VISIT): > self._ViewItem(action_runner, i) > > def _ViewItem(self, action_runner, index): > raise NotImplementedError > > > class _NewsBrowsingStory: > def _ViewItem(self, action_runner, index): > ... > > > class _MediaBrowsingStory: > def _ViewItem(self, action_runner, index): > ... Moved it to the _NewsBrowsingStory. https://codereview.chromium.org/2168743004/diff/60001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:219: # Media browsing stories. On 2016/08/01 11:22:30, petrcermak wrote: > Can you please update the corresponding loading stories (which you might need to > add) to have the same URLs (+logins if applicable) as the ones you use here? Added imgur to the load stories. Changed the links in the browsing stories to be the same as in the load stories. Done. https://codereview.chromium.org/2168743004/diff/60001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:224: """ Abstract base class for media user stories On 2016/08/01 11:22:30, petrcermak wrote: > nit: remove space before "Abstract" Done. https://codereview.chromium.org/2168743004/diff/60001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:280: ITEM_VIEW_TIME_IN_SECONDS = 2 On 2016/07/29 20:10:01, charliea wrote: > IMO this (and the corresponding mobile story time) should be longer: when I ran > these locally (on a fiber connection), there was barely enough time for the > video to load before we moved on to the next story. What about increasing the > time to something like 10 seconds, but cutting the number of pages to something > like 1/3 of what it currently is? That seems like it would more accurately > reproduce how people use YouTube. Done.
Final comments. Two description comments: * s/Youtube/YouTube/ * add bug number please Thanks, Petr https://codereview.chromium.org/2168743004/diff/80001/tools/perf/page_sets/da... File tools/perf/page_sets/data/system_health_desktop.json (right): https://codereview.chromium.org/2168743004/diff/80001/tools/perf/page_sets/da... tools/perf/page_sets/data/system_health_desktop.json:90: "load:media:imgur" I suggest you move this (manually - I know that the description says you shouldn't do it) under system_health_desktop_025.wpr and delete system_health_desktop_024.wpr altogether. That way, both the browsing and loading will have exactly the same condition (better for comparison). https://codereview.chromium.org/2168743004/diff/80001/tools/perf/page_sets/da... File tools/perf/page_sets/data/system_health_mobile.json (right): https://codereview.chromium.org/2168743004/diff/80001/tools/perf/page_sets/da... tools/perf/page_sets/data/system_health_mobile.json:99: "load:media:imgur" ditto https://codereview.chromium.org/2168743004/diff/80001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2168743004/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:247: class ImgurMobileStory(_MediaBrowsingStory): Note that you could share common properties by defining abstract base classes. Example: class _ImgurStory(_MediaBrowsingStory): NAME = 'browse:media:imgur' URL = 'http://imgur.com/gallery/5UlBN' IS_SINGLE_APP = True ABSTRACT_STORY = True class ImgurDesktopStory(_ImgurStory): ITEM_SELECTOR = ... SUPPORTED_PLATFORMS = platforms.DESKTOP_ONLY class ImgurMobileStory(_ImgurStory): ... Feel free to ignore if you prefer it this way. https://codereview.chromium.org/2168743004/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:263: class YoutubeMobileStory(_MediaBrowsingStory): nit: can you please change this to YouTubeMobileStory (capitalization of "T") so that it's consistent with loading_stories.py? https://codereview.chromium.org/2168743004/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:267: IS_SINGLE_PAGE_APP = True nit: Can you please order the class fields consistently (here you have IS_SINGLE_PAGE_APP before SUPPORTED_PLATFORMS, but the ordering is reversed in YoutubeDesktopStory) https://codereview.chromium.org/2168743004/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:272: class YoutubeDesktopStory(_MediaBrowsingStory): ditto (capital "T") https://codereview.chromium.org/2168743004/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:278: ITEM_VIEW_TIME_IN_SECONDS = 5 Could you please add a short comment why you override the default arguments here (e.g. because it crashes otherwise). https://codereview.chromium.org/2168743004/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:284: NAME = 'browse:media:facebookphotos' nit: I think facebook_photos would be better (consistent with 'google_images' in loading_stories.py) https://codereview.chromium.org/2168743004/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:293: action_runner.Navigate('https://m.facebook.com/NASA') Why do you go to NASA and then back to Rihanna? If this is intentional, please add a comment with an explanation https://codereview.chromium.org/2168743004/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:295: nit: too many blank lines (should be 2) https://codereview.chromium.org/2168743004/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:299: NAME = 'browse:media:facebookphotos' ditto (facebook_photos) https://codereview.chromium.org/2168743004/diff/80001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/loading_stories.py (right): https://codereview.chromium.org/2168743004/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/loading_stories.py:244: nit: add blank line (there should be two blank lines between top-level definitions)
https://codereview.chromium.org/2168743004/diff/80001/tools/perf/page_sets/da... File tools/perf/page_sets/data/system_health_desktop.json (right): https://codereview.chromium.org/2168743004/diff/80001/tools/perf/page_sets/da... tools/perf/page_sets/data/system_health_desktop.json:90: "load:media:imgur" On 2016/08/04 13:32:42, petrcermak wrote: > I suggest you move this (manually - I know that the description says you > shouldn't do it) under system_health_desktop_025.wpr and delete > system_health_desktop_024.wpr altogether. That way, both the browsing and > loading will have exactly the same condition (better for comparison). Done. https://codereview.chromium.org/2168743004/diff/80001/tools/perf/page_sets/da... File tools/perf/page_sets/data/system_health_mobile.json (right): https://codereview.chromium.org/2168743004/diff/80001/tools/perf/page_sets/da... tools/perf/page_sets/data/system_health_mobile.json:99: "load:media:imgur" On 2016/08/04 13:32:42, petrcermak wrote: > ditto Done. https://codereview.chromium.org/2168743004/diff/80001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2168743004/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:247: class ImgurMobileStory(_MediaBrowsingStory): On 2016/08/04 13:32:42, petrcermak wrote: > Note that you could share common properties by defining abstract base classes. > Example: > > class _ImgurStory(_MediaBrowsingStory): > NAME = 'browse:media:imgur' > URL = 'http://imgur.com/gallery/5UlBN' > IS_SINGLE_APP = True > ABSTRACT_STORY = True > > class ImgurDesktopStory(_ImgurStory): > ITEM_SELECTOR = ... > SUPPORTED_PLATFORMS = platforms.DESKTOP_ONLY > > class ImgurMobileStory(_ImgurStory): > ... > > Feel free to ignore if you prefer it this way. Right on. I decided to keep the current structure to keep it uniform. https://codereview.chromium.org/2168743004/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:263: class YoutubeMobileStory(_MediaBrowsingStory): On 2016/08/04 13:32:42, petrcermak wrote: > nit: can you please change this to YouTubeMobileStory (capitalization of "T") so > that it's consistent with loading_stories.py? Done. https://codereview.chromium.org/2168743004/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:267: IS_SINGLE_PAGE_APP = True On 2016/08/04 13:32:42, petrcermak wrote: > nit: Can you please order the class fields consistently (here you have > IS_SINGLE_PAGE_APP before SUPPORTED_PLATFORMS, but the ordering is reversed in > YoutubeDesktopStory) Done. https://codereview.chromium.org/2168743004/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:272: class YoutubeDesktopStory(_MediaBrowsingStory): On 2016/08/04 13:32:42, petrcermak wrote: > ditto (capital "T") Done. https://codereview.chromium.org/2168743004/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:278: ITEM_VIEW_TIME_IN_SECONDS = 5 On 2016/08/04 13:32:42, petrcermak wrote: > Could you please add a short comment why you override the default arguments here > (e.g. because it crashes otherwise). Done. https://codereview.chromium.org/2168743004/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:284: NAME = 'browse:media:facebookphotos' On 2016/08/04 13:32:42, petrcermak wrote: > nit: I think facebook_photos would be better (consistent with 'google_images' in > loading_stories.py) Done. https://codereview.chromium.org/2168743004/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:293: action_runner.Navigate('https://m.facebook.com/NASA') On 2016/08/04 13:32:42, petrcermak wrote: > Why do you go to NASA and then back to Rihanna? If this is intentional, please > add a comment with an explanation Leftover, changed to rihanna. https://codereview.chromium.org/2168743004/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:295: On 2016/08/04 13:32:42, petrcermak wrote: > nit: too many blank lines (should be 2) Done. https://codereview.chromium.org/2168743004/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:299: NAME = 'browse:media:facebookphotos' On 2016/08/04 13:32:42, petrcermak wrote: > ditto (facebook_photos) Done. https://codereview.chromium.org/2168743004/diff/80001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/loading_stories.py (right): https://codereview.chromium.org/2168743004/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/loading_stories.py:244: On 2016/08/04 13:32:42, petrcermak wrote: > nit: add blank line (there should be two blank lines between top-level > definitions) Done.
LGTM with some final comments. I think you skipped my previous description nits (that's why I'm making them more visible): |||||||||||||||||||||||||||||||||||||||||||||||| |||||||||||||||||||||||||||||||||||||||||||||||| |||||||||||||||||||||||||||||||||||||||||||||||| vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv Two description comments: * s/Youtube/YouTube/ * add bug number please ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ |||||||||||||||||||||||||||||||||||||||||||||||| |||||||||||||||||||||||||||||||||||||||||||||||| |||||||||||||||||||||||||||||||||||||||||||||||| Thanks, Petr https://codereview.chromium.org/2168743004/diff/100001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2168743004/diff/100001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/browsing_stories.py:265: URL = 'https://m.youtube.com/watch?v=QGfhS1hfTWw&autoplay=false' Could you please update the loading stories to use this URL as well (you can do that by simply reusing the WPR archive like you did with imgur)? https://codereview.chromium.org/2168743004/diff/100001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/browsing_stories.py:285: NAME = 'browse:media:facebook_photos' Could you please add facebook_photos loading stories (again, you can reuse the WPR archive for the browsing ones)? We want to make sure that we have a loading story for each browsing story. https://codereview.chromium.org/2168743004/diff/100001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/browsing_stories.py:294: action_runner.Navigate('https://m.facebook.com/rihanna') Why do you have this _Login hook at all? It just adds an unnecessary navigation to https://m.facebook.com/rihanna. The story will automatically navigate to the URL on line 286 (https://m.facebook.com/rihanna/photos/a.207477806675.138795.10092511675/10153...). If there is a genuine reason for it, please document it in a comment. Otherwise, please remove this hook completely. https://codereview.chromium.org/2168743004/diff/100001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/browsing_stories.py:310: self.credentials_path) nit: this should be indented so that it's vertically aligned with "action_runner": facebook_login.LoginWithDesktopSite(action_runner, 'facebook3', self.credentials_path) ^ |
https://codereview.chromium.org/2168743004/diff/100001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2168743004/diff/100001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/browsing_stories.py:265: URL = 'https://m.youtube.com/watch?v=QGfhS1hfTWw&autoplay=false' On 2016/08/05 10:30:46, petrcermak wrote: > Could you please update the loading stories to use this URL as well (you can do > that by simply reusing the WPR archive like you did with imgur)? Please ignore this comment. I've just realized that these already are the same URLs O:-)
Description was changed from ========== [system health] Add media browsing stories. The workloads emulate real user behavior and is mainly used to benchmark memory consumption. Each media browsing story performs the following workload: 1. Load a page showing a media item 2. Click on the next link to go to the next media item 3. etc. The following websites were selected: - Facebook - Youtube - Imgur BUG= ========== to ========== [system health] Add media browsing stories. The workloads emulate real user behavior and is mainly used to benchmark memory consumption. Each media browsing story performs the following workload: 1. Load a page showing a media item 2. Click on the next link to go to the next media item 3. etc. The following websites were selected: - Facebook - YouTube - Imgur BUG= ==========
Description was changed from ========== [system health] Add media browsing stories. The workloads emulate real user behavior and is mainly used to benchmark memory consumption. Each media browsing story performs the following workload: 1. Load a page showing a media item 2. Click on the next link to go to the next media item 3. etc. The following websites were selected: - Facebook - YouTube - Imgur BUG= ========== to ========== [system health] Add media browsing stories. The workloads emulate real user behavior and is mainly used to benchmark memory consumption. Each media browsing story performs the following workload: 1. Load a page showing a media item 2. Click on the next link to go to the next media item 3. etc. The following websites were selected: - Facebook - YouTube - Imgur BUG=chromium:589726 ==========
Indeed, I overlooked the description comments. Fixed. https://codereview.chromium.org/2168743004/diff/100001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2168743004/diff/100001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/browsing_stories.py:265: URL = 'https://m.youtube.com/watch?v=QGfhS1hfTWw&autoplay=false' On 2016/08/05 10:32:08, petrcermak wrote: > On 2016/08/05 10:30:46, petrcermak wrote: > > Could you please update the loading stories to use this URL as well (you can > do > > that by simply reusing the WPR archive like you did with imgur)? > > Please ignore this comment. I've just realized that these already are the same > URLs O:-) Acknowledged. https://codereview.chromium.org/2168743004/diff/100001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/browsing_stories.py:285: NAME = 'browse:media:facebook_photos' On 2016/08/05 10:30:46, petrcermak wrote: > Could you please add facebook_photos loading stories (again, you can reuse the > WPR archive for the browsing ones)? We want to make sure that we have a loading > story for each browsing story. This is why I navigate to the regular page before. See comment below. I do not think that adding an additional facebook photo load story adds much value. WDYT? https://codereview.chromium.org/2168743004/diff/100001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/browsing_stories.py:294: action_runner.Navigate('https://m.facebook.com/rihanna') On 2016/08/05 10:30:46, petrcermak wrote: > Why do you have this _Login hook at all? It just adds an unnecessary navigation > to https://m.facebook.com/rihanna. The story will automatically navigate to the > URL on line 286 > (https://m.facebook.com/rihanna/photos/a.207477806675.138795.10092511675/10153...). > > If there is a genuine reason for it, please document it in a comment. Otherwise, > please remove this hook completely. To make it comparable to the load story. With that it will go to the page of the load story first and will then start opening photos and clicking through them. I am happy to remove it if you think that it is not necessary. https://codereview.chromium.org/2168743004/diff/100001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/browsing_stories.py:310: self.credentials_path) On 2016/08/05 10:30:46, petrcermak wrote: > nit: this should be indented so that it's vertically aligned with > "action_runner": > > facebook_login.LoginWithDesktopSite(action_runner, 'facebook3', > self.credentials_path) > ^ > | Done.
LGTM with one final comment. Thanks for your patience! Petr https://codereview.chromium.org/2168743004/diff/100001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2168743004/diff/100001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/browsing_stories.py:294: action_runner.Navigate('https://m.facebook.com/rihanna') On 2016/08/05 11:13:07, Hannes Payer wrote: > On 2016/08/05 10:30:46, petrcermak wrote: > > Why do you have this _Login hook at all? It just adds an unnecessary > navigation > > to https://m.facebook.com/rihanna. The story will automatically navigate to > the > > URL on line 286 > > > (https://m.facebook.com/rihanna/photos/a.207477806675.138795.10092511675/10153...). > > > > If there is a genuine reason for it, please document it in a comment. > Otherwise, > > please remove this hook completely. > > To make it comparable to the load story. With that it will go to the page of the > load story first and will then start opening photos and clicking through them. I > am happy to remove it if you think that it is not necessary. That makes the mobile and desktop browsing stories inconsistent though: Mobile: 1) go to https://m.facebook.com/rihanna 2) go to https://m.facebook.com/rihanna/photos/... Desktop: 1) log into Facebook 2) go to https://www.facebook.com/rihanna/photos/... Unless this is necessary for some reason, could you please: * Either login or don't login in *all* Facebook stories (load, browse, mobile, desktop), or describe why this is not possible. * Either navigate to https://m.facebook.com/rihanna in both browsing stories, or change the loading story to show https://m.facebook.com/rihanna/photos/a.207477806675.138795.10092511675/10153.... In either case, the loading stories should be able to reuse the WPR of the browsing stories.
The CQ bit was checked by hpayer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com, charliea@chromium.org, petrcermak@chromium.org Link to the patchset: https://codereview.chromium.org/2168743004/#ps160001 (title: "keep facebook separate")
https://codereview.chromium.org/2168743004/diff/100001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2168743004/diff/100001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/browsing_stories.py:294: action_runner.Navigate('https://m.facebook.com/rihanna') On 2016/08/08 13:52:16, petrcermak wrote: > On 2016/08/05 11:13:07, Hannes Payer wrote: > > On 2016/08/05 10:30:46, petrcermak wrote: > > > Why do you have this _Login hook at all? It just adds an unnecessary > > navigation > > > to https://m.facebook.com/rihanna. The story will automatically navigate to > > the > > > URL on line 286 > > > > > > (https://m.facebook.com/rihanna/photos/a.207477806675.138795.10092511675/10153...). > > > > > > If there is a genuine reason for it, please document it in a comment. > > Otherwise, > > > please remove this hook completely. > > > > To make it comparable to the load story. With that it will go to the page of > the > > load story first and will then start opening photos and clicking through them. > I > > am happy to remove it if you think that it is not necessary. > > That makes the mobile and desktop browsing stories inconsistent though: > > Mobile: 1) go to https://m.facebook.com/rihanna > 2) go to https://m.facebook.com/rihanna/photos/... > > Desktop: 1) log into Facebook > 2) go to https://www.facebook.com/rihanna/photos/... > > Unless this is necessary for some reason, could you please: > > * Either login or don't login in *all* Facebook stories (load, browse, mobile, > desktop), or describe why this is not possible. > * Either navigate to https://m.facebook.com/rihanna in both browsing stories, or > change the loading story to show > https://m.facebook.com/rihanna/photos/a.207477806675.138795.10092511675/10153.... > In either case, the loading stories should be able to reuse the WPR of the > browsing stories. I unified the facebook login and reuse the story also for load.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by hpayer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com, petrcermak@chromium.org, charliea@chromium.org Link to the patchset: https://codereview.chromium.org/2168743004/#ps180001 (title: "merge")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hpayer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com, petrcermak@chromium.org, charliea@chromium.org Link to the patchset: https://codereview.chromium.org/2168743004/#ps200001 (title: "disable facebook on desktop")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [system health] Add media browsing stories. The workloads emulate real user behavior and is mainly used to benchmark memory consumption. Each media browsing story performs the following workload: 1. Load a page showing a media item 2. Click on the next link to go to the next media item 3. etc. The following websites were selected: - Facebook - YouTube - Imgur BUG=chromium:589726 ========== to ========== [system health] Add media browsing stories. The workloads emulate real user behavior and is mainly used to benchmark memory consumption. Each media browsing story performs the following workload: 1. Load a page showing a media item 2. Click on the next link to go to the next media item 3. etc. The following websites were selected: - Facebook - YouTube - Imgur BUG=chromium:589726 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== [system health] Add media browsing stories. The workloads emulate real user behavior and is mainly used to benchmark memory consumption. Each media browsing story performs the following workload: 1. Load a page showing a media item 2. Click on the next link to go to the next media item 3. etc. The following websites were selected: - Facebook - YouTube - Imgur BUG=chromium:589726 ========== to ========== [system health] Add media browsing stories. The workloads emulate real user behavior and is mainly used to benchmark memory consumption. Each media browsing story performs the following workload: 1. Load a page showing a media item 2. Click on the next link to go to the next media item 3. etc. The following websites were selected: - Facebook - YouTube - Imgur BUG=chromium:589726 Committed: https://crrev.com/2b3bbf6088bf4b53cb1cd3ad63e038c2a699113f Cr-Commit-Position: refs/heads/master@{#410622} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/2b3bbf6088bf4b53cb1cd3ad63e038c2a699113f Cr-Commit-Position: refs/heads/master@{#410622} |
