Refactor perf bisect script _CalculateConfidence method.

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 1357 matching lines @@
       RmTreeAndMkDir(output_dir, skip_makedir=True)
       ExtractZip(archive_file_dest, abs_build_dir)
       if os.path.exists(output_dir):
         self.BackupOrRestoreOutputdirectory(restore=False)
         print 'Moving build from %s to %s' % (
             output_dir, target_build_output_dir)
         shutil.move(output_dir, target_build_output_dir)
         return True
       raise IOError('Missing extracted folder %s ' % output_dir)
     except e:
-      print 'Somewthing went wrong while extracting archive file: %s' % e
+      print 'Something went wrong while extracting archive file: %s' % e
       self.BackupOrRestoreOutputdirectory(restore=True)
       # Cleanup any leftovers from unzipping.
       if os.path.exists(output_dir):
         RmTreeAndMkDir(output_dir, skip_makedir=True)
     finally:
       # Delete downloaded archive
       if os.path.exists(archive_file_dest):
         os.remove(archive_file_dest)
     return False
 
@@ skipping 1392 matching lines @@
               bad_greater_than_good else not prev_less_than_current)
 
           # Only report potential regressions with high confidence.
           if is_same_direction and confidence > 50:
             other_regressions.append([current_id, previous_id, confidence])
         previous_values.append(current_values)
         previous_id = current_id
     return other_regressions
 
   def _CalculateConfidence(self, working_means, broken_means):
-    bounds_working = []
-    bounds_broken = []
-    for m in working_means:
-      current_mean = CalculateTruncatedMean(m, 0)
-      if bounds_working:
-        bounds_working[0] = min(current_mean, bounds_working[0])
-        bounds_working[1] = max(current_mean, bounds_working[0])
-      else:
-        bounds_working = [current_mean, current_mean]
-    for m in broken_means:
-      current_mean = CalculateTruncatedMean(m, 0)
-      if bounds_broken:
-        bounds_broken[0] = min(current_mean, bounds_broken[0])
-        bounds_broken[1] = max(current_mean, bounds_broken[0])
-      else:
-        bounds_broken = [current_mean, current_mean]
+    """Calculates the confidence percentage.
+
+    This is calculated based on how distinct the values are before and after
+    the last broken revision, and how noisy the results are.
+
+    Args:
+      working_means: A list of lists of "good" result numbers.

[qyearsley, 2014/03/25 02:25:33]

So, this argument name kind of confused me. Individual members of this list are
input to CalculateTruncatedMean, so that makes it seem like the members of this
list are lists of values, and are not means?

+      broken means: A list of lists of "bad" result numbers.
+
+    Returns:
+      A number between in the range [0, 100].
+    """
+    bounds_working = self._CalculateBounds(working_means)
+    bounds_broken = self._CalculateBounds(broken_means)
     dist_between_groups = min(math.fabs(bounds_broken[1] - bounds_working[0]),
-        math.fabs(bounds_broken[0] - bounds_working[1]))
+                              math.fabs(bounds_broken[0] - bounds_working[1]))
     working_mean = sum(working_means, [])
     broken_mean = sum(broken_means, [])
     len_working_group = CalculateStandardDeviation(working_mean)
     len_broken_group = CalculateStandardDeviation(broken_mean)
-
-    confidence = (dist_between_groups / (
-        max(0.0001, (len_broken_group + len_working_group ))))
+    confidence = (dist_between_groups /
+                  (max(0.0001, (len_broken_group + len_working_group))))
     confidence = int(min(1.0, max(confidence, 0.0)) * 100.0)
     return confidence
 
+  def _CalculateBounds(self, values_list):
+    """Returns the lower/upper bounds for the means of the given value lists.
+
+    Args:
+      values_list: A non-empty list of lists of numbers.
+
+    Returns:
+      A (lower, upper) pair of bounds.
+    """
+    bounds = None
+    for values in data:

[prasadv, 2014/03/25 16:43:47]

s/data/values_list

[qyearsley, 2014/03/25 17:31:19]

Done.

+      mean = CalculateTruncatedMean(values, 0)

[qyearsley, 2014/03/25 02:25:33]

The second argument to CalculateTruncatedMean is supposed to be a "The % from
the upper/lower portions of the data set to discards".

So wouldn't CalculateTruncatedMean(vals, 0) be just Mean(vals) ? Maybe it would
be clearer to add a function like the one below just to make it clear we're not
doing any fancy truncation stuff?

def Mean(x):
  return CalculateTruncatedMean(x, 0)

+      if bounds:
+        bounds[0] = min(current_mean, bounds[0])

[prasadv, 2014/03/25 16:43:47]

s/current_mean/mean

[qyearsley, 2014/03/25 17:31:19]

Done.

+        bounds[1] = max(current_mean, bounds[1])

[qyearsley, 2014/03/25 02:25:33]

Attention! This changes the behavior!

In the existing version on line 2797, max is calculated as:
  max(current_mean, bounds_working[0])

But it looks like bounds_working[0] is supposed to be the lower bound. Is it?
Does this seem right?

[shatch, 2014/03/25 19:56:21]

Nice catch! Yeah that should be max(current_mean, bounds_working[1]).

On 2014/03/25 02:25:33, qyearsley wrote:

[shatch, 2014/03/25 19:58:36]

BTW, if you want to experiment with changing this, go right ahead! It was just a
simple, conservative stab at calculating confidence, but it's pretty arbitrary.
Had plans to try out a t-test at some point to see if it gave better results.

On 2014/03/25 19:56:21, shatch wrote:

+      else:
+        bounds = (mean, mean)
+    return bounds
+
   def _GetResultsDict(self, revision_data, revision_data_sorted):
+    """Makes and returns a dictionary of overall results from the bisect.
+
+    Args:
+      revision_data: The revisions_data dict as returned by the Run method.
+      revision_data_sorted: A list of pairs from the above dictionary, sorted
+          in order of commits.
+
+    Returns:
+      A dictionary containing results from the bisect.
+    """
     # Find range where it possibly broke.
     first_working_revision = None
     first_working_revision_index = -1
     last_broken_revision = None
     last_broken_revision_index = -1
 
     for i in xrange(len(revision_data_sorted)):
       k, v = revision_data_sorted[i]
       if v['passed'] == 1:
         if not first_working_revision:
@@ skipping 552 matching lines @@
       # The perf dashboard scrapes the "results" step in order to comment on
       # bugs. If you change this, please update the perf dashboard as well.
       bisect_utils.OutputAnnotationStepStart('Results')
     print 'Error: %s' % e.message
     if opts.output_buildbot_annotations:
       bisect_utils.OutputAnnotationStepClosed()
   return 1
 
 if __name__ == '__main__':
   sys.exit(main())