Have repeats understand "time"

tools/telemetry/telemetry/page/page_runner.py

Index: tools/telemetry/telemetry/page/page_runner.py
diff --git a/tools/telemetry/telemetry/page/page_runner.py b/tools/telemetry/telemetry/page/page_runner.py
index 9ec2d348dc654ad955b6ad52f8ae083897ba6601..cbe8cbfcb2cfa54eba06836be9e0dbd96b18c77e 100644
--- a/tools/telemetry/telemetry/page/page_runner.py
+++ b/tools/telemetry/telemetry/page/page_runner.py
@@ -71,7 +71,6 @@ class _RunState(object):
 
     if self.first_page:
       self.first_page = False
-      test.WillRunPageSet(self.tab)
 
   def StopBrowser(self):
     self._is_tracing = False
@@ -91,7 +90,8 @@ class _RunState(object):
 
   def StartProfiling(self, page, options):
     output_file = os.path.join(options.profiler_dir, page.url_as_file_safe_name)
-    if options.page_repeat != 1 or options.pageset_repeat != 1:
+    if (options.page_repeat != 1 or options.pageset_repeat != 1 or
+        options.page_repeat_time or options.pageset_repeat_time):
       output_file = _GetSequentialFileName(output_file)
     self.browser.StartProfiling(options, output_file)
 
@@ -116,7 +116,8 @@ class _RunState(object):
     logging.info('Processing trace...')
 
     trace_file = os.path.join(options.trace_dir, page.url_as_file_safe_name)
-    if options.page_repeat != 1 or options.pageset_repeat != 1:
+    if (options.page_repeat != 1 or options.pageset_repeat != 1 or
+        options.page_repeat_time or options.pageset_repeat_time):
       trace_file = _GetSequentialFileName(trace_file)
     trace_file += '.json'
 
@@ -165,6 +166,97 @@ def AddCommandLineOptions(parser):
   page_filter_module.PageFilter.AddCommandLineOptions(parser)
 
 
+def _LaunchAndRunPage(test, page_set, options, page, credentials_path,
+                  possible_browser, results, state):

[dtu, 2013/07/10 23:15:41]

I'm glad you made this a separate method. The nesting level was getting kind of
crazy :)
How about _PrepareAndRunPage?

[edmundyan, 2013/07/11 01:18:55]

Done.

+  if options.wpr_mode != wpr_modes.WPR_RECORD:
+    if page.archive_path and os.path.isfile(page.archive_path):
+      possible_browser.options.wpr_mode = wpr_modes.WPR_REPLAY
+    else:
+      possible_browser.options.wpr_mode = wpr_modes.WPR_OFF
+  # TODO(eyan): maybe related to dtu's TODO, but results_for_current_run is

[dennis_jeffrey, 2013/07/10 20:11:20]

put your full official LDAP here

+  # not used

[dennis_jeffrey, 2013/07/10 20:11:20]

Are you sure it's unused?  I thought this was an important object that stores
results of a run?

[edmundyan, 2013/07/10 21:13:09]

I mean the re-assigning to the variable 'results_for_current_run' is
unnecessary.  We should just modify 'results'

[dtu, 2013/07/10 23:15:41]

Yeah, this is kind of hacky and the logic of it isn't obvious. (I take no
responsibility for it :) The test expects a results object and adds measurement
numbers to it.  Some tests only want to record numbers for runs after the first
(e.g. to test only with a warm cache). To do that, we just replace the "one
true" results object with a dummy object, which is then discarded.

Maybe you can add a comment here to make it clearer.

[edmundyan, 2013/07/11 01:18:55]

Oh, I understand the reason for this now.  I think the comment under the if
condition is fine.

[dtu, 2013/07/11 17:28:38]

If you got confused, other people will get confused too. I don't think the
comment is sufficient.

+  results_for_current_run = results
+  if state.first_page and test.discard_first_result:
+    # If discarding results, substitute a dummy object.
+    results_for_current_run = type(results)()
+  results_for_current_run.StartTest(page)
+  tries = 3
+  while tries:
+    try:
+      state.StartBrowser(test, page_set, page, possible_browser,
+                         credentials_path, page.archive_path)
+
+      _WaitForThermalThrottlingIfNeeded(state.browser.platform)
+
+      if options.trace_dir:
+        state.StartTracing()
+      if options.profiler_dir:
+        state.StartProfiling(page, options)
+
+      try:
+        _RunPage(test, page, state.tab, results_for_current_run, options)
+        _CheckThermalThrottling(state.browser.platform)
+      except exceptions.TabCrashException:
+        stdout = ''
+        if not options.show_stdout:
+          stdout = state.browser.GetStandardOutput()
+          stdout = (('\nStandard Output:\n') +
+                    ('*' * 80) +
+                    '\n\t' + stdout.replace('\n', '\n\t') + '\n' +
+                    ('*' * 80))
+        logging.warning('Tab crashed: %s%s', page.url, stdout)
+        state.StopBrowser()
+
+      if options.trace_dir:
+        state.StopTracing(page, options)
+      if options.profiler_dir:
+        state.StopProfiling()
+
+      if test.NeedsBrowserRestartAfterEachRun(state.tab):
+        state.StopBrowser()
+
+      break
+    except exceptions.BrowserGoneException:
+      logging.warning('Lost connection to browser. Retrying.')
+      state.StopBrowser()
+      tries -= 1
+      if not tries:
+        logging.error('Lost connection to browser 3 times. Failing.')
+        raise
+  results_for_current_run.StopTest(page)
+
+
+def _RunIters(test, page_set, options, credentials_path, possible_browser,
+              results, state, pages):
+  """Runs each page or pageset for a specified number of iterations"""
+  for page in [p
+              for _ in xrange(int(options.pageset_repeat))
+              for p in pages
+              for _ in xrange(int(options.page_repeat))]:
+    _LaunchAndRunPage(test, page_set, options, page, credentials_path,
+                  possible_browser, results, state)
+
+
+def _RunTime(test, page_set, options, credentials_path, possible_browser,
+             results, state, pages):
+  """Runs each page or pageset until a specified amount of time"""
+  pageset_time = 0.0
+  while True:
+    for page in pages:
+      page_time = 0.0
+      while True:
+        start = time.time()
+        _LaunchAndRunPage(test, page_set, options, page, credentials_path,
+                      possible_browser, results, state)
+        duration = time.time() - start
+        page_time += duration
+        pageset_time += duration
+        if page_time > float(options.page_repeat_time):
+          break
+    if pageset_time > float(options.pageset_repeat_time):
+      break

[dtu, 2013/07/10 23:15:41]

To do this more concisely:

pageset_start_time = time.time()
while time.time() - pageset_start_time < options.pageset_repeat_seconds:
   # Stuff.

[edmundyan, 2013/07/11 01:18:55]

Way nicer, thank you! It originally did a Do..While for when --page-repeat=0s we
would still run at least one iteration.  Should that be put back?

+
+
 def Run(test, page_set, options):
   """Runs a given test against a given page_set with the given options."""
   results = test.PrepareResults(options)
@@ -206,62 +298,14 @@ def Run(test, page_set, options):
   results_for_current_run = results
 
   try:
-    for page in pages:
-      if options.wpr_mode != wpr_modes.WPR_RECORD:
-        if page.archive_path and os.path.isfile(page.archive_path):
-          possible_browser.options.wpr_mode = wpr_modes.WPR_REPLAY
-        else:
-          possible_browser.options.wpr_mode = wpr_modes.WPR_OFF
-      results_for_current_run = results
-      if state.first_page and test.discard_first_result:
-        # If discarding results, substitute a dummy object.
-        results_for_current_run = type(results)()
-      results_for_current_run.StartTest(page)
-      tries = 3
-      while tries:
-        try:
-          state.StartBrowser(test, page_set, page, possible_browser,
-                             credentials_path, page.archive_path)
-
-          _WaitForThermalThrottlingIfNeeded(state.browser.platform)
-
-          if options.trace_dir:
-            state.StartTracing()
-          if options.profiler_dir:
-            state.StartProfiling(page, options)
-
-          try:
-            _RunPage(test, page, state.tab, results_for_current_run, options)
-            _CheckThermalThrottling(state.browser.platform)
-          except exceptions.TabCrashException:
-            stdout = ''
-            if not options.show_stdout:
-              stdout = state.browser.GetStandardOutput()
-              stdout = (('\nStandard Output:\n') +
-                        ('*' * 80) +
-                        '\n\t' + stdout.replace('\n', '\n\t') + '\n' +
-                        ('*' * 80))
-            logging.warning('Tab crashed: %s%s', page.url, stdout)
-            state.StopBrowser()
-
-          if options.trace_dir:
-            state.StopTracing(page, options)
-          if options.profiler_dir:
-            state.StopProfiling()
-
-          if test.NeedsBrowserRestartAfterEachRun(state.tab):
-            state.StopBrowser()
-
-          break
-        except exceptions.BrowserGoneException:
-          logging.warning('Lost connection to browser. Retrying.')
-          state.StopBrowser()
-          tries -= 1
-          if not tries:
-            logging.error('Lost connection to browser 3 times. Failing.')
-            raise
-      results_for_current_run.StopTest(page)
-    test.DidRunPageSet(state.tab, results_for_current_run)
+    test.WillRunTest(state.tab)
+    if options.page_repeat_time or options.pageset_repeat_time:
+      _RunTime(test, page_set, options, credentials_path, possible_browser,
+               results, state, pages)
+    else:
+      _RunIters(test, page_set, options, credentials_path, possible_browser,
+                results, state, pages)
+    test.DidRunTest(state.tab, results)
   finally:
     state.StopBrowser()
 
@@ -281,10 +325,8 @@ def _ShuffleAndFilterPageSet(page_set, options):
 
   if options.pageset_shuffle:
     random.Random().shuffle(pages)
-  return [page
-      for _ in xrange(int(options.pageset_repeat))
-      for page in pages
-      for _ in xrange(int(options.page_repeat))]
+
+  return pages
 
 
 def _CheckArchives(page_set, pages, results):