rebaseline_server: add "prefetch" directive that just warms the cache without awaiting results

gm/rebaseline_server/imagediffdb.py

 #!/usr/bin/python
 
 """
 Copyright 2013 Google Inc.
 
 Use of this source code is governed by a BSD-style license that can be
 found in the LICENSE file.
 
 Calulate differences between image pairs, and store them in a database.
 """
@@ skipping 83 matching lines @@
     expected_image_locator = _sanitize_locator(expected_image_locator)
     actual_image_locator = _sanitize_locator(actual_image_locator)
 
     # Download the expected/actual images, if we don't have them already.
     expected_image_file = os.path.join(
         storage_root, expected_images_subdir,
         str(expected_image_locator) + image_suffix)
     actual_image_file = os.path.join(
         storage_root, actual_images_subdir,
         str(actual_image_locator) + image_suffix)
-    try:
-      _download_file(gs, expected_image_file, expected_image_url)
-    except Exception:
-      logging.exception('unable to download expected_image_url %s to file %s' %
-                        (expected_image_url, expected_image_file))
-      raise
-    try:
-      _download_file(gs, actual_image_file, actual_image_url)
-    except Exception:
-      logging.exception('unable to download actual_image_url %s to file %s' %
-                        (actual_image_url, actual_image_file))
-      raise
+    for image_file, image_url in [
+        (expected_image_file, expected_image_url),
+        (actual_image_file, actual_image_url)]:
+      if image_file and image_url:
+        try:
+          _download_file(gs, image_file, image_url)
+        except Exception:
+          logging.exception('unable to download image_url %s to file %s' %
+                            (image_url, image_file))
+          raise
 
-    # Get all diff images and values from skpdiff binary.
+    # Return early if we do not need to generate diffs.
+    if (expected_image_url == actual_image_url or
+        not expected_image_url or not actual_image_url):
+      return
+
+    # Get all diff images and values using the skpdiff binary.
     skpdiff_output_dir = tempfile.mkdtemp()
     try:
       skpdiff_summary_file = os.path.join(skpdiff_output_dir,
                                           'skpdiff-output.json')
       skpdiff_rgbdiff_dir = os.path.join(skpdiff_output_dir, 'rgbDiff')
       skpdiff_whitediff_dir = os.path.join(skpdiff_output_dir, 'whiteDiff')
-      expected_img = os.path.join(storage_root, expected_images_subdir,
-                                  str(expected_image_locator) + image_suffix)
-      actual_img = os.path.join(storage_root, actual_images_subdir,
-                                str(actual_image_locator) + image_suffix)
 
       # TODO(epoger): Consider calling skpdiff ONCE for all image pairs,
       # instead of calling it separately for each image pair.
       # Pro: we'll incur less overhead from making repeated system calls,
       # spinning up the skpdiff binary, etc.
       # Con: we would have to wait until all image pairs were loaded before
       # generating any of the diffs?
       find_run_binary.run_command(
-          [SKPDIFF_BINARY, '-p', expected_img, actual_img,
+          [SKPDIFF_BINARY, '-p', expected_image_file, actual_image_file,
            '--jsonp', 'false',
            '--output', skpdiff_summary_file,
            '--differs', 'perceptual', 'different_pixels',
            '--rgbDiffDir', skpdiff_rgbdiff_dir,
            '--whiteDiffDir', skpdiff_whitediff_dir,
            ])
 
       # Get information out of the skpdiff_summary_file.
       with contextlib.closing(open(skpdiff_summary_file)) as fp:
         data = json.load(fp)
@@ skipping 107 matching lines @@
     # previously downloaded to local disk.
     # I guess we should figure out how expensive it is to download vs diff the
     # image pairs... if diffing them is expensive too, we can write these
     # _diff_dict objects out to disk if there's too many to hold in RAM.
     # Or we could use virtual memory to handle that automatically.
     self._diff_dict = {}
     self._diff_dict_writelock = threading.RLock()
 
     # Set up the queue for asynchronously loading DiffRecords, and start the
     # worker threads reading from it.
-    self._tasks_queue = Queue.Queue(maxsize=2*num_worker_threads)
+    # The queue maxsize must be 0 (infinite size queue), so that asynchronous
+    # calls can return as soon as possible.
+    self._tasks_queue = Queue.Queue(maxsize=0)
     self._workers = []
     for i in range(num_worker_threads):
       worker = threading.Thread(target=self.worker, args=(i,))
       worker.daemon = True
       worker.start()
       self._workers.append(worker)
 
   def worker(self, worker_num):
     """Launch a worker thread that pulls tasks off self._tasks_queue.
 
@@ skipping 28 matching lines @@
                      expected_image_url, expected_image_locator,
                      actual_image_url, actual_image_locator):
     """Asynchronously prepare a DiffRecord for a pair of images.
 
     This method will return quickly; calls to get_diff_record() will block
     until the DiffRecord is available (or we have given up on creating it).
 
     If we already have a DiffRecord for this particular image pair, no work
     will be done.
 
+    If expected_image_url (or its locator) is None, just download actual_image.
+    If actual_image_url (or its locator) is None, just download expected_image.
+
     Args:
       expected_image_url: file, GS, or HTTP url from which we will download the
           expected image
       expected_image_locator: a unique ID string under which we will store the
           expected image within storage_root (probably including a checksum to
           guarantee uniqueness)
       actual_image_url: file, GS, or HTTP url from which we will download the
           actual image
       actual_image_locator: a unique ID string under which we will store the
           actual image within storage_root (probably including a checksum to
@@ skipping 107 matching lines @@
       pass
 
 
 def _sanitize_locator(locator):
   """Returns a sanitized version of a locator (one in which we know none of the
   characters will have special meaning in filenames).
 
   Args:
     locator: string, or something that can be represented as a string
   """
-  return DISALLOWED_FILEPATH_CHAR_REGEX.sub('_', str(locator))
-
+  if locator:
+    return DISALLOWED_FILEPATH_CHAR_REGEX.sub('_', str(locator))
+  else:
+    return locator

[rmistry, 2014/08/06 17:49:01]

Is it ok for this to be an empty string or None? should this throw an exception
instead?

[epoger, 2014/08/06 17:53:34]

We actually need to allow for None.  Added a comment explaining why.

 
 def _get_difference_locator(expected_image_locator, actual_image_locator):
   """Returns the locator string used to look up the diffs between expected_image
   and actual_image.
 
   We must keep this function in sync with getImageDiffRelativeUrl() in
   static/loader.js
 
   Args:
     expected_image_locator: locator string pointing at expected image
     actual_image_locator: locator string pointing at actual image
 
   Returns: already-sanitized locator where the diffs between expected and
       actual images can be found
   """
   return "%s-vs-%s" % (_sanitize_locator(expected_image_locator),
                        _sanitize_locator(actual_image_locator))