First pass on tool to bisect across range of revisions to help narrow down where a regression in a …

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
 syncing, building, and running a performance test. If the change is
 suspected to occur as a result of WebKit/Skia/V8 changes, the script will
 further bisect changes to those depots and attempt to narrow down the revision
 range.
+
+
+An example usage:
+
+./tools/bisect-perf-regression.py -c\
+"out/Release/performance_ui_tests --gtest_filter=ShutdownTest.SimpleUserQuit"\
+-g 1f6e67861535121c5c819c16a666f2436c207e7b\
+-b b732f23b4f81c382db0b23b9035f3dadc7d925bb\

[tonyg, 2013/01/30 18:26:28]

Recommend putting svn revisions in the example since that will be the common use
case.

[shatch, 2013/01/30 19:50:29]

Done.

+-m shutdown/simple-user-quit
+
 """
 
-DEPOT_WEBKIT = 'webkit'
-DEPOT_SKIA = 'skia'
-DEPOT_V8 = 'v8'
-DEPOT_NAMES = [DEPOT_WEBKIT, DEPOT_SKIA, DEPOT_V8]
-DEPOT_DEPS_NAME = { DEPOT_WEBKIT : "src/third_party/WebKit",
-                    DEPOT_SKIA : 'src/third_party/skia/src',
-                    DEPOT_V8 : 'src/v8' }
-
-DEPOT_PATHS_FROM_SRC = { DEPOT_WEBKIT : '/third_party/WebKit',
-                         DEPOT_SKIA : '/third_party/skia/src',
-                         DEPOT_V8 : '/v8' }
-
-FILE_DEPS_GIT = '.DEPS.git'
-
-SUPPORTED_OS_TYPES = ['posix']
-
-ERROR_MESSAGE_OS_NOT_SUPPORTED = "Sorry, this platform isn't supported yet."
-
-ERROR_MESSAGE_SVN_UNSUPPORTED = "Sorry, only the git workflow is supported"\
-                                " at the moment."
-
-ERROR_MESSAGE_GCLIENT_NOT_FOUND = "An error occurred trying to read .gclient"\
-                                  " file. Check that you are running the tool"\
-                                  " from $chromium/src"
-
-ERROR_MESSAGE_IMPORT_GCLIENT = "An error occurred while importing .gclient"\
-                               " file."
-
-ERROR_MISSING_PARAMETER = 'Error: missing required parameter:'
-ERROR_MISSING_COMMAND = ERROR_MISSING_PARAMETER + ' --command'
-ERROR_MISSING_GOOD_REVISION = ERROR_MISSING_PARAMETER + ' --good_revision'
-ERROR_MISSING_BAD_REVISION = ERROR_MISSING_PARAMETER + ' --bad_revision'
-ERROR_MISSING_METRIC = ERROR_MISSING_PARAMETER + ' --metric'
-
-ERROR_RETRIEVE_REVISION_RANGE = 'An error occurred attempting to retrieve'\
-                                ' revision range: [%s..%s]'
-ERROR_RETRIEVE_REVISION = 'An error occurred attempting to retrieve revision:'\
-                          '[%s]'
-
-ERROR_PERF_TEST_NO_VALUES = 'No values returned from performance test.'
-ERROR_PERF_TEST_FAILED_RUN = 'Failed to run performance test.'
-
-ERROR_FAILED_DEPS_PARSE = 'Failed to parse DEPS file for WebKit revision.'
-ERROR_FAILED_BUILD = 'Failed to build revision: [%s]'
-ERROR_FAILED_GCLIENT_RUNHOOKS = 'Failed to run [gclient runhooks].'
-ERROR_FAILED_SYNC = 'Failed to sync revision: [%s]'
-ERROR_INCORRECT_BRANCH = "You must switch to master branch to run bisection."
-
-MESSAGE_REGRESSION_DEPOT_METRIC = 'Regression in metric:%s appears to be the'\
-                                   ' result of changes in [%s].'
-MESSAGE_BISECT_DEPOT = 'Revisions to bisect on [%s]:'
-MESSAGE_BISECT_SRC ='Revisions to bisect on chromium/src:'
-MESSAGE_GATHERING_REFERENCE = 'Gathering reference values for bisection.'
-MESSAGE_GATHERING_REVISIONS = 'Gathering revision range for bisection.'
-MESSAGE_WORK_ON_REVISION = 'Working on revision: [%s]'
-
-
-###############################################################################
 
 import re
 import os
 import imp
 import sys
 import shlex
 import optparse
 import subprocess
 
 
+DEPOT_DEPS_NAME = { 'webkit' : "src/third_party/WebKit",
+                    'skia' : 'src/third_party/skia/src',
+                    'v8' : 'src/v8' }
+DEPOT_NAMES = DEPOT_DEPS_NAME.keys()
 
-GLOBAL_BISECT_OPTIONS = {'build' : False,
-                         'sync' : False,
-                         'perf' : False,
-                         'goma' : False}
-
-def SetGlobalGomaFlag(flag):
-  """Sets global flag for using goma with builds."""
-  global GLOBAL_BISECT_OPTIONS
-
-  GLOBAL_BISECT_OPTIONS['goma'] = flag
+FILE_DEPS_GIT = '.DEPS.git'
 
 
-def IsGlobalGomaFlagEnabled():
-  global GLOBAL_BISECT_OPTIONS
+###############################################################################

[tonyg, 2013/01/30 18:26:28]

Now that most of the comments are gone, this bar isn't really as necessary

[shatch, 2013/01/30 19:50:29]

Done.

 
-  return GLOBAL_BISECT_OPTIONS['goma']
-
-
-def SetDebugIgnoreFlags(build_flag, sync_flag, perf_flag):
-  """Sets global flags for ignoring builds, syncs, and perf tests."""
-  global GLOBAL_BISECT_OPTIONS
-
-  GLOBAL_BISECT_OPTIONS['build'] = build_flag
-  GLOBAL_BISECT_OPTIONS['sync'] = sync_flag
-  GLOBAL_BISECT_OPTIONS['perf'] = perf_flag
-
-
-def IsDebugIgnoreBuildEnabled():
-  """Returns whether build commands are being ignored."""
-  global GLOBAL_BISECT_OPTIONS
-
-  return GLOBAL_BISECT_OPTIONS['build']
-
-
-def IsDebugIgnoreSyncEnabled():
-  """Returns whether sync commands are being ignored."""
-  global GLOBAL_BISECT_OPTIONS
-
-  return GLOBAL_BISECT_OPTIONS['sync']
-
-
-def IsDebugIgnorePerfTestEnabled():
-  """Returns whether performance test commands are being ignored."""
-  global GLOBAL_BISECT_OPTIONS
-
-  return GLOBAL_BISECT_OPTIONS['perf']
 
 
 def IsStringFloat(string_to_check):
   """Checks whether or not the given string can be converted to a floating
-  point number."""
+  point number.
+
+  Args:
+    string_to_check: Input string to check if it can be converted to a float.
+
+  Returns:
+    True if the string can be converted to a float.
+  """
   try:
     float(string_to_check)
 
-    result = True
+    return True
   except ValueError:
-    result = False
-
-  return result
+    return False
 
 
 def IsStringInt(string_to_check):
-  """Checks whether or not the given string can be converted to a integer."""
+  """Checks whether or not the given string can be converted to a integer.
+
+  Args:
+    string_to_check: Input string to check if it can be converted to an int.
+
+  Returns:
+    True if the string can be converted to an int.
+  """
   try:
     int(string_to_check)
 
-    result = True
+    return True
   except ValueError:
-    result = False
-
-  return result
+    return False
 
 
 def RunProcess(command):
-  """Run an arbitrary command, returning its output and return code."""
+  """Run an arbitrary command, returning its output and return code.
+
+  Args:
+    command: A list containing the command and args to execute.
+
+  Returns:
+    A tuple of the output and return code.
+  """
   # On Windows, use shell=True to get PATH interpretation.
   shell = (os.name == 'nt')
   proc = subprocess.Popen(command,
                           shell=shell,
                           stdout=subprocess.PIPE,
                           stderr=subprocess.PIPE)
   out = proc.communicate()[0]
 
   return (out, proc.returncode)
 
 
+
 class SourceControl(object):
   """SourceControl is an abstraction over the underlying source control
   system used for chromium. For now only git is supported, but in the
   future, the svn workflow could be added as well."""
   def __init__(self):
     super(SourceControl, self).__init__()
 
   def SyncToRevisionWithGClient(self, revision):
-    """Uses gclient to sync to the specified revision."""
+    """Uses gclient to sync to the specified revision.
+
+    ie. gclient sync --revision <revision>
+
+    Args:
+      revision: The git SHA1 or svn CL (depending on workflow).
+
+    Returns:
+      A tuple of the output and return code.
+    """
     args = ['gclient', 'sync', '--revision', revision]
 
     return RunProcess(args)
 
 
 class GitSourceControl(SourceControl):
   """GitSourceControl is used to query the underlying source control. """
   def __init__(self):
     super(GitSourceControl, self).__init__()
 
   def RunGit(self, command):
-    """Run a git subcommand, returning its output and return code."""
+    """Run a git subcommand, returning its output and return code.
+
+    Args:
+      command: A list containing the args to git.
+
+    Returns:
+      A tuple of the output and return code.
+    """
     command = ['git'] + command
 
     return RunProcess(command)
 
   def GetRevisionList(self, revision_range_end, revision_range_start):
-    """Retrieves a list of revisions between the bad revision and last known
-    good revision."""
+    """Retrieves a list of revisions between |revision_range_start| and
+    |revision_range_end|.
+
+    Args:
+      revision_range_end: The SHA1 for the end of the range.
+      revision_range_start: The SHA1 for the beginning of the range.
+
+    Returns:
+      A list of the revisions between |revision_range_start| and
+      |revision_range_end| (inclusive).
+    """
     revision_range = '%s..%s' % (revision_range_start, revision_range_end)
     (log_output, return_code) = self.RunGit(['log',
                                              '--format=%H',
                                              '-10000',
                                              '--first-parent',
                                              revision_range])
 
+    assert not return_code, 'An error occurred while running'\
+                            ' "git log --format=%%H -10000'\
+                            ' --first-parent %s' % (svn_pattern,)

[tonyg, 2013/01/30 18:26:28]

To avoid repeating the command here, I recommend extracting out a cmd list and
using it here.
cmd = ['log', ...]
self.RunGit(cmd)
'An error occurred while running %s' % ' '.join(cmd)

ditto in other assertions.

It also makes me wonder whether the assertion should just be moved up into
RunGit().

[shatch, 2013/01/30 19:50:29]

Done.

+
     revision_hash_list = log_output.split()
     revision_hash_list.append(revision_range_start)
 
     return revision_hash_list
 
   def SyncToRevision(self, revision, use_gclient=True):
-    """Syncs to the specified revision."""
+    """Syncs to the specified revision.
 
-    if IsDebugIgnoreSyncEnabled():
-      return True
+    Args:
+      revision: The revision to sync to.
+      use_gclient: Specifies whether or not we should sync using gclient or
+        just use source control directly.
+
+    Returns:
+      True if successful.
+    """
 
     if use_gclient:
       results = self.SyncToRevisionWithGClient(revision)
     else:
       results = self.RunGit(['checkout', revision])
 
-    return results[1] == 0
+    return not results[1]
 
   def ResolveToRevision(self, revision_to_check):
     """If the user supplied an SVN revision, try to resolve it to a

[tonyg, 2013/01/30 18:26:28]

Probably shouldn't mention the user in this function level comment

[shatch, 2013/01/30 19:50:29]

Done.

-    git SHA1."""
-    if not(IsStringInt(revision_to_check)):
+    git SHA1.
+
+    Args:
+      revision_to_check: The user supplied revision string that may need to be
+        resolved to a git SHA1.
+
+    Returns:
+      A string containing a git SHA1 hash, otherwise None.
+    """
+    if not IsStringInt(revision_to_check):
       return revision_to_check
 
     svn_pattern = 'SVN changes up to revision ' + revision_to_check
 
     (log_output, return_code) = self.RunGit(['log',
                                              '--format=%H',
                                              '-1',
                                              '--grep',
                                              svn_pattern,
                                              'origin/master'])
 
+    assert not return_code, 'An error occurred while running'\
+                            ' "git log --format=%%H -1'\
+                            ' --grep \'%s\' origin/master"' % (svn_pattern,)
+
     revision_hash_list = log_output.split()
 
-    if len(revision_hash_list):
+    if revision_hash_list:
       return revision_hash_list[0]
 
     return None
 
   def IsInProperBranch(self):
     """Confirms they're in the master branch for performing the bisection.
-    This is needed or gclient will fail to sync properly."""
+    This is needed or gclient will fail to sync properly.
+
+    Returns:
+      True if the current branch on src is 'master'
+    """
     (log_output, return_code) = self.RunGit(['rev-parse',
                                              '--abbrev-ref',
                                              'HEAD'])
 
+    assert not return_code, 'An error occurred while running'\
+                            ' \'git rev-parse --abbrev-ref HEAD\''
+
     log_output = log_output.strip()
 
     return log_output == "master"
 
 
 
 class BisectPerformanceMetrics(object):
   """BisectPerformanceMetrics performs a bisection against a list of range
   of revisions to narrow down where performance regressions may have
   occurred."""
 
-  def __init__(self, source_control):
+  def __init__(self, source_control, opts):
     super(BisectPerformanceMetrics, self).__init__()
 
+    self.opts = opts
     self.source_control = source_control
     self.src_cwd = os.getcwd()
     self.depot_cwd = {}
 
     for d in DEPOT_NAMES:
-      self.depot_cwd[d] = self.src_cwd + DEPOT_PATHS_FROM_SRC[d]
+      self.depot_cwd[d] = self.src_cwd + DEPOT_DEPS_NAME[d][3:]

[tonyg, 2013/01/30 18:26:28]

Perhaps a brief comment about the [3:]. Maybe # skip 'src'

[shatch, 2013/01/30 19:50:29]

Done.

 
   def GetRevisionList(self, bad_revision, good_revision):
     """Retrieves a list of all the commits between the bad revision and
     last known good revision."""
 
     revision_work_list = self.source_control.GetRevisionList(bad_revision,
                                                              good_revision)
 
     return revision_work_list
 
   def Get3rdPartyRevisionsFromCurrentRevision(self):
-    """Parses the DEPS file to determine WebKit/skia/v8/etc... versions."""
+    """Parses the DEPS file to determine WebKit/skia/v8/etc... versions.
+
+    Returns:
+      A dict in the format {depot:revision} if successful, otherwise None.
+    """
 
     cwd = os.getcwd()
     os.chdir(self.src_cwd)
 
     locals = {'Var': lambda _: locals["vars"][_],
               'From': lambda *args: None}
     execfile(FILE_DEPS_GIT, {}, locals)
 
     os.chdir(cwd)
 
     results = {}
 
-    rxp = ".git@(?P<revision>[a-zA-Z0-9]+)"
-    rxp = re.compile(rxp)
+    rxp = re.compile(".git@(?P<revision>[a-zA-Z0-9]+)")

[tonyg, 2013/01/30 18:26:28]

Is it worth being more specific and using a-fA-F instead of a-zA-Z

[shatch, 2013/01/30 19:50:29]

Done.

 
     for d in DEPOT_NAMES:
       if locals['deps'].has_key(DEPOT_DEPS_NAME[d]):
         re_results = rxp.search(locals['deps'][DEPOT_DEPS_NAME[d]])
 
         if re_results:
           results[d] = re_results.group('revision')
         else:
           return None
       else:
         return None
 
     return results
 
   def BuildCurrentRevision(self):
-    """Builds chrome and the performance_uit_tests suite on the current
-    revision."""
+    """Builds chrome and performance_ui_tests on the current revision.
 
-    if IsDebugIgnoreBuildEnabled():
+    Returns:
+      True if the build was successful.
+    """
+
+    if self.opts.debug_ignore_build:
       return True
 
     gyp_var = os.getenv('GYP_GENERATORS')
 
     num_threads = 16
 
-    if IsGlobalGomaFlagEnabled():
-      num_threads = 200
+    if self.opts.use_goma:
+      num_threads = 100
 
     if gyp_var != None and 'ninja' in gyp_var:
       args = ['ninja',
               '-C',
               'out/Release',
               '-j%d' % num_threads,
               'chrome',
               'performance_ui_tests']
     else:
       args = ['make',
               'BUILDTYPE=Release',
               '-j%d' % num_threads,
               'chrome',
               'performance_ui_tests']
 
     cwd = os.getcwd()
     os.chdir(self.src_cwd)
 
     (output, return_code) = RunProcess(args)
 
     os.chdir(cwd)
 
-    #print('build exited with: %d' % (return_code))
-    return return_code == 0
+    return not return_code
 
   def RunGClientHooks(self):
-    """Runs gclient with runhooks command."""
+    """Runs gclient with runhooks command.
 
-    if IsDebugIgnoreBuildEnabled():
+    Returns:
+      True if gclient reports no errors.
+    """
+
+    if self.opts.debug_ignore_build:
       return True
 
     results = RunProcess(['gclient', 'runhooks'])
 
-    return results[1] == 0
+    return not results[1]
 
   def ParseMetricValuesFromOutput(self, metric, text):
     """Parses output from performance_ui_tests and retrieves the results for
-    a given metric."""
+    a given metric.
+
+    Args:
+      metric: The metric as a list of [<trace>, <value>] strings.
+      text: The text to parse the metric values from.
+
+    Returns:
+      A list of floating point numbers found.
+    """
     # Format is: RESULT <graph>: <trace>= <value> <units>
     metric_formatted = 'RESULT %s: %s=' % (metric[0], metric[1])
 
     text_lines = text.split('\n')
     values_list = []
 
     for current_line in text_lines:
       # Parse the output from the performance test for the metric we're
       # interested in.
       metric_re = metric_formatted +\
@@ skipping 10 matching lines @@
         regex_results = metric_re.search(current_line)
 
         if not regex_results is None:
           metric_values = regex_results.group('values')
 
           values_list += metric_values.split(',')
 
     return [float(v) for v in values_list if IsStringFloat(v)]
 
   def RunPerformanceTestAndParseResults(self, command_to_run, metric):
-    """Runs a performance test on the current revision and parses
-    the results."""
+    """Runs a performance test on the current revision by executing the
+    'command_to_run' and parses the results.
 
-    if IsDebugIgnorePerfTestEnabled():
+    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.
+
+    Returns:
+      On success, it will return a tuple of the average value of the metric,
+      and a success code of 0.
+    """
+
+    if self.opts.debug_ignore_perf_test:
       return (0.0, 0)
 
     args = shlex.split(command_to_run)
 
     cwd = os.getcwd()
     os.chdir(self.src_cwd)
 
+    # Can ignore the return code since if the tests fail, it won't return 0.
     (output, return_code) = RunProcess(args)
 
     os.chdir(cwd)
 
     metric_values = self.ParseMetricValuesFromOutput(metric, output)
 
     # Need to get the average value if there were multiple values.
-    if len(metric_values) > 0:
+    if metric_values:
       average_metric_value = reduce(lambda x, y: float(x) + float(y),
                                     metric_values) / len(metric_values)
 
       return (average_metric_value, 0)
     else:
-      return (ERROR_PERF_TEST_NO_VALUES, -1)
+      return ('No values returned from performance test.', -1)
 
   def SyncBuildAndRunRevision(self, revision, depot, command_to_run, metric):
-    """Performs a full sync/build/run of the specified revision."""
+    """Performs a full sync/build/run of the specified revision.
+
+    Args:
+      revision: The revision to sync to.
+      depot: The depot that's being used at the moment (src, webkit, etc.)
+      command_to_run: The command to execute the performance test.
+      metric: The performance metric being tested.
+
+    Returns:
+      On success, a tuple containing the results of the performance test.
+      Otherwise, a tuple with the error message.
+    """
     use_gclient = (depot == 'chromium')
 
-    if self.source_control.SyncToRevision(revision, use_gclient):
+    if self.opts.debug_ignore_sync or\
+       self.source_control.SyncToRevision(revision, use_gclient):
+
       success = True
       if not(use_gclient):
         success = self.RunGClientHooks()
 
       if success:
         if self.BuildCurrentRevision():
           results = self.RunPerformanceTestAndParseResults(command_to_run,
                                                            metric)
 
           if results[1] == 0 and use_gclient:
             external_revisions = self.Get3rdPartyRevisionsFromCurrentRevision()
 
             if external_revisions:
               return (results[0], results[1], external_revisions)
             else:
-              return (ERROR_FAILED_DEPS_PARSE, 1)
+              return ('Failed to parse DEPS file for external revisions.', 1)
           else:
             return results
         else:
-          return (ERROR_FAILED_BUILD % (str(revision, )), 1)
+          return ('Failed to build revision: [%s]' % (str(revision, )), 1)
       else:
-        return (ERROR_FAILED_GCLIENT_RUNHOOKS, 1)
+        return ('Failed to run [gclient runhooks].', 1)
     else:
-      return (ERROR_FAILED_SYNC % (str(revision, )), 1)
+      return ('Failed to sync revision: [%s]' % (str(revision, )), 1)
 
   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."""
+    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 - known_good_value)
     dist_to_bad_value = abs(current_value - known_bad_value)
 
     return dist_to_good_value < dist_to_bad_value
 
+  def ChangeToDepotWorkingDirectory(self, depot_name):
+    """Given a depot, changes to the appropriate working directory.
+
+    Args:
+      depot_name: The name of the depot (see DEPOT_NAMES).
+    """
+    if depot_name == 'chromium':
+      os.chdir(self.src_cwd)
+    elif depot_name in DEPOT_NAMES:
+      os.chdir(self.depot_cwd[depot_name])
+    else:
+      assert False, 'Unknown depot [ %s ] encountered. Possibly a new one'\
+                    ' was added without proper support?' %\
+                    (depot_name,)
+
+  def PrepareToBisectOnDepot(self,
+                             current_depot,
+                             end_revision,
+                             start_revision):
+    """Changes to the appropriate directory and gathers a list of revisions
+    to bisect between |start_revision| and |end_revision|.
+
+    Args:
+      current_depot: The depot we want to bisect.
+      end_revision: End of the revision range.
+      start_revision: Start of the revision range.
+
+    Returns:
+      A list containing the revisions between |start_revision| and
+      |end_revision| inclusive.
+    """
+    # Change into working directory of external library to run
+    # subsequent commands.
+    old_cwd = os.getcwd()
+    os.chdir(self.depot_cwd[current_depot])
+
+    depot_revision_list = self.GetRevisionList(end_revision, start_revision)
+
+    os.chdir(old_cwd)
+
+    return depot_revision_list
+
+  def GatherReferenceValues(self, good_rev, bad_rev, cmd, metric):
+    """Gathers reference values by running the performance tests on the
+    known good and bad revisions.
+
+    Args:
+      good_rev: The last known good revision where the performance regression
+        has not occurred yet.
+      bad_rev: A revision where the performance regression has already occurred.
+      cmd: The command to execute the performance test.
+      metric: The metric being tested for regression.
+
+    Returns:
+      A tuple with the results of building and running each revision.
+    """
+    bad_run_results = self.SyncBuildAndRunRevision(bad_rev,
+                                                   'chromium',
+                                                   cmd,
+                                                   metric)
+
+    good_run_results = None
+
+    if not bad_run_results[1]:
+      good_run_results = self.SyncBuildAndRunRevision(good_rev,
+                                                      'chromium',
+                                                      cmd,
+                                                      metric)
+
+    return (bad_run_results, good_run_results)
+
+  def AddRevisionsIntoRevisionData(self, revisions, depot, sort, revision_data):
+    """Adds new revisions to the revision_data dict and initializes them.
+
+    Args:
+      revisions: List of revisions to add.
+      depot: Depot that's currently in use (src, webkit, etc...)
+      sort: Sorting key for displaying revisions.
+      revision_data: A dict to add the new revisions into. Existing revisions
+        will have their sort keys offset.
+    """
+
+    num_depot_revisions = len(revisions)
+
+    for k, v in revision_data.iteritems():
+      if v['sort'] > sort:
+        v['sort'] += num_depot_revisions
+
+    for i in xrange(num_depot_revisions):
+      r = revisions[i]
+
+      revision_data[r] = {'revision' : r,
+                          'depot' : depot,
+                          'value' : None,
+                          'passed' : '?',
+                          'sort' : i + sort + 1}
+
+  def PrintRevisionsToBisectMessage(self, revision_list, depot):
+    print
+    print 'Revisions to bisect on [src]:'
+    for revision_id in revision_list:
+      print('  -> %s' % (revision_id, ))
+    print
+
   def Run(self, command_to_run, bad_revision_in, good_revision_in, metric):
     """Given known good and bad revisions, run a binary search on all
     intermediate revisions to determine the CL where the performance regression
     occurred.
 
-    @param command_to_run Specify the command to execute the performance test.
-    @param good_revision Number/tag of the known good revision.
-    @param bad_revision Number/tag of the known bad revision.
-    @param metric The performance metric to monitor.
+    Args:
+        command_to_run: Specify the command to execute the performance test.
+        good_revision: Number/tag of the known good revision.
+        bad_revision: Number/tag of the known bad revision.
+        metric: The performance metric to monitor.
+
+    Returns:
+        A dict with 2 members, 'revision_data' and 'error'. On success,
+        'revision_data' will contain a dict mapping revision ids to
+        data about that revision. Each piece of revision data consists of a
+        dict with the following keys:
+
+        'passed': Represents whether the performance test was successful at
+            that revision. Possible values include: 1 (passed), 0 (failed),
+            '?' (skipped), 'F' (build failed).
+        'depot': The depot that this revision is from (ie. WebKit)
+        'external': If the revision is a 'src' revision, 'external' contains
+            the revisions of each of the external libraries.
+        'sort': A sort value for sorting the dict in order of commits.
+
+        For example:
+        {
+          'error':None,
+          'revision_data':
+          {
+            'CL #1':
+            {
+              'passed':False,
+              'depot':'chromium',
+              'external':None,
+              'sort':0
+            }
+          }
+        }
+
+        If an error occurred, the 'error' field will contain the message and
+        'revision_data' will be empty.
     """
 
     results = {'revision_data' : {},
                'error' : None}
 
     # If they passed SVN CL's, etc... we can try match them to git SHA1's.
     bad_revision = self.source_control.ResolveToRevision(bad_revision_in)
     good_revision = self.source_control.ResolveToRevision(good_revision_in)
 
     if bad_revision is None:
-      results['error'] = 'Could\'t resolve [%s] to SHA1.'%(bad_revision_in,)
+      results['error'] = 'Could\'t resolve [%s] to SHA1.' % (bad_revision_in,)
       return results
 
     if good_revision is None:
-      results['error'] = 'Could\'t resolve [%s] to SHA1.'%(good_revision_in,)
+      results['error'] = 'Could\'t resolve [%s] to SHA1.' % (good_revision_in,)
       return results
 
-    print MESSAGE_GATHERING_REVISIONS
+    print 'Gathering revision range for bisection.'
 
     # Retrieve a list of revisions to do bisection on.
     src_revision_list = self.GetRevisionList(bad_revision, good_revision)
 
     if src_revision_list:
       # revision_data will store information about a revision such as the
       # depot it came from, the webkit/skia/V8 revision at that time,
       # performance timing, build state, etc...
       revision_data = results['revision_data']
 
       # revision_list is the list we're binary searching through at the moment.
       revision_list = []
 
       sort_key_ids = 0
 
       for current_revision_id in src_revision_list:
         sort_key_ids += 1
 
         revision_data[current_revision_id] = {'value' : None,
                                               'passed' : '?',
                                               'depot' : 'chromium',
                                               'external' : None,
                                               'sort' : sort_key_ids}
         revision_list.append(current_revision_id)
 
       min_revision = 0
       max_revision = len(revision_list) - 1
 
-      print
-      print MESSAGE_BISECT_SRC
-      for revision_id in revision_list:
-        print('  -> %s' % (revision_id, ))
-      print
+      self.PrintRevisionsToBisectMessage(revision_list, 'src')
 
-      print MESSAGE_GATHERING_REFERENCE
+      print 'Gathering reference values for bisection.'
 
       # Perform the performance tests on the good and bad revisions, to get
       # reference values.
-      # todo: tidy this up
-      bad_revision_run_results = self.SyncBuildAndRunRevision(bad_revision,
-                                                              'chromium',
-                                                              command_to_run,
-                                                              metric)
+      (bad_results, good_results) = self.GatherReferenceValues(good_revision,
+                                                               bad_revision,
+                                                               command_to_run,
+                                                               metric)
 
-      if bad_revision_run_results[1] != 0:
-        results['error'] = bad_revision_run_results[0]
+      if bad_results[1]:
+        results['error'] = bad_results[0]
         return results
 
-      good_revision_run_results = self.SyncBuildAndRunRevision(good_revision,
-                                                              'chromium',
-                                                              command_to_run,
-                                                              metric)
+      if good_results[1]:
+        results['error'] = good_results[0]
+        return results
 
-      if good_revision_run_results[1] != 0:
-        results['error'] = good_revision_run_results[0]
-        return results
 
       # We need these reference values to determine if later runs should be
       # classified as pass or fail.
-      known_bad_value = bad_revision_run_results[0]
-      known_good_value = good_revision_run_results[0]
+      known_bad_value = bad_results[0]
+      known_good_value = good_results[0]
 
       # Can just mark the good and bad revisions explicitly here since we
       # already know the results.
       bad_revision_data = revision_data[revision_list[0]]
-      bad_revision_data['external'] = bad_revision_run_results[2]
+      bad_revision_data['external'] = bad_results[2]
       bad_revision_data['passed'] = 0
       bad_revision_data['value'] = known_bad_value
 
       good_revision_data = revision_data[revision_list[max_revision]]
-      good_revision_data['external'] = good_revision_run_results[2]
+      good_revision_data['external'] = good_results[2]
       good_revision_data['passed'] = 1
       good_revision_data['value'] = known_good_value
 
       while True:
         min_revision_data = revision_data[revision_list[min_revision]]
         max_revision_data = revision_data[revision_list[max_revision]]
 
         if max_revision - min_revision <= 1:
           if min_revision_data['passed'] == '?':
             next_revision_index = min_revision
           elif max_revision_data['passed'] == '?':
             next_revision_index = max_revision
           elif min_revision_data['depot'] == 'chromium':
             # If there were changes to any of the external libraries we track,
-            # should bisect the changes there as well..
-            external_changed = False
+            # should bisect the changes there as well.
+            external_depot = None
 
             for current_depot in DEPOT_NAMES:
               if min_revision_data['external'][current_depot] !=\
                  max_revision_data['external'][current_depot]:
-                # Change into working directory of external library to run
-                # subsequent commands.
-                old_cwd = os.getcwd()
-
-                os.chdir(self.depot_cwd[current_depot])
-                depot_rev_range = [min_revision_data['external'][current_depot],
-                                   max_revision_data['external'][current_depot]]
-                depot_revision_list = self.GetRevisionList(depot_rev_range[1],
-                                                           depot_rev_range[0])
-                os.chdir(old_cwd)
-
-                if depot_revision_list == None:
-                  results['error'] = ERROR_RETRIEVE_REVISION_RANGE %\
-                                     (depot_rev_range[1],
-                                      depot_rev_range[0])
-                  return results
-
-                # Add the new revisions
-                num_depot_revisions = len(depot_revision_list)
-                old_sort_key = min_revision_data['sort']
-                sort_key_ids += num_depot_revisions
-
-                for k, v in revision_data.iteritems():
-                  if v['sort'] > old_sort_key:
-                    v['sort'] += num_depot_revisions
-
-                for i in xrange(num_depot_revisions):
-                  r = depot_revision_list[i]
-
-                  revision_data[r] = {'revision' : r,
-                                      'depot' : current_depot,
-                                      'value' : None,
-                                      'passed' : '?',
-                                      'sort' : i + old_sort_key + 1}
-
-                revision_list = depot_revision_list
-
-                print MESSAGE_REGRESSION_DEPOT_METRIC % (metric, current_depot)
-                print
-                print MESSAGE_BISECT_DEPOT % (current_depot, )
-                for r in revision_list:
-                  print '  -> %s' % (r, )
-                print
-
-                # Reset the bisection and perform it on the webkit changelists.
-                min_revision = 0
-                max_revision = len(revision_list) - 1
-
-                # One of the external libraries changed, so we'll loop around
-                # and binary search through the new changelists.
-                external_changed = True
+                external_depot = current_depot
 
                 break
 
-            if external_changed:
-              continue
-            else:
+            # If there was no change in any of the external depots, the search
+            # is over.
+            if not external_depot:
               break
+
+            rev_range = [min_revision_data['external'][current_depot],
+                         max_revision_data['external'][current_depot]]
+
+            new_revision_list = self.PrepareToBisectOnDepot(external_depot,
+                                                            rev_range[1],
+                                                            rev_range[0])
+
+            if not new_revision_list:
+              results['error'] = 'An error occurred attempting to retrieve'\
+                                 ' revision range: [%s..%s]' %\
+                                 (depot_rev_range[1], depot_rev_range[0])
+              return results
+
+            self.AddRevisionsIntoRevisionData(new_revision_list,
+                                              external_depot,
+                                              min_revision_data['sort'],
+                                              revision_data)
+
+            # Reset the bisection and perform it on the newly inserted
+            # changelists.
+            revision_list = new_revision_list
+            min_revision = 0
+            max_revision = len(revision_list) - 1
+            sort_key_ids += len(revision_list)
+
+            print 'Regression in metric:%s appears to be the result of changes'\
+                  ' in [%s].' % (metric, current_depot)
+
+            self.PrintRevisionsToBisectMessage(revision_list, external_depot)
+
+            continue
           else:
             break
         else:
           next_revision_index = int((max_revision - min_revision) / 2) +\
                                 min_revision
 
         next_revision_id = revision_list[next_revision_index]
         next_revision_data = revision_data[next_revision_id]
         next_revision_depot = next_revision_data['depot']
 
+        self.ChangeToDepotWorkingDirectory(next_revision_depot)
 
-        # Change current working directory depending on what we're building.
-        if next_revision_depot == 'chromium':
-          os.chdir(self.src_cwd)
-        elif next_revision_depot in DEPOT_NAMES:
-          os.chdir(self.depot_cwd[next_revision_depot])
-        else:
-          assert False
-
-        print MESSAGE_WORK_ON_REVISION % next_revision_id
+        print 'Working on revision: [%s]' % next_revision_id
 
         run_results = self.SyncBuildAndRunRevision(next_revision_id,
                                                    next_revision_depot,
                                                    command_to_run,
                                                    metric)
 
         # If the build is successful, check whether or not the metric
         # had regressed.
         if run_results[1] == 0:
           if next_revision_depot == 'chromium':
@@ skipping 12 matching lines @@
             min_revision = next_revision_index
         else:
           next_revision_data['passed'] = 'F'
 
           # If the build is broken, remove it and redo search.
           revision_list.pop(next_revision_index)
 
           max_revision -= 1
     else:
       # Weren't able to sync and retrieve the revision range.
-      results['error'] = ERROR_RETRIEVE_REVISION_RANGE % (good_revision,
-                                                          bad_revision)
+      results['error'] = 'An error occurred attempting to retrieve revision '\
+                         'range: [%s..%s]' % (good_revision, bad_revision)
 
     return results
 
+  def FormatAndPrintResults(self, bisect_results):
+    """Prints the results from a bisection run in a readable format.
+
+    Args
+      bisect_results: The results from a bisection test run.
+    """
+    revision_data = bisect_results['revision_data']
+    revision_data_sorted = sorted(revision_data.iteritems(),
+                                  key = lambda x: x[1]['sort'])
+
+    print
+    print 'Full results of bisection:'
+    for current_id, current_data  in revision_data_sorted:
+      build_status = current_data['passed']
+      metric_value = current_data['value']
+
+      if type(build_status) is bool:
+        build_status = int(build_status)
+
+      if metric_value is None:
+        metric_value = ''
+
+      print('  %8s  %s  %s %6s' %\
+            (current_data['depot'], current_id, build_status, metric_value))
+    print
+
+    # Find range where it possibly broke.
+    first_working_revision = None
+    last_broken_revision = None
+
+    for k, v in revision_data_sorted:
+      if v['passed'] == True:
+        if first_working_revision is None:
+          first_working_revision = k
+
+      if v['passed'] == False:
+        last_broken_revision = k
+
+    if last_broken_revision != None and first_working_revision != None:
+      print 'Results: Regression was detected as a result of changes on:'
+      print '  -> First Bad Revision: [%s] [%s]' %\
+            (last_broken_revision,
+            revision_data[last_broken_revision]['depot'])
+      print '  -> Last Good Revision: [%s] [%s]' %\
+            (first_working_revision,
+            revision_data[first_working_revision]['depot'])
+
 
 def ParseGClientForSourceControl():

[shatch, 2013/01/30 19:50:29]

This name doesn't reflect what the function does anymore, will change it.

-  """Parses the .gclient file to determine source control method."""
-  try:
-    deps_file = imp.load_source('gclient', '../.gclient')
+  """Parses the .gclient file to determine source control method.
 
-    if deps_file.solutions[0].has_key('deps_file'):
-      if deps_file.solutions[0]['deps_file'] == '.DEPS.git':
-        source_control = GitSourceControl()
+  Returns:
+    An instance of a SourceControl object, or None if the current workflow
+    is unsupported.
+  """
 
-        return (source_control, None)
-      else:
-        return (None, ERROR_MESSAGE_SVN_UNSUPPORTED)
-    else:
-      return (None, ERROR_MESSAGE_SVN_UNSUPPORTED)
-  except ImportError:
-    return (None, ERROR_MESSAGE_IMPORT_GCLIENT)
-  except IOError:
-    return (None, ERROR_MESSAGE_GCLIENT_NOT_FOUND)
+  (output, return_code) = RunProcess(['git',

[tonyg, 2013/01/30 18:26:28]

Maybe RunGit should become an @staticmethod so that it can be used here.

[shatch, 2013/01/30 19:50:29]

Done.

+                                      'rev-parse',
+                                      '--is-inside-work-tree'])
+
+  if output.strip() == 'true':
+    return GitSourceControl()
+
+  return None
 
 
 def main():
 
   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')
 
   parser = optparse.OptionParser(usage=usage)
 
   parser.add_option('-c', '--command',
-                    type = 'str',
-                    help = 'A command to execute your performance test at' +
+                    type='str',
+                    help='A command to execute your performance test at' +
                     ' each point in the bisection.')
   parser.add_option('-b', '--bad_revision',
-                    type = 'str',
-                    help = 'A bad revision to start bisection. ' +
+                    type='str',
+                    help='A bad revision to start bisection. ' +
                     'Must be later than good revision. ')

[tonyg, 2013/01/30 18:26:28]

Might be worth stating in the help strings that these are git or svn revisions.

[shatch, 2013/01/30 19:50:29]

Done.

   parser.add_option('-g', '--good_revision',
-                    type = 'str',
-                    help = 'A revision to start bisection where performance' +
+                    type='str',
+                    help='A revision to start bisection where performance' +
                     ' test is known to pass. Must be earlier than the ' +
                     'bad revision.')
   parser.add_option('-m', '--metric',
-                    type = 'str',
-                    help = 'The desired metric to bisect on.')
+                    type='str',
+                    help='The desired metric to bisect on.')
   parser.add_option('--use_goma',
-                    action = "store_true",
-                    help = 'Add a bunch of extra threads for goma.')
+                    action="store_true",
+                    help='Add a bunch of extra threads for goma.')
   parser.add_option('--debug_ignore_build',
-                    action = "store_true",
-                    help = 'DEBUG: Don\'t perform builds.')
+                    action="store_true",
+                    help='DEBUG: Don\'t perform builds.')
   parser.add_option('--debug_ignore_sync',
-                    action = "store_true",
-                    help = 'DEBUG: Don\'t perform syncs.')
+                    action="store_true",
+                    help='DEBUG: Don\'t perform syncs.')
   parser.add_option('--debug_ignore_perf_test',
-                    action = "store_true",
-                    help = 'DEBUG: Don\'t perform performance tests.')
+                    action="store_true",
+                    help='DEBUG: Don\'t perform performance tests.')
   (opts, args) = parser.parse_args()
 
-  if opts.command is None:
-    print ERROR_MISSING_COMMAND
+  if not opts.command:
+    print 'Error: missing required parameter: --command'
     print
     parser.print_help()
     return 1
 
-  if opts.good_revision is None:
-    print ERROR_MISSING_GOOD_REVISION
+  if not opts.good_revision:
+    print 'Error: missing required parameter: --good_revision'
     print
     parser.print_help()
     return 1
 
-  if opts.bad_revision is None:
-    print ERROR_MISSING_BAD_REVISION
+  if not opts.bad_revision:
+    print 'Error: missing required parameter: --bad_revision'
     print
     parser.print_help()
     return 1
 
-  if opts.metric is None:
-    print ERROR_MISSING_METRIC
+  if not opts.metric:
+    print 'Error: missing required parameter: --metric'
     print
     parser.print_help()
     return 1
 
   # Haven't tested the script out on any other platforms yet.
-  if not os.name in SUPPORTED_OS_TYPES:
-    print ERROR_MESSAGE_OS_NOT_SUPPORTED
+  if not os.name in ['posix']:
+    print "Sorry, this platform isn't supported yet."
     print
     return 1
 
 
-  SetDebugIgnoreFlags(opts.debug_ignore_build,
-                      opts.debug_ignore_sync,
-                      opts.debug_ignore_perf_test)
-
-  SetGlobalGomaFlag(opts.use_goma)
-
   # Check what source control method they're using. Only support git workflow
   # at the moment.
-  (source_control, error_message) = ParseGClientForSourceControl()
+  source_control = ParseGClientForSourceControl()
 
-  if source_control == None:
-    print error_message
+  if not source_control:
+    print "Sorry, only the git workflow is supported at the moment."
     print
     return 1
 
   # gClient sync seems to fail if you're not in master branch.
-  if not(source_control.IsInProperBranch()):
-    print ERROR_INCORRECT_BRANCH
+  if not source_control.IsInProperBranch():
+    print "You must switch to master branch to run bisection."
     print
     return 1
 
   metric_values = opts.metric.split('/')
   if len(metric_values) < 2:
-    metric_values.append('')
+    print "Invalid metric specified: [%s]" % (opts.metric,)
+    print
+    return 1
 
-  bisect_test = BisectPerformanceMetrics(source_control)
+
+  bisect_test = BisectPerformanceMetrics(source_control, opts)
   bisect_results = bisect_test.Run(opts.command,
                                    opts.bad_revision,
                                    opts.good_revision,
                                    metric_values)
 
   if not(bisect_results['error']):
-    revision_data = bisect_results['revision_data']
-    revision_data_sorted = sorted(revision_data.iteritems(),
-                                  key = lambda x: x[1]['sort'])
-
-    print
-    print 'Full results of bisection:'
-    for k, v in revision_data_sorted:
-      b = v['passed']
-      f = v['value']
-
-      if type(b) is bool:
-        b = int(b)
-
-      if f is None:
-        f = ''
-
-      print('  %8s  %s  %s %6s' % (v['depot'], k, b, f))
-    print
-
-    # Find range where it possibly broke.
-    first_working_revision = None
-    last_broken_revision = None
-
-    for k, v in revision_data_sorted:
-      if v['passed'] == True:
-        if first_working_revision is None:
-          first_working_revision = k
-
-      if v['passed'] == False:
-        last_broken_revision = k
-
-    if last_broken_revision != None and first_working_revision != None:
-      print 'Results: Regression was detected as a result of changes on:'
-      print '  -> First Bad Revision: [%s] [%s]' %\
-            (last_broken_revision,
-            revision_data[last_broken_revision]['depot'])
-      print '  -> Last Good Revision: [%s] [%s]' %\
-            (first_working_revision,
-            revision_data[first_working_revision]['depot'])
+    bisect_test.FormatAndPrintResults(bisect_results)
     return 0
   else:
     print 'Error: ' + bisect_results['error']
     print
     return 1
 
 if __name__ == '__main__':
   sys.exit(main())