Refactor mojo_benchmark to get rid of warm/cold start special cases.

mojo/devtools/common/mojo_benchmark

 #!/usr/bin/env python
 # Copyright 2015 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """Runner for Mojo application benchmarks."""
 
 import argparse
 import logging
 import sys
@@ skipping 32 matching lines @@
 ['android', 'linux'], indicating the system on which the benchmarks are to be
 run.
 """
 
 _logger = logging.getLogger()
 
 _BENCHMARK_APP = 'https://core.mojoapps.io/benchmark.mojo'
 _CACHE_SERVICE_URL = 'mojo:url_response_disk_cache'
 _NETWORK_SERVICE_URL = 'mojo:network_service'
 
+_COLD_START_SHELL_ARGS = [
+  '--args-for=%s %s' % (_CACHE_SERVICE_URL, '--clear'),
+  '--args-for=%s %s' % (_NETWORK_SERVICE_URL, '--clear'),
+]
+
 # Additional time in seconds allocated per shell run to accommodate start-up.
 # The shell should terminate before hitting this time out, it is an error if it
 # doesn't.
 _EXTRA_TIMEOUT = 20
 
 
-def _get_output_file(shell, name, cold_start):
-    file_name = 'benchmark-%s-%s-%s.trace' % (
-        name.replace(' ', '_'),
-        'cold_start' if cold_start else 'warm_start',
-        time.strftime('%Y%m%d%H%M%S'))
-    return file_name
+def _rewrite_benchmark_list(benchmark_list):
+  """Rewrites the specification of benchmarks to run, yielding two benchmarks
+  (one for cold and one for warm start) for each benchmark defined in the input
+  list.
+  """
+  result = list()

[viettrungluu, 2015/10/14 21:24:16]

Probably "= []" would look more normal.

[ppi, 2015/10/14 21:47:27]

Done.

+  for benchmark_spec in benchmark_list:

[viettrungluu, 2015/10/14 21:24:16]

I wonder if you shouldn't have the function be: _generate_benchmark_variants(),
and do below:

benchmark_list = []
for benchmark in benchmark_list_params['benchmarks']:
  benchmark += _generate_benchmark_variants(benchmark)

[ppi, 2015/10/14 21:47:27]

Sg, done.

+    # Cold start.
+    result.append({
+      'name': benchmark_spec['name'] + ' (cold start)',
+      'app': benchmark_spec['app'],
+      'duration': benchmark_spec['duration'],
+      'measurements': benchmark_spec['measurements'],
+      'shell-args': benchmark_spec.get('shell-args',
+                                       []) + _COLD_START_SHELL_ARGS})
+    # Warm start.
+    result.append({
+      'name': benchmark_spec['name'] + ' (warm start)',
+      'app': benchmark_spec['app'],
+      'duration': benchmark_spec['duration'],
+      'measurements': benchmark_spec['measurements'],
+      'shell-args': benchmark_spec.get('shell-args', [])})
+  return result
 
 
 def _run_benchmark(shell, shell_args, name, app, duration_seconds, measurements,
-                   cold_start, verbose, android, save_traces):
+                   verbose, android, save_traces):
   """Runs `benchmark.mojo` in shell with correct arguments, parses and
   presents the benchmark results.
   """
   timeout = duration_seconds + _EXTRA_TIMEOUT
   benchmark_args = []
   benchmark_args.append('--app=' + app)
   benchmark_args.append('--duration=' + str(duration_seconds))
 
   output_file = None
   device_output_file = None
   if save_traces:
-    output_file = _get_output_file(shell, name, cold_start)
+    output_file = 'benchmark-%s-%s.trace' % (name.replace(' ', '_'),
+        time.strftime('%Y%m%d%H%M%S'))
     if android:
       device_output_file = os.path.join(shell.get_tmp_dir_path(), output_file)
       benchmark_args.append('--trace-output=' + device_output_file)
     else:
       benchmark_args.append('--trace-output=' + output_file)
 
   for measurement in measurements:
     benchmark_args.append(measurement)
 
   shell_args = list(shell_args)
   shell_args.append(_BENCHMARK_APP)
   shell_args.append('--force-offline-by-default')
   shell_args.append('--args-for=%s %s' % (_BENCHMARK_APP,
                                           ' '.join(benchmark_args)))
 
-  if cold_start:
-    shell_args.append('--args-for=%s %s' % (_CACHE_SERVICE_URL, '--clear'))
-    shell_args.append('--args-for=%s %s' % (_NETWORK_SERVICE_URL, '--clear'))
-
   if verbose:
     print 'shell arguments: ' + str(shell_args)
-  print '[ %s ] %s' % (name, 'cold start' if cold_start else 'warm start')
+  print '[ %s ]' % name
   return_code, output, did_time_out = shell.run_and_get_output(
       shell_args, timeout=timeout)
   output_lines = [line.strip() for line in output.split('\n')]
 
   if return_code or did_time_out or 'benchmark succeeded' not in output_lines:
     print 'timed out' if did_time_out else 'failed'
     if return_code:
       print 'Return code: ' + str(return_code)
     print 'Output: '
     print output
@@ skipping 26 matching lines @@
 
   try:
     shell, common_shell_args = shell_arguments.get_shell(config, [])
   except shell_arguments.ShellConfigurationException as e:
     print e
     return 1
 
   target_os = 'android' if script_args.android else 'linux'
   benchmark_list_params = {"target_os": target_os}
   exec script_args.benchmark_list_file in benchmark_list_params
-  benchmark_list = benchmark_list_params['benchmarks']
+  benchmark_list = _rewrite_benchmark_list(benchmark_list_params['benchmarks'])
 
   succeeded = True
   for benchmark_spec in benchmark_list:
     name = benchmark_spec['name']
     app = benchmark_spec['app']
     duration = benchmark_spec['duration']
     shell_args = benchmark_spec.get('shell-args', []) + common_shell_args
     measurements = benchmark_spec['measurements']
     _run_benchmark(shell, shell_args, name, app, duration, measurements,
-                   cold_start=True, verbose=script_args.verbose,
-                   android=script_args.android,
-                   save_traces=script_args.save_traces)
-    _run_benchmark(shell, shell_args, name, app, duration, measurements,
-                   cold_start=False, verbose=script_args.verbose,
-                   android=script_args.android,
+                   verbose=script_args.verbose, android=script_args.android,

[viettrungluu, 2015/10/14 21:24:16]

I find it odd that you give these things as keyword arguments, when they're
required positional arguments (though this is a pre-existing defect).

[ppi, 2015/10/14 21:47:27]

Done.

                    save_traces=script_args.save_traces)
 
   return 0 if succeeded else 1
 
 if __name__ == '__main__':
   sys.exit(main())