Extensions Docs Server: Large performance increase

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

Index: chrome/common/extensions/docs/server2/appengine_memcache.py
diff --git a/chrome/common/extensions/docs/server2/appengine_memcache.py b/chrome/common/extensions/docs/server2/appengine_memcache.py
index ce01e8b52866880069f1f3a8d419f7e21fe25865..605ba71653d37181a5c801f5a3dbca66e7cd48ac 100644
--- a/chrome/common/extensions/docs/server2/appengine_memcache.py
+++ b/chrome/common/extensions/docs/server2/appengine_memcache.py
@@ -2,12 +2,44 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+import time
+
 from appengine_wrappers import memcache
+from future import Future
 
 MEMCACHE_FILE_SYSTEM_READ = 'MemcacheFileSystem.Get'
 MEMCACHE_FILE_SYSTEM_STAT = 'MemcacheFileSystem.Stat'
 MEMCACHE_GITHUB_STAT = 'MemcacheGithub.Stat'
 MEMCACHE_BRANCH_UTILITY = 'BranchUtility'
+MEMCACHE_FILE_SYSTEM_CACHE = 'MemcacheFileSystemCache'
+MEMCACHE_FILE_SYSTEM_CACHE_LISTING = 'MemcacheFileSystemCache.Listing'
+
+class _CacheEntry(object):
+  def __init__(self, value, expire_time):
+    self.value = value
+    self._never_expires = (expire_time == 0)
+    self._expiry = time.time() + expire_time
+
+  def HasExpired(self):
+    if self._never_expires:
+      return False
+    return time.time() > self._expiry
+
+class _AsyncGetFuture(object):
+  def __init__(self, cache, time, namespace, rpc=None, mapping=None):

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

document these parameters?

I had to read the code to know what mapping meant. Can you call it
"initial_mapping" or something (at the parameter level; fine to call it _mapping
on self).

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

Done.

+    self._cache = cache
+    self._time = time
+    self._namespace = namespace
+    self._rpc = rpc
+    self._mapping = mapping
+
+  def Get(self):
+    if self._rpc is not None:
+      result = self._rpc.get_result()
+      self._cache[self._namespace].update(
+          dict((k, _CacheEntry(v, self._time)) for k, v in result.iteritems()))
+      self._mapping.update(result)

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

this only works if initial_mapping is not None, and it could be None if left out
as an optional parameter? Perhaps you should make it default to {} not None.

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

Done.

+    return self._mapping
 
 class AppEngineMemcache(object):

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

This all just got complicated.

You're gonna hate me for this, but we should be implementing this as a decorator
structure too, InMemoryFoo over the top of a MemcacheFoo. Why?
 - testability
 - readability
 - we might want to write a DatastoreFoo and then it will get hard without using
decorators.

This class is also starting to suffer from an identity crisis. It's not really a
Memcache anymore, nor does it have anything to do with AppEngine specifically
(cos it's not memcache anymore) apart from the interface being reminiscent of
the memcache APIs.

But yeah, it's kind of like an object store now.

can we, err, consider calling it ObjectStore, behind an interface, implement an
in-memory and a memcache version of it (sorry).

I need to think about what that means for the identity of the other classes.
Probably not much. We still have
 - filesystem interface has filesystem-specific knowledge and interface
 - filesystemcache interface has knowledge about the compiled types (besides
which, it's basically a thin wrapper over this objectstore thing) and some
recursive list logic (which arguably should be moved into the filesystem layer,
but whatevs).

That would make me happy.

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

Done.

   """A wrapper around the standard App Engine memcache that enforces namespace
@@ -16,15 +48,71 @@ class AppEngineMemcache(object):
   """
   def __init__(self, branch):
     self._branch = branch
+    self._cache = {}
+
+  def _SetInMemory(self, key, value, namespace, time):
+    if namespace not in self._cache:
+      self._cache[namespace] = {}
+    self._cache[namespace][key] = _CacheEntry(value, time)
+
+  def _TryGAE(self, key, namespace, time):
+    result = memcache.get(key, namespace=namespace)
+    if result is not None:
+      self._SetInMemory(key, result, namespace, time)
+    return result
+
+  def _MakeNamespace(self, namespace):
+    return self._branch + '.' + namespace
+
+  def Set(self, key, value, namespace, time=300):

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

define 300 as a constant somewhere?

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

Done.

+    namespace = self._MakeNamespace(namespace)
+    self._SetInMemory(key, value, namespace, time)
+    memcache.set(key, value, namespace=namespace, time=time)

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

this can be an async set, right? We don't care about the value after it gets set
in memcache (we have it locally anyway) so we don't need to wait until it's been
committed.

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

There isn't a set_async function in memcache, only set_multi_async. I don't know
if we want to use that for only one item?

+
+  def SetMulti(self, mapping, namespace, time=300):
+    namespace = self._MakeNamespace(namespace)
+    for k, v in mapping.iteritems():
+      self._SetInMemory(k, v, namespace, time)
+      # Use a batch set? App Engine kept throwing:
+      # ValueError: Values may not be more than 1000000 bytes in length

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

Interesting.

Yeah make this a TODO. If it's an async set maybe it won't matter so much.

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

Done.

+      # for the batch set.
+      self.Set(k, v, namespace, time=time)
 
-  def Set(self, key, value, namespace, time=60):
-    return memcache.set(key,
-                        value,
-                        namespace=self._branch + '.' + namespace,
-                        time=time)
+  def Get(self, key, namespace, time=300):
+    namespace = self._MakeNamespace(namespace)
+    if namespace not in self._cache:
+      return self._TryGAE(key, namespace, time)
+    cache_entry = self._cache[namespace].get(key, None)
+    if not cache_entry:
+      return self._TryGAE(key, namespace, time)
+    if cache_entry.HasExpired():
+      self._cache[namespace].pop(key)
+      return memcache.get(key, namespace=namespace)
+    return cache_entry.value
 
-  def Get(self, key, namespace):
-    return memcache.get(key, namespace=self._branch + '.' + namespace)
+  def GetMulti(self, keys, namespace, time=300):
+    namespace = self._MakeNamespace(namespace)
+    keys = keys[:]
+    mapping = {}
+    if namespace not in self._cache:
+      self._cache[namespace] = {}
+    for key in keys:
+      cache_entry = self._cache[namespace].get(key, None)
+      if cache_entry is not None and not cache_entry.HasExpired():

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

nit: reverse the condition to make this more readable

if cache_entry is None or cache_entry.HasExpired():
  mapping[key] = None
else:
  mapping[key] = cache_entry.value
  keys.remove(key)

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

Done.

+        mapping[key] = cache_entry.value
+        keys.remove(key)
+      else:
+        mapping[key] = None
+    client = memcache.Client()
+    rpc = client.get_multi_async(keys, namespace=namespace)
+    return Future(delegate=_AsyncGetFuture(self._cache,
+                                           time,
+                                           namespace,
+                                           rpc=rpc,
+                                           mapping=mapping))
 
   def Delete(self, key, namespace):
-    return memcache.delete(key, namespace=self._branch + '.' + namespace)
+    namespace = self._MakeNamespace(namespace)
+    if namespace in self._cache:
+      self._cache[namespace].pop(key)
+    memcache.delete(key, namespace=namespace)