Change "percentage change" function and add comments/test for it.

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 273 matching lines @@
 
   mean = CalculateMean(values)
   differences_from_mean = [float(x) - mean for x in values]
   squared_differences = [float(x * x) for x in differences_from_mean]
   variance = sum(squared_differences) / (len(values) - 1)
   std_dev = math.sqrt(variance)
 
   return std_dev
 
 
+def CalculateRelativeChange(before, after):
+  """Returns the relative change of before and after, relative to before.
+
+  There are several different ways to define relative difference between
+  two numbers; sometimes it is defined as relative to the smaller number,
+  or to the mean of the two numbers. This version returns the difference
+  relative to the first of the two numbers.
+
+  Args:
+    before: A number representing an earlier value.
+    after: Another number, representing a later value.
+
+  Returns:
+    A non-negative floating point number; 0.1 represents a 10% change.
+  """
+  if before == 0:

[shatch, 2014/04/17 19:46:17]

If the "good" values were legitimately 0 (ie. the values being bisected were #
of memory leaks or some such), won't this report a regression size of nan? Don't
think it functionally changes anything, just might look a bit funny.

[qyearsley, 2014/04/17 20:54:05]

Yeah -- In this version before my latest patch, the output would be:
  Bisect reproduced a nan (+-stderr%) change ...

But it might be more informative to output:
  Bisect reproduced a zero-to-nonzero (+-stderr%) change...

Note that the dashboard has a special case from zero-to-nonzero; Annie used the
string "freakin' huge", so it says "Found a freakin' huge regression in ...".
This occasionally confuses some sheriffs.

In the latest patch set for this CL, I've made a change so that if this function
returns NaN, then the message printed should be "zero-to-nonzero". If you think
it's better, We could also return some other value like 10000000 or "zero to
nonzero".

[shatch, 2014/04/17 22:58:28]

Hmm, yeah I'm not sure what the best output here would be. "zero-to-nonzero"
doesn't feel right either, definitely better than nan though. Maybe Prasad has
some thoughts on this.


On 2014/04/17 20:54:05, qyearsley wrote:

[qyearsley, 2014/04/18 19:38:03]

Anyway, if we're calculating relative change relative to 0, I don't think a
numerical return value is very helpful.

The other way of calculating relative change has the same issue; the relative
change from 1 to 0 or 0 to 1 would have been:
   (1 / 0.0001) * 100 - 100
 = 999900.0

+    return float('nan')
+  difference = math.fabs(after - before)

[shatch, 2014/04/17 19:46:17]

This fabs call seems unnecessary, considering you do it again on the next line.

[qyearsley, 2014/04/17 20:54:05]

Good point, done. (My original thinking was: first line get the absolute
difference. Second line also get abs value because |before| might be negative.
But the result is the same if abs is not taken of the difference.)

+  return math.fabs(difference / before)
+
+
 def CalculatePooledStandardError(work_sets):
   numerator = 0.0
   denominator1 = 0.0
   denominator2 = 0.0
 
   for current_set in work_sets:
     std_dev = CalculateStandardDeviation(current_set)
     numerator += (len(current_set) - 1) * std_dev ** 2
     denominator1 += len(current_set) - 1
     denominator2 += 1.0 / len(current_set)
@@ skipping 2826 matching lines @@
           working_means.append(revision_data_sorted[i][1]['value']['values'])
 
       # Flatten the lists to calculate mean of all values.
       working_mean = sum(working_means, [])
       broken_mean = sum(broken_means, [])
 
       # Calculate the approximate size of the regression
       mean_of_bad_runs = CalculateMean(broken_mean)
       mean_of_good_runs = CalculateMean(working_mean)
 
-      regression_size = math.fabs(max(mean_of_good_runs, mean_of_bad_runs) /
-          max(0.0001, min(mean_of_good_runs, mean_of_bad_runs))) * 100.0 - 100.0
+      regression_size = 100 * CalculateRelativeChange(mean_of_good_runs,
+                                                      mean_of_bad_runs)
 
       regression_std_err = math.fabs(CalculatePooledStandardError(
           [working_mean, broken_mean]) /
           max(0.0001, min(mean_of_good_runs, mean_of_bad_runs))) * 100.0
 
       # Give a "confidence" in the bisect. At the moment we use how distinct the
       # values are before and after the last broken revision, and how noisy the
       # overall graph is.
       confidence = CalculateConfidence(working_means, broken_means)
 
@@ skipping 514 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())