Codes for writing bench data to appengine datastore.

bench/bench_graph_svg.py

 '''
 Created on May 16, 2011
 
 @author: bungeman
 '''
 import sys
 import getopt
 import re
 import os
 import bench_util
 import json
+import httplib

[epoger, 2013/03/05 18:38:52]

Please put imports in alpha order

[benchen, 2013/03/05 20:52:53]

Done. Didn't do so since it conflicted with the previous "style".
On 2013/03/05 18:38:52, epoger wrote:

+import urllib
+import urllib2
 import xml.sax.saxutils
 
 # We throw out any measurement outside this range, and log a warning.
 MIN_REASONABLE_TIME = 0
 MAX_REASONABLE_TIME = 99999
 
 # Constants for prefixes in output title used in buildbot.
 TITLE_PREAMBLE = 'Bench_Performance_for_Skia_'
 TITLE_PREAMBLE_LENGTH = len(TITLE_PREAMBLE)
 
+# Number of data points to send to appengine at once.
+DATA_POINTS_TO_SEND = 100

[epoger, 2013/03/05 18:38:52]

Maybe DATA_POINT_BATCHSIZE instead, because you're sending ALL of the data
points you found... you're just sending them 100 per HTTP request. Right?

[benchen, 2013/03/05 20:52:53]

Yeah that's better. Done.
On 2013/03/05 18:38:52, epoger wrote:

+
 def usage():
     """Prints simple usage information."""
-    
+
+    print '-a <url> the url to use for adding bench values to app engine app.'
+    print '   Example: "https://skiadash.appspot.com/add_point".'
+    print '   If not set, will skip this step.'
     print '-b <bench> the bench to show.'
     print '-c <config> the config to show (GPU, 8888, 565, etc).'
     print '-d <dir> a directory containing bench_r<revision>_<scalar> files.'
     print '-e <file> file containing expected bench values/ranges.'
     print '   Will raise exception if actual bench values are out of range.'
     print '   See bench_expectations.txt for data format and examples.'
     print '-f <revision>[:<revision>] the revisions to use for fitting.'
     print '   Negative <revision> is taken as offset from most recent revision.'
     print '-i <time> the time to ignore (w, c, g, etc).'
     print '   The flag is ignored when -t is set; otherwise we plot all the'
@@ skipping 149 matching lines @@
     On Windows, we need to generate the absolute path within Python to avoid
     the operating system's 260-character pathname limit, including chdirs."""
     abs_path = get_abs_path(output_path)
     sys.stdout = open(abs_path, 'w')
 
 def create_lines(revision_data_points, settings
                , bench_of_interest, config_of_interest, time_of_interest
                , time_to_ignore):
     """Convert revision data into a dictionary of line data.
     
     ({int:[BenchDataPoints]}, {str:str}, str?, str?, str?)

[epoger, 2013/03/06 17:17:22]

What this means:

INPUT parameters of the function are:
1. a dictionary with integer keys (revision #) and a list of bench data points
as values
2. a dictionary of setting names to value
3-5. optional filter parameters: which bench, config, or timer type is of
interest.  For each one, if None, process them all.
6. optional timer type to ignore

OUTPUT is a dictionary of this form:
keys = Label objects
values = a list of (x, y) tuples sorted such that x values increase
monotonically

[benchen, 2013/03/06 18:52:37]

I've added your explanations assuming that bungyman would do so.
On 2013/03/06 17:17:22, epoger wrote:

         -> {Label:[(x,y)] | [n].x <= [n+1].x}"""
     revisions = revision_data_points.keys()
     revisions.sort()
     lines = {} # {Label:[(x,y)] | x[n] <= x[n+1]}
     for revision in revisions:
         for point in revision_data_points[revision]:
             if (bench_of_interest is not None and
                 not bench_of_interest == point.bench):
                 continue
             
@@ skipping 74 matching lines @@
         max_up_slope = max(max_up_slope, min_slope)
         min_down_slope = min(min_down_slope, min_slope)
     
     return (max_up_slope, min_down_slope)
 
 def main():
     """Parses command line and writes output."""
     
     try:
         opts, _ = getopt.getopt(sys.argv[1:]
-                                 , "b:c:d:e:f:i:l:m:o:r:s:t:x:y:"
+                                 , "a:b:c:d:e:f:i:l:m:o:r:s:t:x:y:"
                                  , "default-setting=")
     except getopt.GetoptError, err:
         print str(err) 
         usage()
         sys.exit(2)
     
     directory = None
     config_of_interest = None
     bench_of_interest = None
     time_of_interest = None
     time_to_ignore = None
     bench_expectations = {}
+    appengine_url = None  # used for adding data to appengine datastore
     rep = None  # bench representation algorithm
     revision_range = '0:'
     regression_range = '0:'
     latest_revision = None
     requested_height = None
     requested_width = None
     title = 'Bench graph'
     settings = {}
     default_settings = {}
 
@@ skipping 50 matching lines @@
                 bench_platform_key not in expectations):
                 # Skip benches without value for latest revision.
                 continue
             this_min, this_max = expectations[bench_platform_key]
             if this_bench_value < this_min or this_bench_value > this_max:
                 exceptions.append('Bench %s value %s out of range [%s, %s].' %
                     (bench_platform_key, this_bench_value, this_min, this_max))
         if exceptions:
             raise Exception('Bench values out of range:\n' +
                             '\n'.join(exceptions))
+    def write_to_appengine(lines, url, newest_revision, platform_and_alg):

[epoger, 2013/03/05 18:38:52]

please add newline before this function

[benchen, 2013/03/05 20:52:53]

Done.

+        """Writes latest bench values to appengine datastore."""

[epoger, 2013/03/05 18:38:52]

please document the parameters

[benchen, 2013/03/05 20:52:53]

Done.

+        data = []
+        for line in lines:
+            bot = platform_and_alg[ : platform_and_alg.rfind('-')]

[epoger, 2013/03/05 18:38:52]

Why are we assembling platform_and_alg in line 470 and then taking it apart
here?  Wouldn't it make more sense to just pass in the platform name WITHOUT
algorithm, if that's what we want?

And what happens if platform_and_alg does NOT start with TITLE_PREAMBLE (see
line 468)?  In that case, I guess there is no '-' in the string???

[benchen, 2013/03/05 20:52:53]

I did not separate platform and alg because there was no need to do so before.
You were right that now it's cleaner if we do. I used a separate var "bot" to
store the bot platform name.
On 2013/03/05 18:38:52, epoger wrote:

+            line_str = str(line)[ : str(line).find('_{')]

[epoger, 2013/03/05 18:38:52]

So, is this just trying to extract the portion of the line before the first "_{"
is found?  This seems like a needlessly complex way of doing it.

What is the desired behavior if "_{" is NOT found in the line?

[benchen, 2013/03/05 20:52:53]

create_lines() will make sure there is _{ in the line. Any suggesting on other
ways of stripping the string from _{ ?
On 2013/03/05 18:38:52, epoger wrote:

[epoger, 2013/03/06 17:17:22]

I don't see any mention of "_{" in create_lines.

Overall, I think I just need a better understanding of what the line_data_dict
really looks like.

[benchen, 2013/03/06 18:52:37]

According to Class Label's __str__ method, there's always a "_" before the
settings dict, so when stringify it there will always be "_{" separating
settings and the rest.
On 2013/03/06 17:17:22, epoger wrote:

+            if line_str.find('.skp') < 0 or not line_str.endswith('_'):
+                # filter out non-picture and non-walltime benches
+                continue
+            bench, config = line_str.split('.skp', 1)
+            config = config.strip('_')  # remove leading and trailing '_'
+            rev, val = lines[line][-1]

[epoger, 2013/03/05 18:38:52]

Is line a string or an index?

[benchen, 2013/03/05 20:52:53]

It's an object with Label class. Please see create_lines() written by bungyman
below.
On 2013/03/05 18:38:52, epoger wrote:

+            if rev != newest_revision:
+                continue
+            data.append({'master': 'Skia', 'bot': bot,
+                         'test': config + '/' + bench,
+                         'revision': rev, 'value': val, 'error': 0})
+        while data:
+          curr_data = data[ : DATA_POINTS_TO_SEND]

[epoger, 2013/03/05 18:38:52]

This seems like a really inefficient way to work our way through a dataset.  For
each batch, you're going to create 2 new arrays (curr_data and data) and copy
over a bunch of data points to each one.

Here's a recipe for doing it a faster/simpler way using itertools:
http://stackoverflow.com/a/3703710

[benchen, 2013/03/05 20:52:53]

Nice suggestion. Done.
On 2013/03/05 18:38:52, epoger wrote:

+          data = data[DATA_POINTS_TO_SEND : ]
+          req = urllib2.Request(
+              appengine_url, urllib.urlencode({'data': json.dumps(curr_data)}))
+          try:
+            urllib2.urlopen(req)
+          except urllib2.HTTPError, e:
+              sys.stderr.write("HTTPError: %d for JSON %s\n" % (e.code, data))

[epoger, 2013/03/05 18:38:52]

Why do you display the details of the exception differently in each case? 
(e.code for HTTPError, e.reason for URLError, nothing at all for HTTPException) 
Couldn't you just allow the underlying error to present its description as it
sees fit?

sys.stderr.write("HTTPError for JSON data %s: %s\n" % (data, e))

[benchen, 2013/03/05 20:52:53]

I was just copying from https://codereview.chromium.org/12317053/ to make it
consistent. Changed to your suggestion.

On 2013/03/05 18:38:52, epoger wrote:

+          except urllib2.URLError, e:
+              sys.stderr.write("URLError: %s for JSON %s\n" % (
+                  str(e.reason), data))
+          except httplib.HTTPException, e:
+              sys.stderr.write("HTTPException for JSON %s\n" % lines)
 
     try:
         for option, value in opts:
-            if option == "-b":
+            if option == "-a":
+                appengine_url = value
+            elif option == "-b":
                 bench_of_interest = value
             elif option == "-c":
                 config_of_interest = value
             elif option == "-d":
                 directory = value
             elif option == "-e":
                 read_expectations(bench_expectations, value)
             elif option == "-f":
                 regression_range = value
             elif option == "-i":
@@ skipping 68 matching lines @@
                    , time_of_interest
                    , time_to_ignore)
 
     regressions = create_regressions(lines
                                    , oldest_regression
                                    , newest_regression)
 
     output_xhtml(lines, oldest_revision, newest_revision, ignored_revision_data_points,
                  regressions, requested_width, requested_height, title)
 
+    if appengine_url:
+        write_to_appengine(lines, appengine_url, newest_revision,
+                           platform_and_alg)
+
     check_expectations(lines, bench_expectations, newest_revision,
                        platform_and_alg)
 
 def qa(out):
     """Stringify input and quote as an xml attribute."""
     return xml.sax.saxutils.quoteattr(str(out))
 def qe(out):
     """Stringify input and escape as xml data."""
     return xml.sax.saxutils.escape(str(out))
 
@@ skipping 481 matching lines @@
     print '<a id="rev_link" xlink:href="" target="_top">'
     print '<text id="revision" x="0" y=%s style="' % qa(font_size*2)
     print 'font-size: %s; ' % qe(font_size)
     print 'stroke: #0000dd; text-decoration: underline; '
     print '"> </text></a>'
 
     print '</svg>'
 
 if __name__ == "__main__":
     main()