Extensions Docs Server: Internal file system

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

Index: chrome/common/extensions/docs/server2/memcache_file_system.py
diff --git a/chrome/common/extensions/docs/server2/memcache_file_system.py b/chrome/common/extensions/docs/server2/memcache_file_system.py
new file mode 100644
index 0000000000000000000000000000000000000000..a6e17cefc18bf73f184b564700a4554aee91077a
--- /dev/null
+++ b/chrome/common/extensions/docs/server2/memcache_file_system.py
@@ -0,0 +1,40 @@
+# 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 lazy_value import LazyValue
+
+class MemcacheFileSystem(object):

[not at google - send to devlin, 2012/07/18 10:39:15]

documentation etc. Why does this not extend FileSystem while LocalFileSystem
does? For documentation's sake this should extend FileSystem, I think.

[cduvall, 2012/07/18 21:26:10]

Done.

+  def __init__(self, file_system, memcache):
+    self._file_system = file_system
+    self._memcache = memcache

[not at google - send to devlin, 2012/07/18 10:39:15]

Class needs tests! And for that you'll probably need to pull out memcache into
its own little abstraction thing (AppengineMemcache / InMemoryMemcache), though
it'd be much more trivial than the urlfetch thing. Very trivial.

[cduvall, 2012/07/18 21:26:10]

Done.

+
+  def Stat(self, path):
+    directory = path.rsplit('/', 1)[0]

[not at google - send to devlin, 2012/07/18 10:39:15]

Stat should be at the path level, not the directory level. If somebody wants to
stat a directory they should convert the path into a directory first.

[cduvall, 2012/07/18 21:26:10]

We can only stat directories, not files. See 3 comments down.

[not at google - send to devlin, 2012/07/19 03:55:18]

Ah I see. Bummer, this would be so neat (see comment below) if it did.

We *could* use viewvc for the stat, which gives the version of every file as
well as the directory itself. Then
- if stat'ing a file, returns just the file's version
- if stat'ing a dir, returns the dir's version plus a "files" dict which maps
the file in each directory to its version (part of StatInfo obviously)

So SubversionFileSystem would need to do a bunch of extra HTML parsing for that
(see
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/extensions/docs/
for example) and its implementation of Stat for files would be fairly
inefficient: stat the dir, then pull out just the file.

but...

Then MemcacheFileSystem can be smart, something like:

def Stat(self, path):
  cached = memcache.get(path)
  if cached:
    return cached

  is_dir = path.endswith('/')
  if is_dir:
    dir_path = path
  else
    dir_path, file_path = path.rsplit('/', 1)
    dir_path += '/'
  dir_path = path if is_dir else path[:path.rindex('/') + 1]
  dir_stat = file_system.Stat(dir_path)

  memcache.set(dir_path, dir_stat.version)
  for file_name, file_stat in dir_stat.files.iteritems():
    memcache.set(dir_path + '/' + file_name, file_stat)

  return dir_stat if is_dir else dir_stat.files[file_path]

So, I think that would ultimately turn out to be quite a bit more efficient,
since you update all file stats at once and therefore don't need to read back
every file when one gets invalidated.

However -- unless you're really keen to do that now, it's fine to do it in a
follow-up. There is a lot of value in this patch already, and the change would
be incremental. Plus if it turns out to actually *not* be any better, well, easy
to revert :)

+    stat_info = self._memcache.get(directory, namespace='directories')

[not at google - send to devlin, 2012/07/18 10:39:15]

we should be more disciplined with these namespaces. either name them with a
stronger convention (e.g. here, 'MemcacheFileSystem.Stat', below,
'MemcacheFileSystem.Read'), or list them somewhere.

Actually what might be nice (and would have prevented those bugs below) is to
make your memcache abstraction have a compulsory namespace, and have those
listed. E.g.

class Memcache(object):
  MEMCACHE_FILE_SYSTEM_READ = "MemcacheFileSystem.Get"
  MEMCACHE_FILE_SYSTEM_STAT = "MemcacheFileSystem.Stat"

  def Get(self, key):
    raise NotImplementedError()

class AppengineMemcache(object):
  def Get(self, key, time=60, namespace=None):
    if namespace == None:
      raise ValueError(...)
    return memcache.get(key, time=time, namespace=namespace)

[cduvall, 2012/07/18 21:26:10]

Done.

+    if stat_info is None:
+      stat_info = self._file_system.Stat(directory).version
+      self._memcache.set(directory, stat_info, time=60, namespace='directories')
+    return stat_info
+
+  def Read(self, paths):
+    result = {}
+    uncached = []
+    for path in paths:
+      cached_result = self._memcache.get(path)
+      if cached_result is None:

[not at google - send to devlin, 2012/07/18 10:39:16]

what's wrong with ==

[cduvall, 2012/07/18 21:26:10]

See previous comment.

+        uncached.append(path)
+        continue
+      data, version = cached_result
+      if self.Stat(path) > version:

[not at google - send to devlin, 2012/07/18 10:39:16]

So this means that if any file in a directory changes, all files in that
directory get invalidated. Hm. If Stat took a path, this logic *could* be

if (self.Stat(_GetDir(path)) > version and
    self.Stat(path) > version):
  ...

which wouldn't be a whole lot better since now there's twice the number of stats
(although they get cached, so, hm, maybe not).

My point is that this could be *even better* as:

data, version = cached_result
if self.Stat(_GetDir(path)) > version:
  # Some entities(s) in the directory has changed. It might be this one?
  entity_version = self.Stat(path).version
  if entity_version > version:
    # Entity has changed. Invalidate it and read the whole contents back later.
    memcache.Delete(path, namespace=...)
    uncached.append(path)
    continue
  # Entity *hasn't* changed, and that means we can bump its version
  # in memcache so that it doesn't need to be re-stat'ed next time.
  memcache.Set(path, (data, entity_version), namespace=...)
result[path] = Future(value=data)

p.s. try to document these nontrivial algorithms.

p.s. also my brain is getting a bit sore but this all seems a bit prone to race
conditions, if memcache can change underneath us. Something to look into.

[cduvall, 2012/07/18 21:26:10]

The only problem with this: We can't stat files, just directories. The version
is scraped from the HTML of the directory page, and fetching files just returns
contents.

[not at google - send to devlin, 2012/07/19 03:55:18]

See comment above.

+        self._memcache.delete(path)

[not at google - send to devlin, 2012/07/18 10:39:16]

also missing namespace

[cduvall, 2012/07/18 21:26:10]

Done.

+        uncached.append(path)
+        continue
+      result[path] = LazyValue(value=data)
+    new_items = self._file_system.Read(uncached).Get()
+    for item in new_items:
+      version = self.Stat(item)
+      self._memcache.set(item, (new_items[item], version), namespace='data')

[not at google - send to devlin, 2012/07/18 10:39:16]

set but never get?

Oh, the get is missing its namespace. Foundabug!

[cduvall, 2012/07/18 21:26:10]

Done.

+      new_items[item] = LazyValue(value=new_items[item])

[not at google - send to devlin, 2012/07/18 10:39:16]

why not
result[item] = LazyValue(value=new_items[item])
?
then you don't need the update, and don't need to be writing over the data
that's being iterated over (which looks strange)

[cduvall, 2012/07/18 21:26:10]

Why not? Because I'm a dummy.

+    result.update(new_items)
+    return result