Docserver: Override Walk in CachingFileSystem

chrome/common/extensions/docs/server2/compiled_file_system.py

Index: chrome/common/extensions/docs/server2/compiled_file_system.py
diff --git a/chrome/common/extensions/docs/server2/compiled_file_system.py b/chrome/common/extensions/docs/server2/compiled_file_system.py
index 2d329c0a2efa94ad47607ea1da48c144c351075f..2ba1329a5b7ba446fd3794de8fe5b1b4718815fa 100644
--- a/chrome/common/extensions/docs/server2/compiled_file_system.py
+++ b/chrome/common/extensions/docs/server2/compiled_file_system.py
@@ -14,9 +14,30 @@ from third_party.json_schema_compiler.memoize import memoize
 from third_party.motemplate import Motemplate
 
 
+_CACHEABLE_FUNCTIONS = set()
 _SINGLE_FILE_FUNCTIONS = set()
 
 
+def _GetUnboundFunction(fn):
+  '''Functions bound to an object are separate from the unbound

[ahernandez, 2014/08/29 01:49:42]

I don't know how to explain this concisely, but the SingleFile annotation wasn't
actually doing it's job in a lot of cases because when a method is annotated,
the unbound version of it is passed to the annotation function, e.g. SingleFile.
But then the bound version is passed to compiled fs, e.g.
c_fs_factory.Create(self._comp_func, ...), so when checking if the compilation
function is in the set of single file functions, the test fails because the
bound method != the unbound method. So I wrote this to ensure that the unbound
version is always grabbed.

[not at google - send to devlin, 2014/08/29 05:00:14]

Neat, nice catch. This nested decorator stuff in Python always messed me up.

+  defintion. This causes issues when checking for membership,
+  so always get the unbound function, if possible.
+  '''
+  return getattr(fn, 'im_func', fn)
+
+
+def Cache(fn):
+  '''A decorator which can be applied to the compilation function
+  passed to CompiledFileSystem.Create, indicating that file/list data
+  should be cached.
+
+  This decorator should be listed first in any list of decorators, along
+  with the SingleFile decorator below.
+  '''
+  _CACHEABLE_FUNCTIONS.add(_GetUnboundFunction(fn))
+  return fn
+
+
 def SingleFile(fn):
   '''A decorator which can be optionally applied to the compilation function
   passed to CompiledFileSystem.Create, indicating that the function only
@@ -26,7 +47,7 @@ def SingleFile(fn):
   Note that this decorator must be listed first in any list of decorators to
   have any effect.
   '''
-  _SINGLE_FILE_FUNCTIONS.add(fn)
+  _SINGLE_FILE_FUNCTIONS.add(_GetUnboundFunction(fn))
   return fn
 
 
@@ -100,8 +121,8 @@ class CompiledFileSystem(object):
       These are memoized over file systems tied to different branches.
       '''
       return self.Create(file_system,
-                         SingleFile(lambda _, data:
-                             json_parse.Parse(ToUnicode(data))),
+                         Cache(SingleFile(lambda _, data:

[not at google - send to devlin, 2014/08/29 05:00:14]

JSON should be ... fairly cheap? Very commonly used, though, hm.

Could you do some benchmarking perhaps? Starting from nothing Cached and start
adding back in things?

cron_servlet_test.py is actually pretty good - not only does the time running
reflect well on the pure computational time it tries to do this, but if you run
"top" at the same time you can see how the memory usage is affected.

(I expect that Motemplate compilation will be quite expensive, but, prove me
wrong!).

+                             json_parse.Parse(ToUnicode(data)))),
                          CompiledFileSystem,
                          category='json')
 
@@ -134,6 +155,27 @@ class CompiledFileSystem(object):
     self._file_object_store = file_object_store
     self._list_object_store = list_object_store
 
+  def _GetFromStore(self, key, store):
+    if _GetUnboundFunction(self._compilation_function) in _CACHEABLE_FUNCTIONS:
+      return store.Get(key)
+    return Future(value=None)
+
+  def _SetToStore(self, key, value, store):
+    if _GetUnboundFunction(self._compilation_function) in _CACHEABLE_FUNCTIONS:
+      store.Set(key, value)
+
+  def _GetFromFileStore(self, key):
+    return self._GetFromStore(key, self._file_object_store)
+
+  def _GetFromListStore(self, key):
+    return self._GetFromStore(key, self._list_object_store)
+
+  def _SetToFileStore(self, key, value):
+    self._SetToStore(key, value, self._file_object_store)
+
+  def _SetToListStore(self, key, value):
+    self._SetToStore(key, value, self._list_object_store)
+
   def _RecursiveList(self, path):
     '''Returns a Future containing the recursive directory listing of |path| as
     a flat list of paths.
@@ -199,13 +241,13 @@ class CompiledFileSystem(object):
       else:
         return Future(exc_info=sys.exc_info())
 
-    cache_entry = self._file_object_store.Get(path).Get()
+    cache_entry = self._GetFromFileStore(path).Get()

[not at google - send to devlin, 2014/08/29 05:00:14]

These wrapper functions are nice, but if you had static _Get/_Set methods it
wouldn't be too bad to write just:

cache_entry = _Get(self._file_object_store, path).Get()

Ditto _Set. Then we wouldn't need to add quite so many lines.

     if (cache_entry is not None) and (version == cache_entry.version):
       return Future(value=cache_entry._cache_data)
 
     def compile_(files):
       cache_data = self._compilation_function(path, files)
-      self._file_object_store.Set(path, _CacheEntry(cache_data, version))
+      self._SetToFileStore(path, _CacheEntry(cache_data, version))

[not at google - send to devlin, 2014/08/29 05:00:14]

(FWIW I think that "set in" reads slightly better than "set to").

       return cache_data
 
     return self._file_system.ReadSingle(
@@ -222,25 +264,25 @@ class CompiledFileSystem(object):
     except FileNotFoundError:
       return Future(exc_info=sys.exc_info())
 
-    cache_entry = self._list_object_store.Get(path).Get()
+    cache_entry = self._GetFromListStore(path).Get()
     if (cache_entry is not None) and (version == cache_entry.version):
       return Future(value=cache_entry._cache_data)
 
     def next(files):
       cache_data = self._compilation_function(path, files)
-      self._list_object_store.Set(path, _CacheEntry(cache_data, version))
+      self._SetToListStore(path, _CacheEntry(cache_data, version))
       return cache_data
     return self._RecursiveList(path).Then(next)
 
   def GetFileVersion(self, path):
-    cache_entry = self._file_object_store.Get(path).Get()
+    cache_entry = self._GetFromFileStore(path).Get()
     if cache_entry is not None:
       return cache_entry.version
     return self._file_system.Stat(path).version
 
   def GetFileListingVersion(self, path):
     path = ToDirectory(path)
-    cache_entry = self._list_object_store.Get(path).Get()
+    cache_entry = self._GetFromListStore(path).Get()
     if cache_entry is not None:
       return cache_entry.version
     return self._file_system.Stat(path).version