Codes for writing bench data to appengine datastore.

bench/bench_graph_svg.py

Index: bench/bench_graph_svg.py
===================================================================
--- bench/bench_graph_svg.py	(revision 7965)
+++ bench/bench_graph_svg.py	(working copy)
@@ -9,6 +9,9 @@
 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.
@@ -19,9 +22,15 @@
 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.'
@@ -286,7 +295,7 @@
     
     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) 
@@ -299,6 +308,7 @@
     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:'
@@ -369,10 +379,43 @@
         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
@@ -461,6 +504,10 @@
     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)