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

tools/bisect-perf-regression.py

Index: tools/bisect-perf-regression.py
diff --git a/tools/bisect-perf-regression.py b/tools/bisect-perf-regression.py
index 8f126df091df9b1eebd311614fc9a98d74be5068..1d668afa68204b314e4b8e935bf96a611c0437f5 100755
--- a/tools/bisect-perf-regression.py
+++ b/tools/bisect-perf-regression.py
@@ -12,67 +12,19 @@ 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.
-"""
-
-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"
+An example usage:
 
-ERROR_MESSAGE_IMPORT_GCLIENT = "An error occurred while importing .gclient"\
-                               " file."
+./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
 
-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
@@ -82,82 +34,62 @@ 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
-
-
-def IsGlobalGomaFlagEnabled():
-  global GLOBAL_BISECT_OPTIONS
-
-  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']
+FILE_DEPS_GIT = '.DEPS.git'
 
 
-def IsDebugIgnorePerfTestEnabled():
-  """Returns whether performance test commands are being ignored."""
-  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['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,
@@ -169,6 +101,7 @@ def RunProcess(command):
   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
@@ -177,7 +110,16 @@ class SourceControl(object):
     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)
@@ -189,14 +131,30 @@ class GitSourceControl(SourceControl):
     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',
@@ -204,28 +162,46 @@ class GitSourceControl(SourceControl):
                                              '--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
@@ -237,20 +213,31 @@ class GitSourceControl(SourceControl):
                                              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"
@@ -262,15 +249,16 @@ class BisectPerformanceMetrics(object):
   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
@@ -282,7 +270,11 @@ class BisectPerformanceMetrics(object):
     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)
@@ -295,8 +287,7 @@ class BisectPerformanceMetrics(object):
 
     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]):
@@ -312,18 +303,21 @@ class BisectPerformanceMetrics(object):
     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',
@@ -346,22 +340,33 @@ class BisectPerformanceMetrics(object):
 
     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.
+
+    Returns:
+      True if gclient reports no errors.
+    """
 
-    if IsDebugIgnoreBuildEnabled():
+    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])
 
@@ -392,10 +397,19 @@ class BisectPerformanceMetrics(object):
     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.
+
+    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 IsDebugIgnorePerfTestEnabled():
+    if self.opts.debug_ignore_perf_test:
       return (0.0, 0)
 
     args = shlex.split(command_to_run)
@@ -403,6 +417,7 @@ class BisectPerformanceMetrics(object):
     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)
@@ -410,19 +425,32 @@ class BisectPerformanceMetrics(object):
     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()
@@ -438,33 +466,180 @@ class BisectPerformanceMetrics(object):
             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' : {},
@@ -475,14 +650,14 @@ class BisectPerformanceMetrics(object):
     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)
@@ -511,49 +686,40 @@ class BisectPerformanceMetrics(object):
       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)
-
-      if bad_revision_run_results[1] != 0:
-        results['error'] = bad_revision_run_results[0]
-        return results
+      (bad_results, good_results) = self.GatherReferenceValues(good_revision,
+                                                               bad_revision,
+                                                               command_to_run,
+                                                               metric)
 
-      good_revision_run_results = self.SyncBuildAndRunRevision(good_revision,
-                                                              'chromium',
-                                                              command_to_run,
-                                                              metric)
+      if bad_results[1]:
+        results['error'] = bad_results[0]
+        return results
 
-      if good_revision_run_results[1] != 0:
-        results['error'] = good_revision_run_results[0]
+      if good_results[1]:
+        results['error'] = good_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
 
@@ -568,70 +734,52 @@ class BisectPerformanceMetrics(object):
             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:
@@ -642,16 +790,9 @@ class BisectPerformanceMetrics(object):
         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,
@@ -684,30 +825,75 @@ class BisectPerformanceMetrics(object):
           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():
@@ -719,137 +905,96 @@ def main():
   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']