Extensions Docs Server: Large performance increase

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

Index: chrome/common/extensions/docs/server2/file_system_cache.py
diff --git a/chrome/common/extensions/docs/server2/file_system_cache.py b/chrome/common/extensions/docs/server2/file_system_cache.py
index a38df42f3144f38d976663726606525c7d513dc6..c148810543adef645de3537365b0994f3db82940 100644
--- a/chrome/common/extensions/docs/server2/file_system_cache.py
+++ b/chrome/common/extensions/docs/server2/file_system_cache.py
@@ -4,27 +4,51 @@
 
 import os
 
+import appengine_memcache as memcache
+
+FS_CACHE_CRON = 'FileSystemCache.Cron'
+FS_CACHE_RENDER = 'FileSystemCache.Render'
+FS_CACHE_PERMS = 'FileSystemCache.Perms'
+FS_CACHE_JSON = 'FileSystemCache.JSON'
+FS_CACHE_IDL = 'FileSystemCache.IDL'
+FS_CACHE_LIST = 'FileSystemCache.List'
+FS_CACHE_ZIP = 'FileSystemCache.Zip'
+FS_CACHE_INTRO = 'FileSystemCache.Intro'
+FS_CACHE_EXTENSIONS = 'FileSystemCache.Extensions'
+FS_CACHE_APPS = 'FileSystemCache.Apps'
+FS_CACHE_STATIC = 'FileSystemCache.Static'
+FS_CACHE_HANDLEBAR = 'FileSystemCache.Handlebar'

[not at google - send to devlin, 2012/08/20 05:27:10]

nit: alphabet

extra points (extra nitty): align the = vertically, too.

Although... it seems to me like the FS_CACHE and the FileSystemCache. prefixes
are unnecessary; FS_CACHE is implied (because these are in that module), and
FileSystemCache. should be prepended to the namespace on construction of the
FileSystemCache.

Or this should go in AppengineMemcache, or, that should be updated to be
similar. Something consistent.

[cduvall, 2012/08/20 21:28:09]

Done.

+
+class _CacheEntry(object):
+  def __init__(self, cache_data, version):
+    self._cache_data = cache_data
+    self.version = version
+
 class FileSystemCache(object):

[not at google - send to devlin, 2012/08/20 05:27:10]

I reckon we should rename this (as in my email) to CompiledFileSystem or
CompilingFileSystem or TypedFileSystem. The purpose of it has changed.

... but after this patch goes in. No need to add extra noise.

   """This class caches FileSystem data that has been processed.
   """
   class Builder(object):

[not at google - send to devlin, 2012/08/20 05:27:10]

ditto (after this patch): rename to Factory

     """A class to build a FileSystemCache.
     """
-    def __init__(self, file_system):
+    def __init__(self, file_system, memcache):
       self._file_system = file_system
+      self._memcache = memcache
 
-    def build(self, populate_function):
-      return FileSystemCache(self._file_system, populate_function)
+    def build(self, populate_function, namespace):

[not at google - send to devlin, 2012/08/20 05:27:10]

ditto (after this patch): rename to Create

+      return FileSystemCache(self._file_system,
+                             populate_function,
+                             self._memcache,
+                             namespace)
 
-  class _CacheEntry(object):
-    def __init__(self, cache_data, version):
-      self._cache_data = cache_data
-      self.version = version
-
-  def __init__(self, file_system, populate_function):
+  def __init__(self, file_system, populate_function, memcache, namespace):
     self._file_system = file_system
     self._populate_function = populate_function
     self._cache = {}

[not at google - send to devlin, 2012/08/20 05:27:10]

not used anymore?

[cduvall, 2012/08/20 21:28:09]

Done.

+    self._memcache = memcache
+    self._namespace = namespace
+
+  def _MakeKey(self, key):
+    return self._namespace + '.' + key
 
   def _RecursiveList(self, files):
     all_files = files[:]
@@ -41,15 +65,22 @@ class FileSystemCache(object):
     """Calls |populate_function| on the contents of the file at |path|.
     """
     version = self._file_system.Stat(path).version
-    if path in self._cache:
-      if version > self._cache[path].version:
-        self._cache.pop(path)
+    cache_entry = self._memcache.Get(self._MakeKey(path),
+                                     memcache.MEMCACHE_FILE_SYSTEM_CACHE,
+                                     time=0)
+
+    if  cache_entry is not None:

[not at google - send to devlin, 2012/08/20 05:27:10]

nit: extra space

[cduvall, 2012/08/20 21:28:09]

Done.

+      if version != cache_entry.version:
+        self._memcache.Delete(self._MakeKey(path),
+                              memcache.MEMCACHE_FILE_SYSTEM_CACHE)

[not at google - send to devlin, 2012/08/20 05:27:10]

I don't think we need to Delete this, we Set it right afterwards?

[cduvall, 2012/08/20 21:28:09]

Done.

       else:
-        return self._cache[path]._cache_data
-    cache_data = self._file_system.ReadSingle(path)
-    self._cache[path] = self._CacheEntry(self._populate_function(cache_data),
-                                         version)
-    return self._cache[path]._cache_data
+        return cache_entry._cache_data
+    cache_data = self._populate_function(self._file_system.ReadSingle(path))
+    self._memcache.Set(self._MakeKey(path),
+                       _CacheEntry(cache_data, version),
+                       memcache.MEMCACHE_FILE_SYSTEM_CACHE,
+                       time=0)
+    return cache_data
 
   def GetFromFileListing(self, path):
     """Calls |populate_function| on the listing of the files at |path|.
@@ -58,13 +89,20 @@ class FileSystemCache(object):
     if not path.endswith('/'):
       path += '/'
     version = self._file_system.Stat(path).version
-    if path in self._cache:
-      if version > self._cache[path].version:
-        self._cache.pop(path)
+    cache_entry = self._memcache.Get(
+        self._MakeKey(path),
+        memcache.MEMCACHE_FILE_SYSTEM_CACHE_LISTING,
+        time=0)
+    if cache_entry is not None:
+      if version != cache_entry.version:
+        self._memcache.Delete(self._MakeKey(path),
+                              memcache.MEMCACHE_FILE_SYSTEM_CACHE_LISTING)
       else:
-        return self._cache[path]._cache_data
-    cache_data = self._RecursiveList(
-        [path + f for f in self._file_system.ReadSingle(path)])
-    self._cache[path] = self._CacheEntry(self._populate_function(cache_data),
-                                         version)
-    return self._cache[path]._cache_data
+        return cache_entry._cache_data
+    cache_data = self._populate_function(self._RecursiveList(
+        [path + f for f in self._file_system.ReadSingle(path)]))
+    self._memcache.Set(self._MakeKey(path),
+                       _CacheEntry(cache_data, version),
+                       memcache.MEMCACHE_FILE_SYSTEM_CACHE_LISTING,
+                       time=0)
+    return cache_data