Smarter "new interesting code" detection.

tools/testing/perf_testing/run_perf_tests.py

 #!/usr/bin/python
 
 # Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 # for details. All rights reserved. Use of this source code is governed by a
 # BSD-style license that can be found in the LICENSE file.
 
 import datetime
 import math             
 import optparse
 import os
@@ skipping 91 matching lines @@
     """Wrapper around the pulling down a specific archive from Google Storage.
     Adds a specific revision argument as needed.
     Returns: The stderr from running this command."""
     cmd = ['python', os.path.join(DART_REPO_LOC, 'tools', 'get_archive.py'),
            archive_name]
     if self.current_revision_num != -1:
       cmd += ['-r', self.current_revision_num]
     _, stderr = self.run_cmd(cmd)
     return stderr
 
-  def sync_and_build(self, suites, revision_num=''):
-    """Make sure we have the latest version of of the repo, and build it. We
-    begin and end standing in DART_REPO_LOC.
-
-    Args:
-      suites: The set of suites that we wish to build.
-
-    Returns:
-      err_code = 1 if there was a problem building."""
+  def _sync(self, revision_num=''):

[sra1, 2012/08/18 23:08:55]

nit: we would usually use None for the default value and test with  revision_num
is None

[Emily Fortuna, 2012/08/20 19:59:05]

Done.

+    """Update the repository to the latest or specified revision."""
     os.chdir(dirname(DART_REPO_LOC))
     self.clear_out_unversioned_files()
     if revision_num == '':
       self.run_cmd(['gclient', 'sync'])
     else:
       self.run_cmd(['gclient', 'sync', '-r', revision_num, '-t'])
     
     shutil.copytree(os.path.join(TOP_LEVEL_DIR, 'internal'),
                     os.path.join(DART_REPO_LOC, 'internal'))
     shutil.copy(os.path.join(TOP_LEVEL_DIR, 'tools', 'get_archive.py'),
                     os.path.join(DART_REPO_LOC, 'tools', 'get_archive.py'))
     shutil.copy(
         os.path.join(TOP_LEVEL_DIR, 'tools', 'testing', 'run_selenium.py'),
         os.path.join(DART_REPO_LOC, 'tools', 'testing', 'run_selenium.py'))
 
+  def sync_and_build(self, suites, revision_num=''):

[sra1, 2012/08/18 23:08:55]

Google style is to use MixedCase for functions and methods.

[Emily Fortuna, 2012/08/20 19:59:05]

Not for methods: 
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Naming...

and there seems to be some vagueness here:
http://www.chromium.org/developers/coding-style

[sra1, 2012/08/21 22:59:36]

The second link says "Python code should follow PEP-8, except that Chromium uses
two-space indentation instead of four-space indentation, and it uses MixedCase
for method names and function names instead of lower_case_with_underscores."

[Emily Fortuna, 2012/08/22 21:08:20]

Yea, I was reading the wrong section of the style guide like we discussed.
Fixed.

+    """Make sure we have the latest version of of the repo, and build it. We
+    begin and end standing in DART_REPO_LOC.
+
+    Args:
+      suites: The set of suites that we wish to build.
+
+    Returns:
+      err_code = 1 if there was a problem building."""
+    self._sync(revision_num)
     if revision_num == '':
       revision_num = search_for_revision()
 
     self.current_revision_num = revision_num
     stderr = self.get_archive('sdk')
     if not os.path.exists(os.path.join(
         DART_REPO_LOC, 'tools', 'get_archive.py')) \

[sra1, 2012/08/18 23:08:55]

Better style is to put the whole condition in parens to avoid the backslash.

[Emily Fortuna, 2012/08/20 19:59:05]

Done.

         or 'InvalidUriError' in stderr:
       # Couldn't find the SDK on Google Storage. Build it locally.
 
       # On Windows, the output directory is marked as "Read Only," which causes
       # an error to be thrown when we use shutil.rmtree. This helper function
       # changes the permissions so we can still delete the directory.
       def on_rm_error(func, path, exc_info):
         if os.path.exists(path):
           os.chmod(path, stat.S_IWRITE)
           os.unlink(path)
@@ skipping 21 matching lines @@
     Args:
       dir_name: the directory we will create if it does not exist."""
     dir_path = os.path.join(TOP_LEVEL_DIR, 'tools', 
                             'testing', 'perf_testing', dir_name)
     if not os.path.exists(dir_path):
       os.makedirs(dir_path)
       print 'Creating output directory ', dir_path
 
   def has_interesting_code(self, past_revision_num=None):
     """Tests if there are any versions of files that might change performance
-    results on the server."""
+    results on the server.
+    Returns a tuple of a boolean and potentially a revision number that has
+    interesting code. (If the boolean is false, there is no interesting code,
+    therefore the second element in the tuple will be None.)"""
     if not os.path.exists(DART_REPO_LOC):
-      return True
+      self._sync()
     os.chdir(DART_REPO_LOC)
     no_effect = ['client', 'compiler', 'editor', 'pkg', 'samples', 'tests',
-                 'third_party', 'tools', 'utils']
-    # Pass 'p' in if we have a new certificate for the svn server, we want to
-    # (p)ermanently accept it.
-    if past_revision_num:
+                 'third_party', 'utils']
+    def get_svn_log_data(revision):

[sra1, 2012/08/18 23:08:55]

The function is not really getting log data.  it is getting some partially
processed lines from the log file.

I would change this to two totally reusable global functions (they don't need
any TestRunner state)

(1) a small function that reads the svn log text for a revision
(2) another that converts the text into a clean list of filenames, including the
/branches/ prefix.

You can test (2) on string literals (you do have unit tests somewhere, don't
you?)

[Emily Fortuna, 2012/08/20 19:59:05]

Made into a global function. No unit tests for this (yet). :-(

+      """Determine the set of files that were changed for a particular
+      revision."""
       # TODO(efortuna): This assumes you're using svn. Have a git fallback as
       # well.
+      # Pass 'p' in if we have a new certificate for the svn server, we want to
+      # (p)ermanently accept it.
       results, _ = self.run_cmd(['svn', 'log', '-v', '-r',
-                                 str(past_revision_num)], std_in='p\r\n')
+                                 str(revision)], std_in='p\r\n')
       results = results.split('\n')
       if len(results) <= 3:
-        results = []
+        return []
       else:
         # Trim off the details about revision number and commit message. We're
         # only interested in the files that are changed.
         results = results[3:]
         changed_files = []
         for result in results:
-          if result == '':
+          if len(result) <= 1:
             break
           changed_files += [result.replace('/branches/bleeding_edge/dart/', '')]

[sra1, 2012/08/18 23:08:55]

Don't strip of 'dart', you don't want to miss /branches/bleeding_edge/deps/...

Some log files contain extra per-file info:
   A
/branches/bleeding_edge/dart/editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/parser/RecoveryParserTest.java
(from
/branches/bleeding_edge/dart/editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/parser/ParserRecoveryTest.java:9962)

It is probably simplest to pick out the text from /branches/ to the end of the
filename (space or $) with a regexp

[Emily Fortuna, 2012/08/20 19:59:05]

Sort-of-done (with a TODO)... This requires me to have a different checkout of
the repo instead of "gclient config
https://dart.googlecode.com/svn/branches/bleeding_edge/deps/all.deps" and then
"gclient sync". What's the proper method for the different checkout?

-        results = changed_files
+        return changed_files
+
+    def has_perf_affecting_results(results):

[sra1, 2012/08/18 23:08:55]

'results' is kind of generic.
Is this a list of lines, or a list of filenames?

[Emily Fortuna, 2012/08/20 19:59:05]

Done.

+      """Determine if this set of changed files might effect performance
+      tests."""
+      for line in results:
+        tokens = line.split()
+        if len(tokens) >= 1:
+          # Loop through the changed files to see if it contains any files that
+          # are NOT listed in the no_effect list (directories not listed in
+          # the "no_effect" list are assumed to potentially affect performance.
+          if not reduce(lambda x, y: x or y, 
+              [tokens[-1].startswith(item) for item in no_effect], False):

[sra1, 2012/08/18 23:08:55]

If the filenames are cleanly extracted, and the no_effect list has the full
/branches/... prefix, this becomes much simpler:


def IsSafeFile(file):
  return any(file.startswith(prefix) for prefix in no_effect)
return not all(IsSafeFile(file) for file in files)

[Emily Fortuna, 2012/08/20 19:59:05]

Done.

+            return True
+      return False
+
+    if past_revision_num:
+      return (has_perf_affecting_results(get_svn_log_data(past_revision_num)),
+              past_revision_num)
     else:
       results, _ = self.run_cmd(['svn', 'st', '-u'], std_in='p\r\n')
-      results = results.split('\n')
-    for line in results:
-      tokens = line.split()
-      if past_revision_num or len(tokens) >= 3 and '*' in tokens[-3]:
-        # Loop through the changed files to see if it contains any files that
-        # are NOT listed in the no_effect list (directories not listed in
-        # the "no_effect" list are assumed to potentially affect performance.
-        if not reduce(lambda x, y: x or y, 
-            [tokens[-1].startswith(item) for item in no_effect], False):
-          return True
-    return False
+      latest_interesting_server_rev = int(results.split('\n')[-2].split()[-1])
+      done_cls = list(update_set_of_done_cls())
+      done_cls.sort()
+      if len(done_cls) == 0:

[sra1, 2012/08/18 23:08:55]

idoimatic python tests for non-empty list like this:

if done_cls:
  last_done_cl = int(done_cls[-1])
else:
  last_done_cl = EARLIEST_REVISION

[Emily Fortuna, 2012/08/20 19:59:05]

Done.

+        last_done_cl = EARLIEST_REVISION
+      else:
+        last_done_cl = int(done_cls[-1])
+      
+      while latest_interesting_server_rev >= last_done_cl:
+        results = get_svn_log_data(latest_interesting_server_rev)
+        if has_perf_affecting_results(results):
+          return (True, latest_interesting_server_rev)
+        else:
+          update_set_of_done_cls(latest_interesting_server_rev)
+        latest_interesting_server_rev -= 1
+      return (False, None)
     
   def get_os_directory(self):
     """Specifies the name of the directory for the testing build of dart, which
     has yet a different naming convention from utils.getBuildRoot(...)."""
     if platform.system() == 'Windows':
       return 'windows'
     elif platform.system() == 'Darwin':
       return 'macos'
     else:
       return 'linux'
@@ skipping 231 matching lines @@
     Returns: True if the post was successful file."""
     return post_results.report_results(benchmark_name, score, platform, variant,
                                        revision_number, metric)
 
   def calculate_geometric_mean(self, platform, variant, svn_revision):
     """Calculate the aggregate geometric mean for JS and dart2js benchmark sets,
     given two benchmark dictionaries."""
     geo_mean = 0                
     if self.test.is_valid_combination(platform, variant):
       for benchmark in self.test.values_list:
+        if len(self.test.values_dict[platform][variant][benchmark]) == 0:

[sra1, 2012/08/18 23:08:55]

len(x) == 0 --> not x
if not self.test.values_dict[platform][variant][benchmark]:

[Emily Fortuna, 2012/08/20 19:59:05]

Done.

+          print 'Error determining mean for %s %s %s' % (platform, variant,
+                                                         benchmark)
+          continue
         geo_mean += math.log(
             self.test.values_dict[platform][variant][benchmark][
                 len(self.test.values_dict[platform][variant][benchmark]) - 1])

[sra1, 2012/08/18 23:08:55]

Python has negative indexes from the end, so indexing by -1 give the last
element.

x[len(x)-1]  -->  x[-1]

[Emily Fortuna, 2012/08/20 19:59:05]

Done.

 
     self.test.values_dict[platform][variant]['Geo-Mean'] += \
         [math.pow(math.e, geo_mean / len(self.test.values_list))]
     self.test.revision_dict[platform][variant]['Geo-Mean'] += [svn_revision]
 
   def get_score_type(self, benchmark_name):
     """Determine the type of score for posting -- default is 'Score' (aka
     Runtime), other options are CompileTime and CodeSize."""
     return self.SCORE
 
@@ skipping 25 matching lines @@
           file_processor)
     self.platform_list = platform_list
     self.platform_type = platform_type
     self.versions = versions
     self.benchmarks = benchmarks 
 
 
 class BrowserTester(Tester):
   @staticmethod
   def get_browsers(add_dartium=True):
-    browsers = []#['ff', 'chrome']
+    browsers = ['ff', 'chrome']
     if add_dartium:
       browsers += ['dartium']
     has_shell = False
     if platform.system() == 'Darwin':
       browsers += ['safari']
     if platform.system() == 'Windows':
       browsers += ['ie']
       has_shell = True
     return browsers
 
@@ skipping 415 matching lines @@
   has_run_extra = False
   revision_num = int(search_for_revision(DART_REPO_LOC))
 
   def try_to_run_additional(revision_number):
     """Determine the number of results we have stored for a particular revision
     number, and if it is less than 10, run some extra tests.
     Args: 
       - revision_number: the revision whose performance we want to potentially
         test.
     Returns: True if we successfully ran some additional tests."""
-    if not runner.has_interesting_code(revision_number):
+    if not runner.has_interesting_code(revision_number)[0]:
       results_set = update_set_of_done_cls(revision_number)
       return False
     a_test = TestBuilder.make_test(runner.suite_names[0], runner)
     benchmark_name = a_test.values_list[0]
     platform_name = a_test.platform_list[0]
     variant = a_test.values_dict[platform_name].keys()[0]
     num_results = post_results.get_num_results(benchmark_name,
         platform_name, variant, revision_number,
         a_test.file_processor.get_score_type(benchmark_name))
     if num_results < 10:
@@ skipping 17 matching lines @@
     # already has 10 runs, look for another CL number that is not yet have all
     # of its additional runs (do this up to 15 times).
     tries = 0
     # Select which "thousands bucket" we're going to run additional tests for.
     bucket_size = 1000
     thousands_list = range(EARLIEST_REVISION/bucket_size,
                            int(revision_num)/bucket_size + 1)
     weighted_total = sum(thousands_list)
     generated_random_number = random.randint(0, weighted_total - 1)
     for i in list(reversed(thousands_list)):
-      thousands = thousands_list[i - 1]
-      weighted_total -= thousands_list[i - 1]
+      thousands = i
+      weighted_total -= i
       if weighted_total <= generated_random_number:
         break
     while tries < 15 and not has_run_extra:
       # Now select a particular revision in that bucket.
       if thousands == int(revision_num)/bucket_size:
         max_range = 1 + revision_num % bucket_size
       else:
         max_range = bucket_size
       rev = thousands * bucket_size + random.randrange(0, max_range)
       if rev not in results_set:
@@ skipping 22 matching lines @@
     os.mkdir(dirname(DART_REPO_LOC))
     os.chdir(dirname(DART_REPO_LOC))
     p = subprocess.Popen('gclient config https://dart.googlecode.com/svn/' +
                          'branches/bleeding_edge/deps/all.deps',
                          stdout=subprocess.PIPE, stderr=subprocess.PIPE,
                          shell=True)
     p.communicate()
   if continuous:
     while True:
       results_set = update_set_of_done_cls()
-      if runner.has_interesting_code():
-        runner.run_test_sequence()
+      interesting_code_results = runner.has_interesting_code()
+      if interesting_code_results[0]:
+        runner.run_test_sequence(interesting_code_results[1])
       else:
         results_set = fill_in_back_history(results_set, runner)
   else:
     runner.run_test_sequence()
 
 if __name__ == '__main__':
   main()