Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2272993003:
[Telemetry] Convert long running user stories to the System Health format. (Closed)
|
Created:
4 years, 4 months ago by rnephew (Reviews Here) Modified:
4 years, 3 months ago 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[Telemetry] Convert long running user stories to the System Health format.
BUG=639315
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq
Committed: https://crrev.com/13d57aa167188a0ea4388634fcf4589d892002ff
Cr-Commit-Position: refs/heads/master@{#415702}
Patch Set 1 #
Total comments: 13
Patch Set 2 : [Telemetry] Convert long running user stories to the System Health format. #
Total comments: 9
Patch Set 3 : blacklist from smoke test #
Total comments: 5
Patch Set 4 : [Telemetry] Convert long running user stories to the System Health format. #
Messages
Total messages: 33 (5 generated)
Description was changed from ========== [Telemetry] Convert long running user stories to the System Health format. BUG=639315 ========== to ========== [Telemetry] Convert long running user stories to the System Health format. BUG=639315 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq ==========
rnephew@chromium.org changed reviewers: + charliea@chromium.org, nednguyen@google.com, perezju@chromium.org, petrcermak@chromium.org
https://codereview.chromium.org/2272993003/diff/1/tools/perf/benchmarks/batto... File tools/perf/benchmarks/battor.py (right): https://codereview.chromium.org/2272993003/diff/1/tools/perf/benchmarks/batto... tools/perf/benchmarks/battor.py:134: class DesktopBattOrLongRunning(_BattOrBenchmark): If my understanding of how the system_health.memory_* tests work is correct, the new pages should automatically be picked up by it correct? If that is the case I can get rid of these battor tests (or keep them, depending on popular consensus). I just wanted to make sure at least once benchmark is running the new pages.
Looks good overall. I've left a couple of inline comments. Thanks, Petr https://codereview.chromium.org/2272993003/diff/1/tools/perf/benchmarks/batto... File tools/perf/benchmarks/battor.py (right): https://codereview.chromium.org/2272993003/diff/1/tools/perf/benchmarks/batto... tools/perf/benchmarks/battor.py:133: nit: there should be 2 blank lines between top-level definitions. https://codereview.chromium.org/2272993003/diff/1/tools/perf/benchmarks/batto... tools/perf/benchmarks/battor.py:134: class DesktopBattOrLongRunning(_BattOrBenchmark): On 2016/08/24 17:50:45, rnephew (Reviews Here) wrote: > If my understanding of how the system_health.memory_* tests work is correct, the > new pages should automatically be picked up by it correct? > > If that is the case I can get rid of these battor tests (or keep them, depending > on popular consensus). I just wanted to make sure at least once benchmark is > running the new pages. All benchmarks that don't specify |case| or have case='long_running' will automatically pick these up. As far as I can tell, BattOr benchmarks have case='load' (https://cs.chromium.org/chromium/src/tools/perf/benchmarks/battor.py?l=63), so will NOT pick up your stories. Rather than define another benchmark, I suggest you either: 1) Change battor benchmarks to use ALL SH stories (not just the loading ones) by removing "case='load'". 2) Modify the SystemHealthStorySet constructor to accept a |cases| list of strings (instead of just a |case| string) and change the BattOr benchmark ot pass cases=['load', 'long_running']. https://codereview.chromium.org/2272993003/diff/1/tools/perf/benchmarks/batto... tools/perf/benchmarks/battor.py:134: class DesktopBattOrLongRunning(_BattOrBenchmark): If for some reason you decide to spin up a separate benchmark, then please pull all the common code (all three methods, CreateStorySet, ShouldTearDownStateAfterEachStoryRun, Name) into an abstract superclass. https://codereview.chromium.org/2272993003/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/long_running_stories.py (right): https://codereview.chromium.org/2272993003/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/long_running_stories.py:18: ITEM_SELECTOR = NotImplemented Remove these two constants (IS_SINGLE_PAGE_APP, ITEM_SELECTOR). You don't need them. https://codereview.chromium.org/2272993003/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/long_running_stories.py:26: action_runner.tab.browser.DumpMemory() instead of calling DumpMemory directly, please do the following: if self._take_memory_measurement: action_runner.MeasureMemory() https://codereview.chromium.org/2272993003/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/long_running_stories.py:33: # Long running gmail stories. nit: s/gmail/Gmail/ https://codereview.chromium.org/2272993003/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/long_running_stories.py:37: class _LongRunningGmailBase(_LongRunningStory): There's a lot of code duplication between this file and loading_stories.py (_Login and _DidLoadDocument methods of the Gmail stories). I suggest you either fix it (you'd probably need to define a new file in the system_health folder with common code such as CloseMobileInboxInterstitial, LogIntoGmail, ...), or at least add TODOs.
On 2016/08/25 09:57:19, petrcermak wrote: > Looks good overall. I've left a couple of inline comments. > > Thanks, > Petr > > https://codereview.chromium.org/2272993003/diff/1/tools/perf/benchmarks/batto... > File tools/perf/benchmarks/battor.py (right): > > https://codereview.chromium.org/2272993003/diff/1/tools/perf/benchmarks/batto... > tools/perf/benchmarks/battor.py:133: > nit: there should be 2 blank lines between top-level definitions. > > https://codereview.chromium.org/2272993003/diff/1/tools/perf/benchmarks/batto... > tools/perf/benchmarks/battor.py:134: class > DesktopBattOrLongRunning(_BattOrBenchmark): > On 2016/08/24 17:50:45, rnephew (Reviews Here) wrote: > > If my understanding of how the system_health.memory_* tests work is correct, > the > > new pages should automatically be picked up by it correct? > > > > If that is the case I can get rid of these battor tests (or keep them, > depending > > on popular consensus). I just wanted to make sure at least once benchmark is > > running the new pages. > > All benchmarks that don't specify |case| or have case='long_running' will > automatically pick these up. As far as I can tell, BattOr benchmarks have > case='load' > (https://cs.chromium.org/chromium/src/tools/perf/benchmarks/battor.py?l=63), so > will NOT pick up your stories. Rather than define another benchmark, I suggest > you either: > > 1) Change battor benchmarks to use ALL SH stories (not just the loading ones) by > removing "case='load'". > 2) Modify the SystemHealthStorySet constructor to accept a |cases| list of > strings (instead of just a |case| string) and change the BattOr benchmark ot > pass cases=['load', 'long_running']. > > https://codereview.chromium.org/2272993003/diff/1/tools/perf/benchmarks/batto... > tools/perf/benchmarks/battor.py:134: class > DesktopBattOrLongRunning(_BattOrBenchmark): > If for some reason you decide to spin up a separate benchmark, then please pull > all the common code (all three methods, CreateStorySet, > ShouldTearDownStateAfterEachStoryRun, Name) into an abstract superclass. > > https://codereview.chromium.org/2272993003/diff/1/tools/perf/page_sets/system... > File tools/perf/page_sets/system_health/long_running_stories.py (right): > > https://codereview.chromium.org/2272993003/diff/1/tools/perf/page_sets/system... > tools/perf/page_sets/system_health/long_running_stories.py:18: ITEM_SELECTOR = > NotImplemented > Remove these two constants (IS_SINGLE_PAGE_APP, ITEM_SELECTOR). You don't need > them. > > https://codereview.chromium.org/2272993003/diff/1/tools/perf/page_sets/system... > tools/perf/page_sets/system_health/long_running_stories.py:26: > action_runner.tab.browser.DumpMemory() > instead of calling DumpMemory directly, please do the following: > > if self._take_memory_measurement: > action_runner.MeasureMemory() > > https://codereview.chromium.org/2272993003/diff/1/tools/perf/page_sets/system... > tools/perf/page_sets/system_health/long_running_stories.py:33: # Long running > gmail stories. > nit: s/gmail/Gmail/ > > https://codereview.chromium.org/2272993003/diff/1/tools/perf/page_sets/system... > tools/perf/page_sets/system_health/long_running_stories.py:37: class > _LongRunningGmailBase(_LongRunningStory): > There's a lot of code duplication between this file and loading_stories.py > (_Login and _DidLoadDocument methods of the Gmail stories). I suggest you either > fix it (you'd probably need to define a new file in the system_health folder > with common code such as CloseMobileInboxInterstitial, LogIntoGmail, ...), or at > least add TODOs. How long does this run on desktop & mobile?
https://codereview.chromium.org/2272993003/diff/1/tools/perf/benchmarks/batto... File tools/perf/benchmarks/battor.py (right): https://codereview.chromium.org/2272993003/diff/1/tools/perf/benchmarks/batto... tools/perf/benchmarks/battor.py:133: On 2016/08/25 09:57:19, petrcermak wrote: > nit: there should be 2 blank lines between top-level definitions. Charlies CL at https://codereview.chromium.org/2239053003/ will cause this new pageset to run with battor tests, so I will be deleting these benchmarks. https://codereview.chromium.org/2272993003/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/long_running_stories.py (right): https://codereview.chromium.org/2272993003/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/long_running_stories.py:18: ITEM_SELECTOR = NotImplemented On 2016/08/25 09:57:19, petrcermak wrote: > Remove these two constants (IS_SINGLE_PAGE_APP, ITEM_SELECTOR). You don't need > them. Done. https://codereview.chromium.org/2272993003/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/long_running_stories.py:26: action_runner.tab.browser.DumpMemory() On 2016/08/25 09:57:19, petrcermak wrote: > instead of calling DumpMemory directly, please do the following: > > if self._take_memory_measurement: > action_runner.MeasureMemory() Done. https://codereview.chromium.org/2272993003/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/long_running_stories.py:33: # Long running gmail stories. On 2016/08/25 09:57:19, petrcermak wrote: > nit: s/gmail/Gmail/ Done. https://codereview.chromium.org/2272993003/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/long_running_stories.py:37: class _LongRunningGmailBase(_LongRunningStory): On 2016/08/25 09:57:19, petrcermak wrote: > There's a lot of code duplication between this file and loading_stories.py > (_Login and _DidLoadDocument methods of the Gmail stories). I suggest you either > fix it (you'd probably need to define a new file in the system_health folder > with common code such as CloseMobileInboxInterstitial, LogIntoGmail, ...), or at > least add TODOs. Added todo, will start working on merging them next week.
On 2016/08/25 15:28:58, rnephew (Reviews Here) wrote: > https://codereview.chromium.org/2272993003/diff/1/tools/perf/benchmarks/batto... > File tools/perf/benchmarks/battor.py (right): > > https://codereview.chromium.org/2272993003/diff/1/tools/perf/benchmarks/batto... > tools/perf/benchmarks/battor.py:133: > On 2016/08/25 09:57:19, petrcermak wrote: > > nit: there should be 2 blank lines between top-level definitions. > > Charlies CL at https://codereview.chromium.org/2239053003/ will cause this new > pageset to run with battor tests, so I will be deleting these benchmarks. > > https://codereview.chromium.org/2272993003/diff/1/tools/perf/page_sets/system... > File tools/perf/page_sets/system_health/long_running_stories.py (right): > > https://codereview.chromium.org/2272993003/diff/1/tools/perf/page_sets/system... > tools/perf/page_sets/system_health/long_running_stories.py:18: ITEM_SELECTOR = > NotImplemented > On 2016/08/25 09:57:19, petrcermak wrote: > > Remove these two constants (IS_SINGLE_PAGE_APP, ITEM_SELECTOR). You don't need > > them. > > Done. > > https://codereview.chromium.org/2272993003/diff/1/tools/perf/page_sets/system... > tools/perf/page_sets/system_health/long_running_stories.py:26: > action_runner.tab.browser.DumpMemory() > On 2016/08/25 09:57:19, petrcermak wrote: > > instead of calling DumpMemory directly, please do the following: > > > > if self._take_memory_measurement: > > action_runner.MeasureMemory() > > Done. > > https://codereview.chromium.org/2272993003/diff/1/tools/perf/page_sets/system... > tools/perf/page_sets/system_health/long_running_stories.py:33: # Long running > gmail stories. > On 2016/08/25 09:57:19, petrcermak wrote: > > nit: s/gmail/Gmail/ > > Done. > > https://codereview.chromium.org/2272993003/diff/1/tools/perf/page_sets/system... > tools/perf/page_sets/system_health/long_running_stories.py:37: class > _LongRunningGmailBase(_LongRunningStory): > On 2016/08/25 09:57:19, petrcermak wrote: > > There's a lot of code duplication between this file and loading_stories.py > > (_Login and _DidLoadDocument methods of the Gmail stories). I suggest you > either > > fix it (you'd probably need to define a new file in the system_health folder > > with common code such as CloseMobileInboxInterstitial, LogIntoGmail, ...), or > at > > least add TODOs. > > Added todo, will start working on merging them next week. After switching how the memory dump happens I am getting: Traceback (most recent call last): RunBenchmark at /usr/local/google/home/rnephew/chromium/clank/src/third_party/catapult/telemetry/telemetry/internal/story_runner.py:318 benchmark.ShouldTearDownStateAfterEachStorySetRun()) Run at /usr/local/google/home/rnephew/chromium/clank/src/third_party/catapult/telemetry/telemetry/internal/story_runner.py:227 _RunStoryAndProcessErrorIfNeeded(story, results, state, test) _RunStoryAndProcessErrorIfNeeded at /usr/local/google/home/rnephew/chromium/clank/src/third_party/catapult/telemetry/telemetry/internal/story_runner.py:86 state.RunStory(results) RunStory at /usr/local/google/home/rnephew/chromium/clank/src/third_party/catapult/telemetry/telemetry/page/shared_page_state.py:296 self._current_page.Run(self) Run at /usr/local/google/home/rnephew/chromium/clank/src/third_party/catapult/telemetry/telemetry/page/__init__.py:99 self.RunPageInteractions(action_runner) RunPageInteractions at /usr/local/google/home/rnephew/chromium/clank/src/tools/perf/page_sets/system_health/long_running_stories.py:29 action_runner.MeasureMemory() MeasureMemory at /usr/local/google/home/rnephew/chromium/clank/src/third_party/catapult/telemetry/telemetry/internal/actions/action_runner.py:138 assert dump_id, 'Unable to obtain memory dump' AssertionError: Unable to obtain memory dump Any idea why that may be happening? I'm OOO on the Chrome trip today, so I'll look more into it tomorrow when I'm in office.
On 2016/08/25 15:35:15, rnephew (Reviews Here) wrote: > On 2016/08/25 15:28:58, rnephew (Reviews Here) wrote: > > > https://codereview.chromium.org/2272993003/diff/1/tools/perf/benchmarks/batto... > > File tools/perf/benchmarks/battor.py (right): > > > > > https://codereview.chromium.org/2272993003/diff/1/tools/perf/benchmarks/batto... > > tools/perf/benchmarks/battor.py:133: > > On 2016/08/25 09:57:19, petrcermak wrote: > > > nit: there should be 2 blank lines between top-level definitions. > > > > Charlies CL at https://codereview.chromium.org/2239053003/ will cause this new > > pageset to run with battor tests, so I will be deleting these benchmarks. > > > > > https://codereview.chromium.org/2272993003/diff/1/tools/perf/page_sets/system... > > File tools/perf/page_sets/system_health/long_running_stories.py (right): > > > > > https://codereview.chromium.org/2272993003/diff/1/tools/perf/page_sets/system... > > tools/perf/page_sets/system_health/long_running_stories.py:18: ITEM_SELECTOR = > > NotImplemented > > On 2016/08/25 09:57:19, petrcermak wrote: > > > Remove these two constants (IS_SINGLE_PAGE_APP, ITEM_SELECTOR). You don't > need > > > them. > > > > Done. > > > > > https://codereview.chromium.org/2272993003/diff/1/tools/perf/page_sets/system... > > tools/perf/page_sets/system_health/long_running_stories.py:26: > > action_runner.tab.browser.DumpMemory() > > On 2016/08/25 09:57:19, petrcermak wrote: > > > instead of calling DumpMemory directly, please do the following: > > > > > > if self._take_memory_measurement: > > > action_runner.MeasureMemory() > > > > Done. > > > > > https://codereview.chromium.org/2272993003/diff/1/tools/perf/page_sets/system... > > tools/perf/page_sets/system_health/long_running_stories.py:33: # Long running > > gmail stories. > > On 2016/08/25 09:57:19, petrcermak wrote: > > > nit: s/gmail/Gmail/ > > > > Done. > > > > > https://codereview.chromium.org/2272993003/diff/1/tools/perf/page_sets/system... > > tools/perf/page_sets/system_health/long_running_stories.py:37: class > > _LongRunningGmailBase(_LongRunningStory): > > On 2016/08/25 09:57:19, petrcermak wrote: > > > There's a lot of code duplication between this file and loading_stories.py > > > (_Login and _DidLoadDocument methods of the Gmail stories). I suggest you > > either > > > fix it (you'd probably need to define a new file in the system_health folder > > > with common code such as CloseMobileInboxInterstitial, LogIntoGmail, ...), > or > > at > > > least add TODOs. > > > > Added todo, will start working on merging them next week. > > After switching how the memory dump happens I am getting: > Traceback (most recent call last): > RunBenchmark at > /usr/local/google/home/rnephew/chromium/clank/src/third_party/catapult/telemetry/telemetry/internal/story_runner.py:318 > benchmark.ShouldTearDownStateAfterEachStorySetRun()) > Run at > /usr/local/google/home/rnephew/chromium/clank/src/third_party/catapult/telemetry/telemetry/internal/story_runner.py:227 > _RunStoryAndProcessErrorIfNeeded(story, results, state, test) > _RunStoryAndProcessErrorIfNeeded at > /usr/local/google/home/rnephew/chromium/clank/src/third_party/catapult/telemetry/telemetry/internal/story_runner.py:86 > state.RunStory(results) > RunStory at > /usr/local/google/home/rnephew/chromium/clank/src/third_party/catapult/telemetry/telemetry/page/shared_page_state.py:296 > self._current_page.Run(self) > Run at > /usr/local/google/home/rnephew/chromium/clank/src/third_party/catapult/telemetry/telemetry/page/__init__.py:99 > self.RunPageInteractions(action_runner) > RunPageInteractions at > /usr/local/google/home/rnephew/chromium/clank/src/tools/perf/page_sets/system_health/long_running_stories.py:29 > action_runner.MeasureMemory() > MeasureMemory at > /usr/local/google/home/rnephew/chromium/clank/src/third_party/catapult/telemetry/telemetry/internal/actions/action_runner.py:138 > assert dump_id, 'Unable to obtain memory dump' > AssertionError: Unable to obtain memory dump > > Any idea why that may be happening? I'm OOO on the Chrome trip today, so I'll > look more into it tomorrow when I'm in office. Do you have a recent enough version of Chrome on the device? It's possible that your previous implementation was not getting dumps anyway and just silently grabbing no memory measurements at all.
On 2016/08/25 15:48:40, perezju wrote: > On 2016/08/25 15:35:15, rnephew (Reviews Here) wrote: > > On 2016/08/25 15:28:58, rnephew (Reviews Here) wrote: > > > > > > https://codereview.chromium.org/2272993003/diff/1/tools/perf/benchmarks/batto... > > > File tools/perf/benchmarks/battor.py (right): > > > > > > > > > https://codereview.chromium.org/2272993003/diff/1/tools/perf/benchmarks/batto... > > > tools/perf/benchmarks/battor.py:133: > > > On 2016/08/25 09:57:19, petrcermak wrote: > > > > nit: there should be 2 blank lines between top-level definitions. > > > > > > Charlies CL at https://codereview.chromium.org/2239053003/ will cause this > new > > > pageset to run with battor tests, so I will be deleting these benchmarks. > > > > > > > > > https://codereview.chromium.org/2272993003/diff/1/tools/perf/page_sets/system... > > > File tools/perf/page_sets/system_health/long_running_stories.py (right): > > > > > > > > > https://codereview.chromium.org/2272993003/diff/1/tools/perf/page_sets/system... > > > tools/perf/page_sets/system_health/long_running_stories.py:18: ITEM_SELECTOR > = > > > NotImplemented > > > On 2016/08/25 09:57:19, petrcermak wrote: > > > > Remove these two constants (IS_SINGLE_PAGE_APP, ITEM_SELECTOR). You don't > > need > > > > them. > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2272993003/diff/1/tools/perf/page_sets/system... > > > tools/perf/page_sets/system_health/long_running_stories.py:26: > > > action_runner.tab.browser.DumpMemory() > > > On 2016/08/25 09:57:19, petrcermak wrote: > > > > instead of calling DumpMemory directly, please do the following: > > > > > > > > if self._take_memory_measurement: > > > > action_runner.MeasureMemory() > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2272993003/diff/1/tools/perf/page_sets/system... > > > tools/perf/page_sets/system_health/long_running_stories.py:33: # Long > running > > > gmail stories. > > > On 2016/08/25 09:57:19, petrcermak wrote: > > > > nit: s/gmail/Gmail/ > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2272993003/diff/1/tools/perf/page_sets/system... > > > tools/perf/page_sets/system_health/long_running_stories.py:37: class > > > _LongRunningGmailBase(_LongRunningStory): > > > On 2016/08/25 09:57:19, petrcermak wrote: > > > > There's a lot of code duplication between this file and loading_stories.py > > > > (_Login and _DidLoadDocument methods of the Gmail stories). I suggest you > > > either > > > > fix it (you'd probably need to define a new file in the system_health > folder > > > > with common code such as CloseMobileInboxInterstitial, LogIntoGmail, ...), > > or > > > at > > > > least add TODOs. > > > > > > Added todo, will start working on merging them next week. > > > > After switching how the memory dump happens I am getting: > > Traceback (most recent call last): > > RunBenchmark at > > > /usr/local/google/home/rnephew/chromium/clank/src/third_party/catapult/telemetry/telemetry/internal/story_runner.py:318 > > benchmark.ShouldTearDownStateAfterEachStorySetRun()) > > Run at > > > /usr/local/google/home/rnephew/chromium/clank/src/third_party/catapult/telemetry/telemetry/internal/story_runner.py:227 > > _RunStoryAndProcessErrorIfNeeded(story, results, state, test) > > _RunStoryAndProcessErrorIfNeeded at > > > /usr/local/google/home/rnephew/chromium/clank/src/third_party/catapult/telemetry/telemetry/internal/story_runner.py:86 > > state.RunStory(results) > > RunStory at > > > /usr/local/google/home/rnephew/chromium/clank/src/third_party/catapult/telemetry/telemetry/page/shared_page_state.py:296 > > self._current_page.Run(self) > > Run at > > > /usr/local/google/home/rnephew/chromium/clank/src/third_party/catapult/telemetry/telemetry/page/__init__.py:99 > > self.RunPageInteractions(action_runner) > > RunPageInteractions at > > > /usr/local/google/home/rnephew/chromium/clank/src/tools/perf/page_sets/system_health/long_running_stories.py:29 > > action_runner.MeasureMemory() > > MeasureMemory at > > > /usr/local/google/home/rnephew/chromium/clank/src/third_party/catapult/telemetry/telemetry/internal/actions/action_runner.py:138 > > assert dump_id, 'Unable to obtain memory dump' > > AssertionError: Unable to obtain memory dump > > > > Any idea why that may be happening? I'm OOO on the Chrome trip today, so I'll > > look more into it tomorrow when I'm in office. > > Do you have a recent enough version of Chrome on the device? It's possible that > your previous implementation was not getting dumps anyway and just silently > grabbing no memory measurements at all. I'm surprised that your dumping memory at all. Which benchmark are you running? The memory one? No memory dumps should be carried out in the power benchmark.
> I'm surprised that your dumping memory at all. Which benchmark are you running? > The memory one? No memory dumps should be carried out in the power benchmark. Im running system_health.memory_mobile --story-filter="long_running" just to test the pageset. I got rid of the power benchmark in this CL since charlies CL will add battor metrics to the system health runs; didn't want two benchmarks with the same pageset running the same metrics. My guess is Juan is correct in that it has an old version of Chrome on the device.
On 2016/08/25 17:17:37, rnephew (Reviews Here) wrote: > > I'm surprised that your dumping memory at all. Which benchmark are you > running? > > The memory one? No memory dumps should be carried out in the power benchmark. > > Im running system_health.memory_mobile --story-filter="long_running" just to > test the pageset. I got rid of the power benchmark in this CL since charlies CL > will add battor metrics to the system health runs; didn't want two benchmarks > with the same pageset running the same metrics. > > My guess is Juan is correct in that it has an old version of Chrome on the > device. I reinstalled chrome.apk on my device after a fresh build and it worked. rnephew@rnephew0:~/chromium/clank/src/tools/perf$ time ./run_benchmark --browser=android-chrome system_health.memory_mobile --story-filter=long_running 77 KB/s (3387 bytes in 0.042s) [ RUN ] long_running:tools:gmail-background [ RUN ] /tmp/tmpPemCD2.html [ OK ] /tmp/tmpPemCD2.html (17295 ms) [ OK ] long_running:tools:gmail-background (240960 ms) 71 KB/s (3387 bytes in 0.045s) [ RUN ] long_running:tools:gmail-foreground [ RUN ] /tmp/tmprE_vts.html [ OK ] /tmp/tmprE_vts.html (20558 ms) [ OK ] long_running:tools:gmail-foreground (239066 ms) [ PASSED ] 2 tests. real 8m16.903s user 3m43.634s sys 0m32.344s rnephew@rnephew0:~/chromium/clank/src/tools/perf$ time ./run_benchmark --browser=stable system_health.memory_desktop --story-filter=long_running [ RUN ] long_running:tools:gmail-background [ RUN ] /tmp/tmpzmHDau.html [ OK ] /tmp/tmpzmHDau.html (28818 ms) [ OK ] long_running:tools:gmail-background (207856 ms) (WARNING) 2016-08-26 10:25:35,620 desktop_browser_finder.FindAllAvailableBrowsers:147 Chrome build location for linux_x86_64 not found. Browser will be run without Flash. [ RUN ] long_running:tools:gmail-foreground [ RUN ] /tmp/tmpWdFnP2.html [ OK ] /tmp/tmpWdFnP2.html (20448 ms) [ OK ] long_running:tools:gmail-foreground (183260 ms) [ PASSED ] 2 tests. real 6m42.572s user 4m8.985s sys 0m30.458s The trace processing takes awhile, but these traces are longer than the ones we are use to dealing with so that makes sense.
On 2016/08/26 17:29:57, rnephew (Reviews Here) wrote: > On 2016/08/25 17:17:37, rnephew (Reviews Here) wrote: > > > I'm surprised that your dumping memory at all. Which benchmark are you > > running? > > > The memory one? No memory dumps should be carried out in the power > benchmark. > > > > Im running system_health.memory_mobile --story-filter="long_running" just to > > test the pageset. I got rid of the power benchmark in this CL since charlies > CL > > will add battor metrics to the system health runs; didn't want two benchmarks > > with the same pageset running the same metrics. > > > > My guess is Juan is correct in that it has an old version of Chrome on the > > device. > > I reinstalled chrome.apk on my device after a fresh build and it worked. > > rnephew@rnephew0:~/chromium/clank/src/tools/perf$ time ./run_benchmark > --browser=android-chrome system_health.memory_mobile --story-filter=long_running > 77 KB/s (3387 bytes in 0.042s) > [ RUN ] long_running:tools:gmail-background > [ RUN ] /tmp/tmpPemCD2.html > [ OK ] /tmp/tmpPemCD2.html (17295 ms) > [ OK ] long_running:tools:gmail-background (240960 ms) > 71 KB/s (3387 bytes in 0.045s) > [ RUN ] long_running:tools:gmail-foreground > [ RUN ] /tmp/tmprE_vts.html > [ OK ] /tmp/tmprE_vts.html (20558 ms) > [ OK ] long_running:tools:gmail-foreground (239066 ms) > [ PASSED ] 2 tests. > > > real 8m16.903s > user 3m43.634s > sys 0m32.344s > > > rnephew@rnephew0:~/chromium/clank/src/tools/perf$ time ./run_benchmark > --browser=stable system_health.memory_desktop --story-filter=long_running > [ RUN ] long_running:tools:gmail-background > [ RUN ] /tmp/tmpzmHDau.html > [ OK ] /tmp/tmpzmHDau.html (28818 ms) > [ OK ] long_running:tools:gmail-background (207856 ms) > (WARNING) 2016-08-26 10:25:35,620 > desktop_browser_finder.FindAllAvailableBrowsers:147 Chrome build location for > linux_x86_64 not found. Browser will be run without Flash. > [ RUN ] long_running:tools:gmail-foreground > [ RUN ] /tmp/tmpWdFnP2.html > [ OK ] /tmp/tmpWdFnP2.html (20448 ms) > [ OK ] long_running:tools:gmail-foreground (183260 ms) > [ PASSED ] 2 tests. > > > real 6m42.572s > user 4m8.985s > sys 0m30.458s > > > The trace processing takes awhile, but these traces are longer than the ones we > are use to dealing with so that makes sense. ping.
https://codereview.chromium.org/2272993003/diff/20001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/long_running_stories.py (right): https://codereview.chromium.org/2272993003/diff/20001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/long_running_stories.py:10: IDLE_TIME_IN_SECONDS = 100 Why are these constants up at the top of the file and another constant is defined within LongRunningGmailBase? https://codereview.chromium.org/2272993003/diff/20001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/long_running_stories.py:26: for _ in xrange(STEPS): If the memory measurement takes any significant amount of time, this is likely to cause some weird problems. For example, if it takes 0.25s, and there are 100 steps, then the first sample will start at 1s and end at 1.25s, the second sample will start at 2.25s and end at 2.5s, the third sample will start at 3.5s and end at 3.75s, etc. Probably the expected behavior would be that you'd generate the sampling times beforehand (1s, 2s, 3s, 4s), and only wait until the next sampling time. (Or use some other algorithm that yields the same result.)
https://codereview.chromium.org/2272993003/diff/20001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/long_running_stories.py (right): https://codereview.chromium.org/2272993003/diff/20001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/long_running_stories.py:10: IDLE_TIME_IN_SECONDS = 100 On 2016/08/29 17:36:00, charliea (OOO until 8-29) wrote: > Why are these constants up at the top of the file and another constant is > defined within LongRunningGmailBase? The ABSTRACT_STORY is used by the superclass, and background needs to be set on a per-story basis. I only named background all caps to match the style of the other class variables (like abstract_story). https://codereview.chromium.org/2272993003/diff/20001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/long_running_stories.py:26: for _ in xrange(STEPS): On 2016/08/29 17:36:00, charliea (OOO until 8-29) wrote: > If the memory measurement takes any significant amount of time, this is likely > to cause some weird problems. > > For example, if it takes 0.25s, and there are 100 steps, then the first sample > will start at 1s and end at 1.25s, the second sample will start at 2.25s and end > at 2.5s, the third sample will start at 3.5s and end at 3.75s, etc. > > Probably the expected behavior would be that you'd generate the sampling times > beforehand (1s, 2s, 3s, 4s), and only wait until the next sampling time. (Or use > some other algorithm that yields the same result.) This is the behavior of the old version as well though. Juan, is this ok? Or should it be fixed.
LGTM if everyone else is happy. Thanks, Petr https://codereview.chromium.org/2272993003/diff/20001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/long_running_stories.py (right): https://codereview.chromium.org/2272993003/diff/20001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/long_running_stories.py:10: IDLE_TIME_IN_SECONDS = 100 On 2016/08/29 17:42:17, rnephew (Reviews Here) wrote: > On 2016/08/29 17:36:00, charliea (OOO until 8-29) wrote: > > Why are these constants up at the top of the file and another constant is > > defined within LongRunningGmailBase? > > The ABSTRACT_STORY is used by the superclass, and background needs to be set on > a per-story basis. I only named background all caps to match the style of the > other class variables (like abstract_story). I think that the current "layout" is correct. https://codereview.chromium.org/2272993003/diff/20001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/long_running_stories.py:26: for _ in xrange(STEPS): On 2016/08/29 17:42:17, rnephew (Reviews Here) wrote: > On 2016/08/29 17:36:00, charliea (OOO until 8-29) wrote: > > If the memory measurement takes any significant amount of time, this is likely > > to cause some weird problems. > > > > For example, if it takes 0.25s, and there are 100 steps, then the first sample > > will start at 1s and end at 1.25s, the second sample will start at 2.25s and > end > > at 2.5s, the third sample will start at 3.5s and end at 3.75s, etc. > > > > Probably the expected behavior would be that you'd generate the sampling times > > beforehand (1s, 2s, 3s, 4s), and only wait until the next sampling time. (Or > use > > some other algorithm that yields the same result.) > > This is the behavior of the old version as well though. Juan, is this ok? Or > should it be fixed. I'm fine either way. Juan: Any thoughts?
On 2016/08/30 12:04:01, petrcermak wrote: > LGTM if everyone else is happy. > > Thanks, > Petr > > https://codereview.chromium.org/2272993003/diff/20001/tools/perf/page_sets/sy... > File tools/perf/page_sets/system_health/long_running_stories.py (right): > > https://codereview.chromium.org/2272993003/diff/20001/tools/perf/page_sets/sy... > tools/perf/page_sets/system_health/long_running_stories.py:10: > IDLE_TIME_IN_SECONDS = 100 > On 2016/08/29 17:42:17, rnephew (Reviews Here) wrote: > > On 2016/08/29 17:36:00, charliea (OOO until 8-29) wrote: > > > Why are these constants up at the top of the file and another constant is > > > defined within LongRunningGmailBase? > > > > The ABSTRACT_STORY is used by the superclass, and background needs to be set > on > > a per-story basis. I only named background all caps to match the style of the > > other class variables (like abstract_story). > > I think that the current "layout" is correct. > > https://codereview.chromium.org/2272993003/diff/20001/tools/perf/page_sets/sy... > tools/perf/page_sets/system_health/long_running_stories.py:26: for _ in > xrange(STEPS): > On 2016/08/29 17:42:17, rnephew (Reviews Here) wrote: > > On 2016/08/29 17:36:00, charliea (OOO until 8-29) wrote: > > > If the memory measurement takes any significant amount of time, this is > likely > > > to cause some weird problems. > > > > > > For example, if it takes 0.25s, and there are 100 steps, then the first > sample > > > will start at 1s and end at 1.25s, the second sample will start at 2.25s and > > end > > > at 2.5s, the third sample will start at 3.5s and end at 3.75s, etc. > > > > > > Probably the expected behavior would be that you'd generate the sampling > times > > > beforehand (1s, 2s, 3s, 4s), and only wait until the next sampling time. (Or > > use > > > some other algorithm that yields the same result.) > > > > This is the behavior of the old version as well though. Juan, is this ok? Or > > should it be fixed. > > I'm fine either way. Juan: Any thoughts? For now, can you black list this long story in https://cs.chromium.org/chromium/src/tools/perf/benchmarks/system_health_smok... ?
On 2016/08/30 12:08:21, nednguyen wrote: > On 2016/08/30 12:04:01, petrcermak wrote: > > LGTM if everyone else is happy. > > > > Thanks, > > Petr > > > > > https://codereview.chromium.org/2272993003/diff/20001/tools/perf/page_sets/sy... > > File tools/perf/page_sets/system_health/long_running_stories.py (right): > > > > > https://codereview.chromium.org/2272993003/diff/20001/tools/perf/page_sets/sy... > > tools/perf/page_sets/system_health/long_running_stories.py:10: > > IDLE_TIME_IN_SECONDS = 100 > > On 2016/08/29 17:42:17, rnephew (Reviews Here) wrote: > > > On 2016/08/29 17:36:00, charliea (OOO until 8-29) wrote: > > > > Why are these constants up at the top of the file and another constant is > > > > defined within LongRunningGmailBase? > > > > > > The ABSTRACT_STORY is used by the superclass, and background needs to be set > > on > > > a per-story basis. I only named background all caps to match the style of > the > > > other class variables (like abstract_story). > > > > I think that the current "layout" is correct. > > > > > https://codereview.chromium.org/2272993003/diff/20001/tools/perf/page_sets/sy... > > tools/perf/page_sets/system_health/long_running_stories.py:26: for _ in > > xrange(STEPS): > > On 2016/08/29 17:42:17, rnephew (Reviews Here) wrote: > > > On 2016/08/29 17:36:00, charliea (OOO until 8-29) wrote: > > > > If the memory measurement takes any significant amount of time, this is > > likely > > > > to cause some weird problems. > > > > > > > > For example, if it takes 0.25s, and there are 100 steps, then the first > > sample > > > > will start at 1s and end at 1.25s, the second sample will start at 2.25s > and > > > end > > > > at 2.5s, the third sample will start at 3.5s and end at 3.75s, etc. > > > > > > > > Probably the expected behavior would be that you'd generate the sampling > > times > > > > beforehand (1s, 2s, 3s, 4s), and only wait until the next sampling time. > (Or > > > use > > > > some other algorithm that yields the same result.) > > > > > > This is the behavior of the old version as well though. Juan, is this ok? Or > > > should it be fixed. > > > > I'm fine either way. Juan: Any thoughts? > > For now, can you black list this long story in > https://cs.chromium.org/chromium/src/tools/perf/benchmarks/system_health_smok... > ? +1
https://codereview.chromium.org/2272993003/diff/20001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/long_running_stories.py (right): https://codereview.chromium.org/2272993003/diff/20001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/long_running_stories.py:26: for _ in xrange(STEPS): On 2016/08/30 12:04:01, petrcermak wrote: > On 2016/08/29 17:42:17, rnephew (Reviews Here) wrote: > > On 2016/08/29 17:36:00, charliea (OOO until 8-29) wrote: > > > If the memory measurement takes any significant amount of time, this is > likely > > > to cause some weird problems. > > > > > > For example, if it takes 0.25s, and there are 100 steps, then the first > sample > > > will start at 1s and end at 1.25s, the second sample will start at 2.25s and > > end > > > at 2.5s, the third sample will start at 3.5s and end at 3.75s, etc. > > > > > > Probably the expected behavior would be that you'd generate the sampling > times > > > beforehand (1s, 2s, 3s, 4s), and only wait until the next sampling time. (Or > > use > > > some other algorithm that yields the same result.) > > > > This is the behavior of the old version as well though. Juan, is this ok? Or > > should it be fixed. > > I'm fine either way. Juan: Any thoughts? I think it should be fine to keep as is.
Added them to the blacklist. PTAL.
https://codereview.chromium.org/2272993003/diff/40001/tools/perf/benchmarks/s... File tools/perf/benchmarks/system_health_smoke_test.py (left): https://codereview.chromium.org/2272993003/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:51: 'benchmarks.system_health_smoke_test.SystemHealthBenchmarkSmokeTest.system_health.memory_mobile.load:news:cnn', # pylint: disable=line-too-long is this re-enabling load:news:cnn by mistake?
https://codereview.chromium.org/2272993003/diff/40001/tools/perf/benchmarks/s... File tools/perf/benchmarks/system_health_smoke_test.py (left): https://codereview.chromium.org/2272993003/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:51: 'benchmarks.system_health_smoke_test.SystemHealthBenchmarkSmokeTest.system_health.memory_mobile.load:news:cnn', # pylint: disable=line-too-long On 2016/08/31 08:25:41, perezju wrote: > is this re-enabling load:news:cnn by mistake? +1 https://codereview.chromium.org/2272993003/diff/40001/tools/perf/benchmarks/s... File tools/perf/benchmarks/system_health_smoke_test.py (right): https://codereview.chromium.org/2272993003/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:57: nit: remove blank line
lgtm Watch out for OOM memory on the newly added system_health.common_* benchmark
https://codereview.chromium.org/2272993003/diff/20001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/long_running_stories.py (right): https://codereview.chromium.org/2272993003/diff/20001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/long_running_stories.py:10: IDLE_TIME_IN_SECONDS = 100 On 2016/08/30 12:04:01, petrcermak wrote: > On 2016/08/29 17:42:17, rnephew (Reviews Here) wrote: > > On 2016/08/29 17:36:00, charliea (OOO until 8-29) wrote: > > > Why are these constants up at the top of the file and another constant is > > > defined within LongRunningGmailBase? > > > > The ABSTRACT_STORY is used by the superclass, and background needs to be set > on > > a per-story basis. I only named background all caps to match the style of the > > other class variables (like abstract_story). > > I think that the current "layout" is correct. Acknowledged. https://codereview.chromium.org/2272993003/diff/20001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/long_running_stories.py:26: for _ in xrange(STEPS): On 2016/08/30 17:05:50, perezju wrote: > On 2016/08/30 12:04:01, petrcermak wrote: > > On 2016/08/29 17:42:17, rnephew (Reviews Here) wrote: > > > On 2016/08/29 17:36:00, charliea (OOO until 8-29) wrote: > > > > If the memory measurement takes any significant amount of time, this is > > likely > > > > to cause some weird problems. > > > > > > > > For example, if it takes 0.25s, and there are 100 steps, then the first > > sample > > > > will start at 1s and end at 1.25s, the second sample will start at 2.25s > and > > > end > > > > at 2.5s, the third sample will start at 3.5s and end at 3.75s, etc. > > > > > > > > Probably the expected behavior would be that you'd generate the sampling > > times > > > > beforehand (1s, 2s, 3s, 4s), and only wait until the next sampling time. > (Or > > > use > > > > some other algorithm that yields the same result.) > > > > > > This is the behavior of the old version as well though. Juan, is this ok? Or > > > should it be fixed. > > > > I'm fine either way. Juan: Any thoughts? > > I think it should be fine to keep as is. Acknowledged. https://codereview.chromium.org/2272993003/diff/40001/tools/perf/benchmarks/s... File tools/perf/benchmarks/system_health_smoke_test.py (left): https://codereview.chromium.org/2272993003/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:51: 'benchmarks.system_health_smoke_test.SystemHealthBenchmarkSmokeTest.system_health.memory_mobile.load:news:cnn', # pylint: disable=line-too-long On 2016/08/31 11:31:10, petrcermak wrote: > On 2016/08/31 08:25:41, perezju wrote: > > is this re-enabling load:news:cnn by mistake? > > +1 Yeah, not on purpose :/ https://codereview.chromium.org/2272993003/diff/40001/tools/perf/benchmarks/s... File tools/perf/benchmarks/system_health_smoke_test.py (right): https://codereview.chromium.org/2272993003/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health_smoke_test.py:57: On 2016/08/31 11:31:10, petrcermak wrote: > nit: remove blank line Done.
The CQ bit was checked by rnephew@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from petrcermak@chromium.org, nednguyen@google.com Link to the patchset: https://codereview.chromium.org/2272993003/#ps60001 (title: "[Telemetry] Convert long running user stories to the System Health format.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [Telemetry] Convert long running user stories to the System Health format. BUG=639315 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq ========== to ========== [Telemetry] Convert long running user stories to the System Health format. BUG=639315 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq Committed: https://crrev.com/13d57aa167188a0ea4388634fcf4589d892002ff Cr-Commit-Position: refs/heads/master@{#415702} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/13d57aa167188a0ea4388634fcf4589d892002ff Cr-Commit-Position: refs/heads/master@{#415702}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2302803002/ by petrcermak@chromium.org. The reason for reverting is: The patch broke perf bots because it doesn't contain WPR recordings..
Message was sent while issue was closed.
On 2016/09/01 10:38:10, petrcermak wrote: > A revert of this CL (patchset #4 id:60001) has been created in > https://codereview.chromium.org/2302803002/ by mailto:petrcermak@chromium.org. > > The reason for reverting is: The patch broke perf bots because it doesn't > contain WPR recordings.. To be more specific, it doesn't contain the sha1 files for the WPR recordings. Uploading a CL that includes those momentarily. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
