Try to expand the revision range to include a change to .DEPS.git if the script detects a change to…

tools/bisect-perf-regression.py

 #!/usr/bin/env python
 # Copyright (c) 2013 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """Performance Test Bisect Tool
 
 This script bisects a series of changelists using binary search. It starts at
 a bad revision where a performance metric has regressed, and asks for a last
 known-good revision. It will then binary search across this revision range by
@@ skipping 476 matching lines @@
     """Unstages a file and returns it to HEAD.
 
     Returns:
       True if successful.
     """
     # Reset doesn't seem to return 0 on success.
     RunGit(['reset', 'HEAD', bisect_utils.FILE_DEPS_GIT])
 
     return not RunGit(['checkout', bisect_utils.FILE_DEPS_GIT])[1]
 
+  def QueryFileRevisionHistory(self, filename, revision_start, revision_end):
+    """Returns a list of commits that modified this file.
+
+    Args:
+        filename: Name of file.
+        revision_start: Start of revision range.
+        revision_end: End of revision range.
+
+    Returns:
+        Returns a list of commits that touched this file.
+    """
+    cmd = ['log', '--format=%H', '%s^1..%s' % (revision_start, revision_end),
+           filename]
+    (output, return_code) = RunGit(cmd)
+
+    assert not return_code, 'An error occurred while running'\

[tonyg, 2013/05/28 23:16:07]

We already do this assert not return_code dance in a lot of places and this
patch adds 3 new ones. What do you think about factoring out a CheckRunGit(cmd)
method that does the assert and only returns output.

[shatch, 2013/05/28 23:38:17]

Done.

+                            ' "git %s"' % ' '.join(cmd)
+
+    return [o for o in output.split('\n') if o]
+
 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, opts):
     super(BisectPerformanceMetrics, self).__init__()
 
     self.opts = opts
     self.source_control = source_control
     self.src_cwd = os.getcwd()
     self.depot_cwd = {}
     self.cleanup_commands = []
+    self.warnings = []
 
     # This always starts true since the script grabs latest first.
     self.was_blink = True
 
     for d in DEPOT_NAMES:
       # The working directory of each depot is just the path to the depot, but
       # since we're already in 'src', we can skip that part.
 
       self.depot_cwd[d] = self.src_cwd + DEPOT_DEPS_NAME[d]['src'][3:]
 
@@ skipping 517 matching lines @@
 
     print
     print 'Revisions to bisect on [%s]:' % depot
     for revision_id in revision_list:
       print '  -> %s' % (revision_id, )
     print
 
     if self.opts.output_buildbot_annotations:
       bisect_utils.OutputAnnotationStepClosed()
 
+  def NudgeRevisionsIfDEPSChange(self, bad_revision, good_revision):
+    """Checks to see if changes to DEPS file occurred, and that the revision
+    range also includes the change to .DEPS.git. If it doesn't, attempts to
+    expand the revision range to include it.
+
+    Args:
+        good_revision: Last known good revision.
+        bad_rev: First known bad revision.

[tonyg, 2013/05/28 23:16:07]

Nit: args are in the wrong order here

[shatch, 2013/05/28 23:38:17]

Done.

+
+    Returns:
+        A tuple with the new good and bad revisions.

[tonyg, 2013/05/28 23:16:07]

A tuple with the new bad and good revisions

[shatch, 2013/05/28 23:38:17]

Done.

+    """
+    if self.source_control.IsGit():
+      changes_to_deps = self.source_control.QueryFileRevisionHistory(
+          'DEPS', good_revision, bad_revision)
+
+      if changes_to_deps:
+        # DEPS file was changed, search from the oldest change to DEPS file to
+        # bad_revision to see if there are matching .DEPS.git changes.
+        oldest_deps_change = changes_to_deps[len(changes_to_deps) - 1]

[tonyg, 2013/05/28 23:16:07]

Neat python trick:
oldest_deps_change = changes_to_deps[-1]

[shatch, 2013/05/28 23:38:17]

Cool!

+        changes_to_gitdeps = self.source_control.QueryFileRevisionHistory(
+            bisect_utils.FILE_DEPS_GIT, oldest_deps_change, bad_revision)
+
+        if len(changes_to_deps) != len(changes_to_gitdeps):
+          # Grab the timestamp of the last DEPS change
+          cmd = ['log', '--format=%ct', '-1', changes_to_deps[0]]
+          (output, return_code) = RunGit(cmd)
+
+          assert not return_code, 'An error occurred while running'\
+                                  ' "git %s"' % ' '.join(cmd)
+
+          commit_time = int(output)
+
+          # Try looking for a commit that touches the .DEPS.git file in the
+          # next 5 minutes after the DEPS file change.

[tonyg, 2013/05/28 23:16:07]

I didn't look how long it usually takes, but 5 minutes seems slightly brittle.
Maybe 15 or 20 mins?

[shatch, 2013/05/28 23:38:17]

Done.

+          cmd = ['log', '--format=%H', '-1',
+              '--before=%d' % (commit_time + 300), '--after=%d' % commit_time,
+              'origin/master', bisect_utils.FILE_DEPS_GIT]
+          (output, return_code) = RunGit(cmd)
+
+          assert not return_code, 'An error occurred while running'\
+                                  ' "git %s"' % ' '.join(cmd)
+
+          output = output.strip()
+          if output:
+            self.warnings.append('Detected change to DEPS and modified '
+                'revision range to include change to .DEPS.git')
+            return (output, good_revision)
+          else:
+            self.warnings.append('Detected change to DEPS but couldn\'t find '
+                'matching change to .DEPS.git')
+    return (bad_revision, good_revision)
+
   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.
 
     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.
@@ skipping 41 matching lines @@
                                                           'src', -100)
 
     if bad_revision is None:
       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,)
       return results
 
+    (bad_revision, good_revision) = self.NudgeRevisionsIfDEPSChange(
+        bad_revision, good_revision)
+
     if self.opts.output_buildbot_annotations:
       bisect_utils.OutputAnnotationStepStart('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 self.opts.output_buildbot_annotations:
       bisect_utils.OutputAnnotationStepClosed()
@@ skipping 255 matching lines @@
       good_std_dev = revision_data[first_working_revision]['value']['std_dev']
       good_mean = revision_data[first_working_revision]['value']['mean']
       bad_mean = revision_data[last_broken_revision]['value']['mean']
 
       # A standard deviation of 0 could indicate either insufficient runs
       # or a test that consistently returns the same value.
       if good_std_dev > 0:
         deviations = math.fabs(bad_mean - good_mean) / good_std_dev
 
         if deviations < 1.5:
-          print 'Warning: Regression was less than 1.5 standard deviations '\
-                'from "good" value. Results may not be accurate.'
-          print
+          self.warnings.append('Regression was less than 1.5 standard '
+            'deviations from "good" value. Results may not be accurate.')

[tonyg, 2013/05/28 23:16:07]

Something for a future CL that I'll mention while it's on my mind: It would be
nice if the script printed the magnitude of the change that it detected. That
is, the percent difference of the average of all the "good" revisions and the
average of all the "bad" revisions.

Then the sheriff can more easily tell whether the change that it detected
matched the size of the change they were looking for.

[shatch, 2013/05/28 23:38:17]

Yeah that sounds reasonable, I'll get a version with that change up soon.

       elif self.opts.repeat_test_count == 1:
-        print 'Warning: Tests were only set to run once. This may be '\
-              'insufficient to get meaningful results.'
-        print
+        self.warnings.append('Tests were only set to run once. This '
+            'may be insufficient to get meaningful results.')
 
       # Check for any other possible regression ranges
       prev_revision_data = revision_data_sorted[0][1]
       prev_revision_id = revision_data_sorted[0][0]
       possible_regressions = []
       for current_id, current_data in revision_data_sorted:
         if current_data['value']:
           prev_mean = prev_revision_data['value']['mean']
           cur_mean = current_data['value']['mean']
 
@@ skipping 40 matching lines @@
           if percent_change is None:
             percent_change = 0
 
           print '  %8s  %s  [%.2f%%, %s x std.dev]' % (
               previous_data['depot'], previous_id, 100 * percent_change,
               deviations)
           print '  %8s  %s' % (
               current_data['depot'], current_id)
           print
 
+      if self.warnings:
+        print
+        print 'The following warnings were generated:'
+        print
+        for w in self.warnings:
+          print '  - %s' % w
+        print
+
     if self.opts.output_buildbot_annotations:
       bisect_utils.OutputAnnotationStepClosed()
 
 
 def DetermineAndCreateSourceControl():
   """Attempts to determine the underlying source control workflow and returns
   a SourceControl object.
 
   Returns:
     An instance of a SourceControl object, or None if the current workflow
@@ skipping 269 matching lines @@
 
   if not(bisect_results['error']):
     return 0
   else:
     print 'Error: ' + bisect_results['error']
     print
     return 1
 
 if __name__ == '__main__':
   sys.exit(main())