Docserver: Add support for viewing docs with a codereview patch applied

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

 # Copyright (c) 2012 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+from file_system import FileNotFoundError
+
 class _CacheEntry(object):
   def __init__(self, cache_data, version):
-    self._cache_data = cache_data
+    self.cache_data = cache_data
     self.version = version
 
 class CompiledFileSystem(object):
   """This class caches FileSystem data that has been processed.
   """
   class Factory(object):
     """A class to build a CompiledFileSystem backed by |file_system|.
     """
-    def __init__(self, file_system, object_store_creator_factory):
+    def __init__(self,
+                 file_system,
+                 object_store_creator_factory,
+                 patched_file_system=None,
+                 patched_object_store_creator_factory=None):

[not at google - send to devlin, 2013/05/07 05:45:40]

this class is conceptually quite simple and now... it isn't. such a generic
class shouldn't have knowledge of server implementation details such as there
being such a thing as a "patched file system".

is there a different way you can achieve what you're going for? which is...
using the old data for the stuff which hasn't been patched, right?

like I said... is it really such a big deal? how much difference does it make?
can you measure it beyond just the first load (which I wouldn't expect to be so
bad since you can use the existing data anyway)? is it just a matter of saving
datastore space, because we really do have infinity storage basically.

[方觉(Fang Jue), 2013/05/07 06:03:55]

It's definitely not a good way to achieve it. What I think would be better is to
add a class called ChainedCompiledFileSystem (or something like that) and handle
these things there. I'm still thinking about how to do that.

For your 'storagetest' patch, without this change the first load takes about 15
seconds. With this change, it usually takes less than 5 seconds. So it's not a
small deal.

[not at google - send to devlin, 2013/05/07 06:14:31]

That sounds interesting, though it seems to me like the problem is at the
FileSystem not the CompiledFileSystem level, i.e. what sounds more promising is
a CacheChainFilleSystem or something (using the same analogy). I wouldn't want
to modify CachingFileSystem at all - it's already complex enough - so maybe just
a ChainedFileSystem would be enough, which tries one after another (it would be
constructed with the patched then the compiled obviously). Make
PatchedFileSystem throw FileNotFound errors for files it doesn't have
modifications for (hm, though you'd need to distinguish between that and the
file being removed).

Anyway, I'll leave it to you. Getting late here, brain not 100%.
Ok. Are subsequent loads the same? I presume so. What about loads after there
has been a patch update? That is an important question. I would expect them to
be quite comparable to each other, very quick.

+      assert ((patched_file_system is None and
+               patched_object_store_creator_factory is None) or
+              (patched_file_system is not None and
+               patched_object_store_creator_factory is not None))
       self._file_system = file_system
       self._object_store_creator_factory = object_store_creator_factory
+      self._patched_file_system = patched_file_system
+      self._patched_object_store_creator_factory = (
+          patched_object_store_creator_factory)
 
     def Create(self, populate_function, cls, category=None):
       """Create a CompiledFileSystem that populates the cache by calling
       |populate_function| with (path, data), where |data| is the data that was
       fetched from |path|.
       The namespace for the file system is derived like ObjectStoreCreator: from
       |cls| along with an optional |category|.
       """
       assert isinstance(cls, type)
       assert not cls.__name__[0].islower()  # guard against non-class types
       full_name = cls.__name__
       if category is not None:
         full_name = '%s/%s' % (full_name, category)
       object_store_creator = self._object_store_creator_factory.Create(
           CompiledFileSystem)
-      return CompiledFileSystem(
+      base_compiled_fs = CompiledFileSystem(
           self._file_system,
           populate_function,
           object_store_creator.Create(category='%s/file' % full_name),
           object_store_creator.Create(category='%s/list' % full_name))
+      if self._patched_file_system:
+        patched_object_store_creator = (self.
+            _patched_object_store_creator_factory.Create(CompiledFileSystem))
+        return CompiledFileSystem(
+            self._patched_file_system,
+            populate_function,
+            patched_object_store_creator.Create(category='%s/file' % full_name),
+            patched_object_store_creator.Create(category='%s/list' % full_name),
+            base_compiled_fs)
+      else:
+        return base_compiled_fs
 
     def CreateIdentity(self, cls):
       '''Handy helper to get or create the identity compiled file system.
       GetFromFile will return the file's contents.
       GetFromFileListing will return the directory list.
       '''
       return self.Create(lambda _, x: x, cls)
 
   def __init__(self,
                file_system,
                populate_function,
                file_object_store,
-               list_object_store):
+               list_object_store,
+               base_compiled_fs=None):
     self._file_system = file_system
     self._populate_function = populate_function
     self._file_object_store = file_object_store
     self._list_object_store = list_object_store
+    self._base_compiled_fs = base_compiled_fs
 
   def _RecursiveList(self, path):
     files = []
     for filename in self._file_system.ReadSingle(path):
       if filename.endswith('/'):
         files.extend(['%s%s' % (filename, f)
                       for f in self._RecursiveList('%s%s' % (path, filename))])
       else:
         files.append(filename)
     return files
 
   def GetFromFile(self, path, binary=False):
     """Calls |populate_function| on the contents of the file at |path|.  If
     |binary| is True then the file will be read as binary - but this will only
     apply for the first time the file is fetched; if already cached, |binary|
     will be ignored.
     """
+    if self._base_compiled_fs:
+      # It's possible that a new file is added in this compiled file system
+      # and it doesn't exist in base_compiled_fs.
+      try:
+        version = self._file_system.Stat(path).version
+        cache_entry = self._base_compiled_fs._GetCacheEntryFromFile(path,
+                                                                    binary)
+        if version == cache_entry.version:
+          return cache_entry.cache_data
+      except FileNotFoundError:
+        pass
+    return self._GetCacheEntryFromFile(path, binary).cache_data
+
+  def _GetCacheEntryFromFile(self, path, binary=False):
     version = self._file_system.Stat(path).version
     cache_entry = self._file_object_store.Get(path).Get()
     if (cache_entry is not None) and (version == cache_entry.version):
-      return cache_entry._cache_data
+      return cache_entry
     cache_data = self._populate_function(
         path,
         self._file_system.ReadSingle(path, binary=binary))
-    self._file_object_store.Set(path, _CacheEntry(cache_data, version))
-    return cache_data
+    cache_entry = _CacheEntry(cache_data, version)
+    self._file_object_store.Set(path, cache_entry)
+    return cache_entry
 
   def GetFromFileListing(self, path):
     """Calls |populate_function| on the listing of the files at |path|.
     Assumes that the path given is to a directory.
     """
     if not path.endswith('/'):
       path += '/'
+    if self._base_compiled_fs:
+      try:
+        version = self._file_system.Stat(path).version
+        cache_entry = self._base_compiled_fs._GetCacheEntryFromFileListing(
+            path)
+        if version == cache_entry.version:
+          return cache_entry.cache_data
+      except FileNotFoundError:
+        pass
+    return self._GetCacheEntryFromFileListing(path).cache_data
+
+  def _GetCacheEntryFromFileListing(self, path):
+    assert path.endswith('/')
     version = self._file_system.Stat(path).version
     cache_entry = self._list_object_store.Get(path).Get()
     if (cache_entry is not None) and (version == cache_entry.version):
-        return cache_entry._cache_data
+        return cache_entry
     cache_data = self._populate_function(path, self._RecursiveList(path))
-    self._list_object_store.Set(path, _CacheEntry(cache_data, version))
-    return cache_data
+    cache_entry = _CacheEntry(cache_data, version)
+    self._list_object_store.Set(path, cache_entry)
+    return cache_entry