[bisect] - First pass bisect functional breakages.

tools/bisect-perf-regression.py

 #!/usr/bin/env python
 # Copyright (c) 2013 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.
 
 """Performance Test Bisect Tool
 
 This script bisects a series of changelists using binary search. It starts at
 a bad revision where a performance metric has regressed, and asks for a last
 known-good revision. It will then binary search across this revision range by
@@ skipping 159 matching lines @@
 # once build is produced, it reads SHA value from this file and appends it
 # to build archive filename.
 DEPS_SHA_PATCH = """diff --git src/DEPS.sha src/DEPS.sha
 new file mode 100644
 --- /dev/null
 +++ src/DEPS.sha
 @@ -0,0 +1 @@
 +%(deps_sha)s
 """
 
+BISECT_MODE_MEAN = 'mean'
+BISECT_MODE_STD_DEV = 'std_dev'
+BISECT_MODE_RETURN_CODE = 'return_code'
+
+
 def _AddAdditionalDepotInfo(depot_info):
   """Adds additional depot info to the global depot variables."""
   global DEPOT_DEPS_NAME
   global DEPOT_NAMES
   DEPOT_DEPS_NAME = dict(DEPOT_DEPS_NAME.items() +
       depot_info.items())
   DEPOT_NAMES = DEPOT_DEPS_NAME.keys()
 
 
 def CalculateTruncatedMean(data_set, truncate_percent):
@@ skipping 1771 matching lines @@
       path_to_generate = os.path.join('tools', 'perf', 'generate_profile')
 
       if arg_dict.has_key('--profile-dir') and arg_dict.has_key('--browser'):
         profile_path, profile_type = os.path.split(arg_dict['--profile-dir'])
         return not RunProcess(['python', path_to_generate,
             '--profile-type-to-generate', profile_type,
             '--browser', arg_dict['--browser'], '--output-dir', profile_path])
       return False
     return True
 
+  def _IsBisectModeUsingMetric(self):
+    return self.opts.bisect_mode in [BISECT_MODE_MEAN, BISECT_MODE_STD_DEV]
+
+  def _IsBisectModeReturnCode(self):
+    return self.opts.bisect_mode in [BISECT_MODE_RETURN_CODE]
+
+  def _IsBisectModeStandardDeviation(self):
+    return self.opts.bisect_mode in [BISECT_MODE_STD_DEV]
+
   def RunPerformanceTestAndParseResults(
       self, command_to_run, metric, reset_on_first_run=False,
       upload_on_last_run=False, results_label=None):
     """Runs a performance test on the current revision and parses the results.
 
     Args:
       command_to_run: The command to be run to execute the performance test.
       metric: The metric to parse out from the results of the performance test.
           This is the result chart name and trace name, separated by slash.
       reset_on_first_run: If True, pass the flag --reset-results on first run.
@@ skipping 45 matching lines @@
         current_args = copy.copy(args)
         if is_telemetry:
           if i == 0 and reset_on_first_run:
             current_args.append('--reset-results')
           elif i == self.opts.repeat_test_count - 1 and upload_on_last_run:
             current_args.append('--upload-results')
           if results_label:
             current_args.append('--results-label=%s' % results_label)
         (output, return_code) = RunProcessAndRetrieveOutput(current_args,
                                                             cwd=self.src_cwd)
+        output_of_all_runs += output

[tonyg, 2014/04/25 20:38:42]

I'm not following why this block moved into the try-except. Can you explain?

In general, code inside try blocks should be minimized.

[shatch, 2014/04/25 22:46:03]

Oops, leftover from earlier version. Also moved the if statements out, since you
mentioned it.

+        if self.opts.output_buildbot_annotations:
+          print output
+
+        if self._IsBisectModeUsingMetric():
+          metric_values += self.ParseMetricValuesFromOutput(metric, output)
+          # If we're bisecting on a metric (ie, changes in the mean or
+          # standard deviation) and no metric values are produced, bail out.
+          if not metric_values:
+            break
+        elif self._IsBisectModeReturnCode():
+          metric_values.append(return_code)
+
+        elapsed_minutes = (time.time() - start_time) / 60.0
+        if elapsed_minutes >= self.opts.max_time_minutes:
+          break
       except OSError, e:
         if e.errno == errno.ENOENT:
           err_text  = ('Something went wrong running the performance test. '
                        'Please review the command line:\n\n')
           if 'src/' in ' '.join(args):
             err_text += ('Check that you haven\'t accidentally specified a '
                          'path with src/ in the command.\n\n')
           err_text += ' '.join(args)
           err_text += '\n'
 
           return (err_text, failure_code)
         raise
 
-      output_of_all_runs += output
-      if self.opts.output_buildbot_annotations:
-        print output
-
-      metric_values += self.ParseMetricValuesFromOutput(metric, output)
-
-      elapsed_minutes = (time.time() - start_time) / 60.0
-
-      if elapsed_minutes >= self.opts.max_time_minutes or not metric_values:
-        break
-
     if len(metric_values) == 0:
       err_text = 'Metric %s was not found in the test output.' % metric
       # TODO(qyearsley): Consider also getting and displaying a list of metrics
       # that were found in the output here.
       return (err_text, failure_code, output_of_all_runs)
 
-    # Need to get the average value if there were multiple values.
-    truncated_mean = CalculateTruncatedMean(metric_values,
-        self.opts.truncate_percent)
-    standard_err = CalculateStandardError(metric_values)
-    standard_dev = CalculateStandardDeviation(metric_values)
+    # If we're bisecting on return codes, we're really just looking for zero vs
+    # non-zero.
+    if self._IsBisectModeReturnCode():
+      # If any of the return codes is non-zero, output 1.
+      overall_return_code = 0 if (
+          all(current_value == 0 for current_value in metric_values)) else 1
 
-    values = {
-      'mean': truncated_mean,
-      'std_err': standard_err,
-      'std_dev': standard_dev,
-      'values': metric_values,
-    }
+      values = {
+        'mean': overall_return_code,
+        'std_err': 0.0,
+        'std_dev': 0.0,
+        'values': metric_values,
+      }
 
-    print 'Results of performance test: %12f %12f' % (
-        truncated_mean, standard_err)
-    print
+      print 'Results of performance test: Command returned with %d' % (
+          overall_return_code)
+      print
+    else:
+      # Need to get the average value if there were multiple values.
+      truncated_mean = CalculateTruncatedMean(metric_values,
+          self.opts.truncate_percent)
+      standard_err = CalculateStandardError(metric_values)
+      standard_dev = CalculateStandardDeviation(metric_values)
+
+      if self._IsBisectModeStandardDeviation():
+        metric_values = [standard_dev]
+
+      values = {
+        'mean': truncated_mean,
+        'std_err': standard_err,
+        'std_dev': standard_dev,
+        'values': metric_values,
+      }
+
+      print 'Results of performance test: %12f %12f' % (
+          truncated_mean, standard_err)
+      print
     return (values, success_code, output_of_all_runs)
 
   def FindAllRevisionsToSync(self, revision, depot):
     """Finds all dependant revisions and depots that need to be synced for a
     given revision. This is only useful in the git workflow, as an svn depot
     may be split into multiple mirrors.
 
     ie. skia is broken up into 3 git mirrors over skia/src, skia/gyp, and
     skia/include. To sync skia/src properly, one has to find the proper
     revisions in skia/gyp and skia/include.
@@ skipping 240 matching lines @@
             return results
         else:
           return ('Failed to build revision: [%s]' % (str(revision, )),
               BUILD_RESULT_FAIL)
       else:
         return ('Failed to run [gclient runhooks].', BUILD_RESULT_FAIL)
     else:
       return ('Failed to sync revision: [%s]' % (str(revision, )),
           BUILD_RESULT_FAIL)
 
-  def CheckIfRunPassed(self, current_value, known_good_value, known_bad_value):
+  def _CheckIfRunPassed(self, current_value, known_good_value, known_bad_value):
     """Given known good and bad values, decide if the current_value passed
     or failed.
 
     Args:
       current_value: The value of the metric being checked.
       known_bad_value: The reference value for a "failed" run.
       known_good_value: The reference value for a "passed" run.
 
     Returns:
       True if the current_value is closer to the known_good_value than the
       known_bad_value.
     """
-    dist_to_good_value = abs(current_value['mean'] - known_good_value['mean'])
-    dist_to_bad_value = abs(current_value['mean'] - known_bad_value['mean'])
+    if self.opts.bisect_mode == BISECT_MODE_STD_DEV:
+      dist_to_good_value = abs(current_value['std_dev'] -
+          known_good_value['std_dev'])
+      dist_to_bad_value = abs(current_value['std_dev'] -
+          known_bad_value['std_dev'])
+    else:
+      dist_to_good_value = abs(current_value['mean'] - known_good_value['mean'])
+      dist_to_bad_value = abs(current_value['mean'] - known_bad_value['mean'])
 
     return dist_to_good_value < dist_to_bad_value
 
   def _GetDepotDirectory(self, depot_name):
     if depot_name == 'chromium':
       return self.src_cwd
     elif depot_name == 'cros':
       return self.cros_cwd
     elif depot_name in DEPOT_NAMES:
       return self.depot_cwd[depot_name]
@@ skipping 535 matching lines @@
                                                    metric, skippable=True)
 
         # If the build is successful, check whether or not the metric
         # had regressed.
         if not run_results[1]:
           if len(run_results) > 2:
             next_revision_data['external'] = run_results[2]
             next_revision_data['perf_time'] = run_results[3]
             next_revision_data['build_time'] = run_results[4]
 
-          passed_regression = self.CheckIfRunPassed(run_results[0],
-                                                    known_good_value,
-                                                    known_bad_value)
+          passed_regression = self._CheckIfRunPassed(run_results[0],
+                                                     known_good_value,
+                                                     known_bad_value)
 
           next_revision_data['passed'] = passed_regression
           next_revision_data['value'] = run_results[0]
 
           if passed_regression:
             max_revision = next_revision_index
           else:
             min_revision = next_revision_index
         else:
           if run_results[1] == BUILD_RESULT_SKIPPED:
@@ skipping 34 matching lines @@
     # "Confidence in Bisection Results: 100%" to decide whether or not
     # to cc the author(s). If you change this, please update the perf
     # dashboard as well.
     print 'Confidence in Bisection Results: %d%%' % results_dict['confidence']
 
   def _PrintBanner(self, results_dict):
     print
     print " __o_\___          Aw Snap! We hit a speed bump!"
     print "=-O----O-'__.~.___________________________________"
     print
-    print 'Bisect reproduced a %.02f%% (+-%.02f%%) change in the %s metric.' % (
-        results_dict['regression_size'], results_dict['regression_std_err'],
-        '/'.join(self.opts.metric))
+    if self._IsBisectModeReturnCode():
+      print ('Bisect reproduced a change in return codes while running the '
+          'performance test.')
+    else:
+      print ('Bisect reproduced a %.02f%% (+-%.02f%%) change in the '
+          '%s metric.' % (results_dict['regression_size'],
+          results_dict['regression_std_err'], '/'.join(self.opts.metric)))
     self._PrintConfidence(results_dict)
 
   def _PrintFailedBanner(self, results_dict):
     print
-    print ('Bisect could not reproduce a change in the '
-        '%s/%s metric.' % (self.opts.metric[0], self.opts.metric[1]))
+    if self._IsBisectModeReturnCode():
+      print 'Bisect could not reproduce a change in the return code.'
+    else:
+      print ('Bisect could not reproduce a change in the '
+          '%s metric.' % '/'.join(self.opts.metric))
     print
     self._PrintConfidence(results_dict)

[tonyg, 2014/04/25 20:38:42]

Looking at the output in the CL description, it seems like we should omit the
confidence when we indicate that we failed to repro. Because the 0% confidence
kind of contradicts the previous statement.

[shatch, 2014/04/25 22:46:03]

Done.

 
   def _GetViewVCLinkFromDepotAndHash(self, cl, depot):
     info = self.source_control.QueryRevisionInfo(cl,
         self._GetDepotDirectory(depot))
     if depot and DEPOT_DEPS_NAME[depot].has_key('viewvc'):
       try:
         # Format is "git-svn-id: svn://....@123456 <other data>"
         svn_line = [i for i in info['body'].splitlines() if 'git-svn-id:' in i]
         svn_revision = svn_line[0].split('@')
         svn_revision = svn_revision[1].split(' ')[0]
@@ skipping 16 matching lines @@
       print 'Link    : %s' % commit_link
     else:
       print
       print 'Failed to parse svn revision from body:'
       print
       print info['body']
       print
     print 'Commit  : %s' % cl
     print 'Date    : %s' % info['date']
 
+  def _PrintTestedCommitsHeader(self):

[tonyg, 2014/04/25 20:38:42]

These are now fairly hard to read and a little redundant. Do you think a method
something like this would help?

def PrintTable(column_widths, row_data):
  ...

PrintTable([20, 70, 12, 14, 13],
           [['Depot', 'Commit SHA', ...],
            [current_data['depot'], ...],
            ...])

[shatch, 2014/04/25 22:46:03]

Yeah, looks a lot more readable this way.

+    if self.opts.bisect_mode == BISECT_MODE_MEAN:
+      print '  %20s  %70s  %12s %14s %13s' % ('Depot'.center(20, ' '),
+          'Commit SHA'.center(70, ' '), 'Mean'.center(12, ' '),
+          'Std. Error'.center(14, ' '), 'State'.center(13, ' '))
+    elif self.opts.bisect_mode == BISECT_MODE_STD_DEV:
+      print '  %20s  %70s  %14s %12s %13s' % ('Depot'.center(20, ' '),
+          'Commit SHA'.center(70, ' '), 'Std. Dev'.center(14, ' '),
+           'Mean'.center(12, ' '), 'State'.center(13, ' '))
+    elif self.opts.bisect_mode == BISECT_MODE_RETURN_CODE:
+      print '  %20s  %70s  %14s %13s' % ('Depot'.center(20, ' '),
+          'Commit SHA'.center(70, ' '), 'Return Code'.center(14, ' '),
+           'State'.center(13, ' '))
+
+  def _PrintTestedCommitsEntry(self, current_data, cl_link, state_str):
+    if self.opts.bisect_mode == BISECT_MODE_MEAN:
+      std_error = ('+-%.02f' %
+          current_data['value']['std_err']).center(14, ' ')
+      mean = ('%.02f' % current_data['value']['mean']).center(12, ' ')
+      print '  %20s  %70s  %12s %14s %13s' % (
+          current_data['depot'].center(20, ' '), cl_link.center(70, ' '),
+          mean, std_error, state_str)
+    elif self.opts.bisect_mode == BISECT_MODE_STD_DEV:
+      std_dev = ('+-%.02f' %
+          current_data['value']['std_dev']).center(14, ' ')
+      mean = ('%.02f' % current_data['value']['mean']).center(12, ' ')
+      print '  %20s  %70s  %14s %12s %13s' % (
+          current_data['depot'].center(20, ' '), cl_link.center(70, ' '),
+          std_dev, mean, state_str)
+    elif self.opts.bisect_mode == BISECT_MODE_RETURN_CODE:
+      mean = ('%d' % current_data['value']['mean']).center(12, ' ')
+      print '  %20s  %70s  %14s %13s' % (
+          current_data['depot'].center(20, ' '), cl_link.center(70, ' '),
+          mean, state_str)
+
   def _PrintTestedCommitsTable(self, revision_data_sorted,
       first_working_revision, last_broken_revision, confidence,
       final_step=True):
     print
     if final_step:
       print 'Tested commits:'
     else:
       print 'Partial results:'
-    print '  %20s  %70s  %12s %14s %13s' % ('Depot'.center(20, ' '),
-        'Commit SHA'.center(70, ' '), 'Mean'.center(12, ' '),
-        'Std. Error'.center(14, ' '), 'State'.center(13, ' '))
+    self._PrintTestedCommitsHeader()
     state = 0
     for current_id, current_data in revision_data_sorted:
       if current_data['value']:
         if (current_id == last_broken_revision or
             current_id == first_working_revision):
           # If confidence is too low, don't add this empty line since it's
           # used to put focus on a suspected CL.
           if confidence and final_step:
             print
           state += 1
           if state == 2 and not final_step:
             # Just want a separation between "bad" and "good" cl's.
             print
 
         state_str = 'Bad'
         if state == 1 and final_step:
           state_str = 'Suspected CL'
         elif state == 2:
           state_str = 'Good'
 
         # If confidence is too low, don't bother outputting good/bad.
         if not confidence:
           state_str = ''
         state_str = state_str.center(13, ' ')
 
-        std_error = ('+-%.02f' %
-            current_data['value']['std_err']).center(14, ' ')
-        mean = ('%.02f' % current_data['value']['mean']).center(12, ' ')
         cl_link = self._GetViewVCLinkFromDepotAndHash(current_id,
             current_data['depot'])
         if not cl_link:
           cl_link = current_id
-        print '  %20s  %70s  %12s %14s %13s' % (
-            current_data['depot'].center(20, ' '), cl_link.center(70, ' '),
-            mean, std_error, state_str)
+        self._PrintTestedCommitsEntry(current_data, cl_link, state_str)
 
   def _PrintReproSteps(self):
     print
     print 'To reproduce locally:'
     print '$ ' + self.opts.command
     if bisect_utils.IsTelemetryCommand(self.opts.command):
       print
       print 'Also consider passing --profiler=list to see available profilers.'
 
   def _PrintOtherRegressions(self, other_regressions, revision_data):
@@ skipping 354 matching lines @@
     self.no_custom_deps = False
     self.working_directory = None
     self.extra_src = None
     self.debug_ignore_build = None
     self.debug_ignore_sync = None
     self.debug_ignore_perf_test = None
     self.gs_bucket = None
     self.target_arch = 'ia32'
     self.builder_host = None
     self.builder_port = None
+    self.bisect_mode = BISECT_MODE_MEAN
 
   def _CreateCommandLineParser(self):
     """Creates a parser with bisect options.
 
     Returns:
       An instance of optparse.OptionParser.
     """
     usage = ('%prog [options] [-- chromium-options]\n'
              'Perform binary search on revision history to find a minimal '
              'range of revisions where a peformance metric regressed.\n')
@@ skipping 34 matching lines @@
                      'doesn\'t exceed --max_time_minutes. Values will be '
                      'clamped to range [1, 60].'
                      'Default value is 20.')
     group.add_option('-t', '--truncate_percent',
                      type='int',
                      default=25,
                      help='The highest/lowest % are discarded to form a '
                      'truncated mean. Values will be clamped to range [0, '
                      '25]. Default value is 25 (highest/lowest 25% will be '
                      'discarded).')
+    group.add_option('--bisect_mode',
+                     type='choice',
+                     choices=[BISECT_MODE_MEAN, BISECT_MODE_STD_DEV,
+                        BISECT_MODE_RETURN_CODE],
+                     default=BISECT_MODE_MEAN,
+                     help='The bisect mode. Choices are to bisect on the '
+                     'difference in mean, std_dev, or return_code.')
     parser.add_option_group(group)
 
     group = optparse.OptionGroup(parser, 'Build options')
     group.add_option('-w', '--working_directory',
                      type='str',
                      help='Path to the working directory where the script '
                      'will do an initial checkout of the chromium depot. The '
                      'files will be placed in a subdirectory "bisect" under '
                      'working_directory and that will be used to perform the '
                      'bisection. This parameter is optional, if it is not '
@@ skipping 79 matching lines @@
     try:
       if not opts.command:
         raise RuntimeError('missing required parameter: --command')
 
       if not opts.good_revision:
         raise RuntimeError('missing required parameter: --good_revision')
 
       if not opts.bad_revision:
         raise RuntimeError('missing required parameter: --bad_revision')
 
-      if not opts.metric:
+      if not opts.metric and opts.bisect_mode != BISECT_MODE_RETURN_CODE:
         raise RuntimeError('missing required parameter: --metric')
 
       if opts.gs_bucket:
         if not cloud_storage.List(opts.gs_bucket):
           raise RuntimeError('Invalid Google Storage: gs://%s' % opts.gs_bucket)
         if not opts.builder_host:
           raise RuntimeError('Must specify try server hostname, when '
                              'gs_bucket is used: --builder_host')
         if not opts.builder_port:
           raise RuntimeError('Must specify try server port number, when '
                              'gs_bucket is used: --builder_port')
       if opts.target_platform == 'cros':
         # Run sudo up front to make sure credentials are cached for later.
         print 'Sudo is required to build cros:'
         print
         RunProcess(['sudo', 'true'])
 
         if not opts.cros_board:
           raise RuntimeError('missing required parameter: --cros_board')
 
         if not opts.cros_remote_ip:
           raise RuntimeError('missing required parameter: --cros_remote_ip')
 
         if not opts.working_directory:
           raise RuntimeError('missing required parameter: --working_directory')
 
       metric_values = opts.metric.split('/')
-      if len(metric_values) != 2:
+      if (len(metric_values) != 2 and
+          opts.bisect_mode != BISECT_MODE_RETURN_CODE):
         raise RuntimeError("Invalid metric specified: [%s]" % opts.metric)
 
       opts.metric = metric_values
       opts.repeat_test_count = min(max(opts.repeat_test_count, 1), 100)
       opts.max_time_minutes = min(max(opts.max_time_minutes, 1), 60)
       opts.truncate_percent = min(max(opts.truncate_percent, 0), 25)
       opts.truncate_percent = opts.truncate_percent / 100.0
 
       for k, v in opts.__dict__.iteritems():
         assert hasattr(self, k), "Invalid %s attribute in BisectOptions." % k
@@ skipping 91 matching lines @@
       # The perf dashboard scrapes the "results" step in order to comment on
       # bugs. If you change this, please update the perf dashboard as well.
       bisect_utils.OutputAnnotationStepStart('Results')
     print 'Error: %s' % e.message
     if opts.output_buildbot_annotations:
       bisect_utils.OutputAnnotationStepClosed()
   return 1
 
 if __name__ == '__main__':
   sys.exit(main())