Extensions Docs Server: Large performance increase

Extensions Docs Server: Large performance increase

Based off: https://chromiumcodereview.appspot.com/10834329

This adds a few things to speed up performance:
1. Caches items in memcache from FileSystemCache.
2. Caches everything primarily in memory, and falls back on memcache.
3. 5 minute timeout on stats.
4. Cron job to update memcache every 5 minutes (just enough time to update expired stats).
5. Memcache uses set_multi_async and get_multi_async where applicable.

I have an instance at chrome-docs-test.appspot.com, but the cron job is only running
every 10 minutes so I don't go over quota.

After the cron job is run, even samples.html is fast!

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=152472

[cduvall, 2012-08-18 00:48:37]

Fast server yay! I think it's ready for review.

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

exciting!

http://codereview.chromium.org/10829348/diff/1020/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/app.yaml (right):

http://codereview.chromium.org/10829348/diff/1020/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/app.yaml:1: application: chrome-docs-test
:P

http://codereview.chromium.org/10829348/diff/1020/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/appengine_memcache.py (right):

http://codereview.chromium.org/10829348/diff/1020/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/appengine_memcache.py:29: def
__init__(self, cache, time, namespace, rpc=None, mapping=None):
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).

http://codereview.chromium.org/10829348/diff/1020/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/appengine_memcache.py:41:
self._mapping.update(result)
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.

http://codereview.chromium.org/10829348/diff/1020/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/appengine_memcache.py:44: class
AppEngineMemcache(object):
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.

http://codereview.chromium.org/10829348/diff/1020/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/appengine_memcache.py:67: def Set(self,
key, value, namespace, time=300):
define 300 as a constant somewhere?

http://codereview.chromium.org/10829348/diff/1020/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/appengine_memcache.py:70:
memcache.set(key, value, namespace=namespace, time=time)
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.

http://codereview.chromium.org/10829348/diff/1020/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/appengine_memcache.py:77: # ValueError:
Values may not be more than 1000000 bytes in length
Interesting.

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

http://codereview.chromium.org/10829348/diff/1020/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/appengine_memcache.py:101: if cache_entry
is not None and not cache_entry.HasExpired():
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)

http://codereview.chromium.org/10829348/diff/1020/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/appengine_wrappers.py (right):

http://codereview.chromium.org/10829348/diff/1020/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/appengine_wrappers.py:80: def
set_multi_async(self, mapping, namespace='', time=300):
300 looks especially arbitrary here. Make it 0 or -1 or 42 or something.

http://codereview.chromium.org/10829348/diff/1020/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/cron.yaml (right):

http://codereview.chromium.org/10829348/diff/1020/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/cron.yaml:4: schedule: every 5 minutes
This means that the worst case is 10 minutes for a refresh, right?

The best case being that the cronjob finishes and right afterwards the memory
refreshes. The worse case being that the memory refreshes and right afterwards
the cronjob refreshes, meaning it'll be another 5 minutes before the memory
refreshes.

Just something to observe...

http://codereview.chromium.org/10829348/diff/1020/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/cron.yaml:5: 
Another comment:

we need to be careful with races. What if somebody adds an API and its page, the
cronjob finds the page, the memory refreshes and then the cronjob finds the API?
We'll end up with a broken link.

And I wonder if there are more insidious things that could happen.

Just another thing to observe... I think we can get memcache/datastore to do
atomic writes. Something to look into later (but keep it in mind).

http://codereview.chromium.org/10829348/diff/1020/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/file_system_cache.py (right):

http://codereview.chromium.org/10829348/diff/1020/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/file_system_cache.py:20:
FS_CACHE_HANDLEBAR = 'FileSystemCache.Handlebar'
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.

http://codereview.chromium.org/10829348/diff/1020/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/file_system_cache.py:27: class
FileSystemCache(object):
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.

http://codereview.chromium.org/10829348/diff/1020/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/file_system_cache.py:30: class
Builder(object):
ditto (after this patch): rename to Factory

http://codereview.chromium.org/10829348/diff/1020/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/file_system_cache.py:37: def build(self,
populate_function, namespace):
ditto (after this patch): rename to Create

http://codereview.chromium.org/10829348/diff/1020/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/file_system_cache.py:46: self._cache = {}
not used anymore?

http://codereview.chromium.org/10829348/diff/1020/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/file_system_cache.py:72: if  cache_entry
is not None:
nit: extra space

http://codereview.chromium.org/10829348/diff/1020/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/file_system_cache.py:75:
memcache.MEMCACHE_FILE_SYSTEM_CACHE)
I don't think we need to Delete this, we Set it right afterwards?

http://codereview.chromium.org/10829348/diff/1020/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/handler.py (right):

http://codereview.chromium.org/10829348/diff/1020/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/handler.py:149: def _NavigateToPath(self,
path):
this name always trips me up. Can we call it _HandleGet or something, to
distinguish it from the cron case?

http://codereview.chromium.org/10829348/diff/1020/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/handler.py:173: if
path.startswith('/cron'):
nit: can we split up these two distinct cases (cron vs get) into two distinct
functions, like,

path = self.request.path
if path.startswith('/cron'):
  self._HandleCron(path.split('/')[-1])
else:
  # Redirect paths...
  path = ...
  self._HandleGet(self.path)

http://codereview.chromium.org/10829348/diff/1020/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/handler.py:177: self._file_system =
_CreateMemcacheFileSystem(branch, branch_memcache)
file_system doesn't need to be on self, we can define _Cron as a local function,
or even as a lamba.

http://codereview.chromium.org/10829348/diff/1020/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/handler.py:185:
render_cache.GetFromFileListing(PUBLIC_TEMPLATE_PATH)
why does rendering all files from the public template path not pull in
everything that the server needs? I.e. why all the cache.GetFromFileListing
stuff above?

I thought maybe it was an optimisation to prevent needing to render anything (we
really just need to compile things), but yeah, this seems non-trivial. Easier to
render everything even though all that string joining is a bit wasted.

http://codereview.chromium.org/10829348/diff/1020/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/intro_data_source.py (right):

http://codereview.chromium.org/10829348/diff/1020/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/intro_data_source.py:61: self._intro_regex
= re.compile('<h1[^>.]*?>.*?</h1>', flags=re.DOTALL)
nit: doesn't need to be an instance variable?

http://codereview.chromium.org/10829348/diff/1020/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/memcache_file_system.py (right):

http://codereview.chromium.org/10829348/diff/1020/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/memcache_file_system.py:57: version =
child_version
dunno why I wrote it that way. seems better to put it up in an "else version =
dir_stat.child_versions[path]" on line 51.

http://codereview.chromium.org/10829348/diff/1020/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/memcache_file_system.py:73:
memcache.MEMCACHE_FILE_SYSTEM_STAT).Get()
It would be nice if Stat took multiple things, like Read, then we could make
this call Stat efficiently (using multi-get) rather than singly-getting them
when they're not there, later.

The server seems to work fine, though, so add a TODO here and on FileSystem.Stat
itself?

http://codereview.chromium.org/10829348/diff/1020/tools/json_schema_compiler/...
File tools/json_schema_compiler/model.py (right):

http://codereview.chromium.org/10829348/diff/1020/tools/json_schema_compiler/...
tools/json_schema_compiler/model.py:308: return self.name
Add a note that this is so it can be pickled.

Also, perhaps call something that links it with PropertyType, like,
_PropertyTypeInfo :p

[not at google - send to devlin, 2012-08-20 05:36:52]

btw, do as much of the ObjectStore rename as you want in a follow-up (i.e.
renaming variables and stuff), it's just the interface/decorator stuff that's
important for this patch.

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

ObjectStore :)

https://chromiumcodereview.appspot.com/10829348/diff/1020/chrome/common/exten...
File chrome/common/extensions/docs/server2/appengine_memcache.py (right):

https://chromiumcodereview.appspot.com/10829348/diff/1020/chrome/common/exten...
chrome/common/extensions/docs/server2/appengine_memcache.py:29: def
__init__(self, cache, time, namespace, rpc=None, mapping=None):
On 2012/08/20 05:27:10, kalman wrote:
> 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).

Done.

https://chromiumcodereview.appspot.com/10829348/diff/1020/chrome/common/exten...
chrome/common/extensions/docs/server2/appengine_memcache.py:41:
self._mapping.update(result)
On 2012/08/20 05:27:10, kalman wrote:
> 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.

Done.

https://chromiumcodereview.appspot.com/10829348/diff/1020/chrome/common/exten...
chrome/common/extensions/docs/server2/appengine_memcache.py:44: class
AppEngineMemcache(object):
On 2012/08/20 05:27:10, kalman wrote:
> 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.

Done.

https://chromiumcodereview.appspot.com/10829348/diff/1020/chrome/common/exten...
chrome/common/extensions/docs/server2/appengine_memcache.py:67: def Set(self,
key, value, namespace, time=300):
On 2012/08/20 05:27:10, kalman wrote:
> define 300 as a constant somewhere?

Done.

https://chromiumcodereview.appspot.com/10829348/diff/1020/chrome/common/exten...
chrome/common/extensions/docs/server2/appengine_memcache.py:70:
memcache.set(key, value, namespace=namespace, time=time)
On 2012/08/20 05:27:10, kalman wrote:
> 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.

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?

https://chromiumcodereview.appspot.com/10829348/diff/1020/chrome/common/exten...
chrome/common/extensions/docs/server2/appengine_memcache.py:77: # ValueError:
Values may not be more than 1000000 bytes in length
On 2012/08/20 05:27:10, kalman wrote:
> Interesting.
> 
> Yeah make this a TODO. If it's an async set maybe it won't matter so much.

Done.

https://chromiumcodereview.appspot.com/10829348/diff/1020/chrome/common/exten...
chrome/common/extensions/docs/server2/appengine_memcache.py:101: if cache_entry
is not None and not cache_entry.HasExpired():
On 2012/08/20 05:27:10, kalman wrote:
> 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)

Done.

https://chromiumcodereview.appspot.com/10829348/diff/1020/chrome/common/exten...
File chrome/common/extensions/docs/server2/appengine_wrappers.py (right):

https://chromiumcodereview.appspot.com/10829348/diff/1020/chrome/common/exten...
chrome/common/extensions/docs/server2/appengine_wrappers.py:80: def
set_multi_async(self, mapping, namespace='', time=300):
On 2012/08/20 05:27:10, kalman wrote:
> 300 looks especially arbitrary here. Make it 0 or -1 or 42 or something.

Done.

https://chromiumcodereview.appspot.com/10829348/diff/1020/chrome/common/exten...
File chrome/common/extensions/docs/server2/cron.yaml (right):

https://chromiumcodereview.appspot.com/10829348/diff/1020/chrome/common/exten...
chrome/common/extensions/docs/server2/cron.yaml:5: 
On 2012/08/20 05:27:10, kalman wrote:
> Another comment:
> 
> we need to be careful with races. What if somebody adds an API and its page,
the
> cronjob finds the page, the memory refreshes and then the cronjob finds the
API?
> We'll end up with a broken link.
> 
> And I wonder if there are more insidious things that could happen.
> 
> Just another thing to observe... I think we can get memcache/datastore to do
> atomic writes. Something to look into later (but keep it in mind).

Both good things to think about. Thanks.

https://chromiumcodereview.appspot.com/10829348/diff/1020/chrome/common/exten...
File chrome/common/extensions/docs/server2/file_system_cache.py (right):

https://chromiumcodereview.appspot.com/10829348/diff/1020/chrome/common/exten...
chrome/common/extensions/docs/server2/file_system_cache.py:20:
FS_CACHE_HANDLEBAR = 'FileSystemCache.Handlebar'
On 2012/08/20 05:27:10, kalman wrote:
> 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.

Done.

https://chromiumcodereview.appspot.com/10829348/diff/1020/chrome/common/exten...
chrome/common/extensions/docs/server2/file_system_cache.py:46: self._cache = {}
On 2012/08/20 05:27:10, kalman wrote:
> not used anymore?

Done.

https://chromiumcodereview.appspot.com/10829348/diff/1020/chrome/common/exten...
chrome/common/extensions/docs/server2/file_system_cache.py:72: if  cache_entry
is not None:
On 2012/08/20 05:27:10, kalman wrote:
> nit: extra space

Done.

https://chromiumcodereview.appspot.com/10829348/diff/1020/chrome/common/exten...
chrome/common/extensions/docs/server2/file_system_cache.py:75:
memcache.MEMCACHE_FILE_SYSTEM_CACHE)
On 2012/08/20 05:27:10, kalman wrote:
> I don't think we need to Delete this, we Set it right afterwards?

Done.

https://chromiumcodereview.appspot.com/10829348/diff/1020/chrome/common/exten...
File chrome/common/extensions/docs/server2/handler.py (right):

https://chromiumcodereview.appspot.com/10829348/diff/1020/chrome/common/exten...
chrome/common/extensions/docs/server2/handler.py:149: def _NavigateToPath(self,
path):
On 2012/08/20 05:27:10, kalman wrote:
> this name always trips me up. Can we call it _HandleGet or something, to
> distinguish it from the cron case?

Done.

https://chromiumcodereview.appspot.com/10829348/diff/1020/chrome/common/exten...
chrome/common/extensions/docs/server2/handler.py:173: if
path.startswith('/cron'):
On 2012/08/20 05:27:10, kalman wrote:
> nit: can we split up these two distinct cases (cron vs get) into two distinct
> functions, like,
> 
> path = self.request.path
> if path.startswith('/cron'):
>   self._HandleCron(path.split('/')[-1])
> else:
>   # Redirect paths...
>   path = ...
>   self._HandleGet(self.path)

Done.

https://chromiumcodereview.appspot.com/10829348/diff/1020/chrome/common/exten...
chrome/common/extensions/docs/server2/handler.py:177: self._file_system =
_CreateMemcacheFileSystem(branch, branch_memcache)
On 2012/08/20 05:27:10, kalman wrote:
> file_system doesn't need to be on self, we can define _Cron as a local
function,
> or even as a lamba.

Done.

https://chromiumcodereview.appspot.com/10829348/diff/1020/chrome/common/exten...
chrome/common/extensions/docs/server2/handler.py:185:
render_cache.GetFromFileListing(PUBLIC_TEMPLATE_PATH)
On 2012/08/20 05:27:10, kalman wrote:
> why does rendering all files from the public template path not pull in
> everything that the server needs? I.e. why all the cache.GetFromFileListing
> stuff above?
> 
> I thought maybe it was an optimisation to prevent needing to render anything
(we
> really just need to compile things), but yeah, this seems non-trivial. Easier
to
> render everything even though all that string joining is a bit wasted.

I just thought it would be a little faster getting all the files ahead of time
in one large parallel fetch, so all the rendering could just use the files from
memcache.

https://chromiumcodereview.appspot.com/10829348/diff/1020/chrome/common/exten...
File chrome/common/extensions/docs/server2/intro_data_source.py (right):

https://chromiumcodereview.appspot.com/10829348/diff/1020/chrome/common/exten...
chrome/common/extensions/docs/server2/intro_data_source.py:61: self._intro_regex
= re.compile('<h1[^>.]*?>.*?</h1>', flags=re.DOTALL)
On 2012/08/20 05:27:10, kalman wrote:
> nit: doesn't need to be an instance variable?

Done.

https://chromiumcodereview.appspot.com/10829348/diff/1020/chrome/common/exten...
File chrome/common/extensions/docs/server2/memcache_file_system.py (right):

https://chromiumcodereview.appspot.com/10829348/diff/1020/chrome/common/exten...
chrome/common/extensions/docs/server2/memcache_file_system.py:57: version =
child_version
On 2012/08/20 05:27:10, kalman wrote:
> dunno why I wrote it that way. seems better to put it up in an "else version =
> dir_stat.child_versions[path]" on line 51.

Done.

https://chromiumcodereview.appspot.com/10829348/diff/1020/chrome/common/exten...
chrome/common/extensions/docs/server2/memcache_file_system.py:73:
memcache.MEMCACHE_FILE_SYSTEM_STAT).Get()
On 2012/08/20 05:27:10, kalman wrote:
> It would be nice if Stat took multiple things, like Read, then we could make
> this call Stat efficiently (using multi-get) rather than singly-getting them
> when they're not there, later.
> 
> The server seems to work fine, though, so add a TODO here and on
FileSystem.Stat
> itself?

Done.

https://chromiumcodereview.appspot.com/10829348/diff/1020/tools/json_schema_c...
File tools/json_schema_compiler/model.py (right):

https://chromiumcodereview.appspot.com/10829348/diff/1020/tools/json_schema_c...
tools/json_schema_compiler/model.py:308: return self.name
On 2012/08/20 05:27:10, kalman wrote:
> Add a note that this is so it can be pickled.
> 
> Also, perhaps call something that links it with PropertyType, like,
> _PropertyTypeInfo :p

Done.

[not at google - send to devlin, 2012-08-21 00:30:11]

The cron job needs a test too, by the way. We should find out before it goes
live if it's not working.

https://chromiumcodereview.appspot.com/10829348/diff/1020/chrome/common/exten...
File chrome/common/extensions/docs/server2/handler.py (right):

https://chromiumcodereview.appspot.com/10829348/diff/1020/chrome/common/exten...
chrome/common/extensions/docs/server2/handler.py:185:
render_cache.GetFromFileListing(PUBLIC_TEMPLATE_PATH)
On 2012/08/20 21:28:09, cduvall wrote:
> On 2012/08/20 05:27:10, kalman wrote:
> > why does rendering all files from the public template path not pull in
> > everything that the server needs? I.e. why all the cache.GetFromFileListing
> > stuff above?
> > 
> > I thought maybe it was an optimisation to prevent needing to render anything
> (we
> > really just need to compile things), but yeah, this seems non-trivial.
Easier
> to
> > render everything even though all that string joining is a bit wasted.
> 
> I just thought it would be a little faster getting all the files ahead of time
> in one large parallel fetch, so all the rendering could just use the files
from
> memcache.

Interesting. Did you find that to be the case? If it's faster to get this stuff
ahead of time then we should do it :\ sorry. Though comment that's why you're
doing it. And make sure it is actually faster.

https://chromiumcodereview.appspot.com/10829348/diff/3047/chrome/common/exten...
File chrome/common/extensions/docs/server2/file_system_cache.py (right):

https://chromiumcodereview.appspot.com/10829348/diff/3047/chrome/common/exten...
chrome/common/extensions/docs/server2/file_system_cache.py:72: if version ==
cache_entry.version:
if (cache_entry is not None) and (version == cache_entry.version)?

https://chromiumcodereview.appspot.com/10829348/diff/3047/chrome/common/exten...
chrome/common/extensions/docs/server2/file_system_cache.py:94:
self._object_store.Delete(self._MakeKey(path),
same deal here, don't need to Delete since it's set afterwards?

https://chromiumcodereview.appspot.com/10829348/diff/3047/chrome/common/exten...
File chrome/common/extensions/docs/server2/github_file_system_test.py (right):

https://chromiumcodereview.appspot.com/10829348/diff/3047/chrome/common/exten...
chrome/common/extensions/docs/server2/github_file_system_test.py:9: from
in_memory_object_store import InMemoryObjectStore
import order

https://chromiumcodereview.appspot.com/10829348/diff/3047/chrome/common/exten...
File chrome/common/extensions/docs/server2/in_memory_object_store.py (right):

https://chromiumcodereview.appspot.com/10829348/diff/3047/chrome/common/exten...
chrome/common/extensions/docs/server2/in_memory_object_store.py:65: return
'ObjectStore.' + self._branch + '.' + namespace
+'ing strings in python is inefficient, use '.'.join(['ObjectStore',
self._branch, namespace]) or %s

https://chromiumcodereview.appspot.com/10829348/diff/3047/chrome/common/exten...
chrome/common/extensions/docs/server2/in_memory_object_store.py:112: 
A lot of this code would be simplified by having Get call GetMulti and Set call
SetMulti.

https://chromiumcodereview.appspot.com/10829348/diff/3047/chrome/common/exten...
File chrome/common/extensions/docs/server2/intro_data_source.py (right):

https://chromiumcodereview.appspot.com/10829348/diff/3047/chrome/common/exten...
chrome/common/extensions/docs/server2/intro_data_source.py:13: _intro_regex =
re.compile('<h1[^>.]*?>.*?</h1>', flags=re.DOTALL)
should be _INTRO_REGEX

or _H1_REGEX

https://chromiumcodereview.appspot.com/10829348/diff/3047/chrome/common/exten...
File chrome/common/extensions/docs/server2/memcache_file_system.py (right):

https://chromiumcodereview.appspot.com/10829348/diff/3047/chrome/common/exten...
chrome/common/extensions/docs/server2/memcache_file_system.py:55: # is thrown so
they never get cached.
oh, that seems bad. Is that because it's looking for a json but it's an idl or
something?

https://chromiumcodereview.appspot.com/10829348/diff/3047/chrome/common/exten...
File chrome/common/extensions/docs/server2/memcache_object_store.py (right):

https://chromiumcodereview.appspot.com/10829348/diff/3047/chrome/common/exten...
chrome/common/extensions/docs/server2/memcache_object_store.py:17:
memcache.set(key, value, namespace=namespace, time=time)
yeah, I wonder if we should call set_multi_async here anyway (or rather,
SetMulti({key: value}, namespace, time=time))

https://chromiumcodereview.appspot.com/10829348/diff/3047/chrome/common/exten...
chrome/common/extensions/docs/server2/memcache_object_store.py:21:
client.set_multi_async(mapping, namespace=namespace, time=time)
nit: inline client? and below?

https://chromiumcodereview.appspot.com/10829348/diff/3047/chrome/common/exten...
chrome/common/extensions/docs/server2/memcache_object_store.py:24: return
memcache.get(key, namespace=namespace)
I wonder if this should also return a future (i.e. call GetMulti with [key])

In that case you could have ObjectStore implement Get/Set as delegates to
GetMulti/SetMulti and not need to implement it in here nor in
InMemoryObjectStore. Just a thought.

https://chromiumcodereview.appspot.com/10829348/diff/3047/chrome/common/exten...
File chrome/common/extensions/docs/server2/object_store.py (right):

https://chromiumcodereview.appspot.com/10829348/diff/3047/chrome/common/exten...
chrome/common/extensions/docs/server2/object_store.py:22: def Get(self, key,
namespace, time=CACHE_TIMEOUT):
I think these all need a bit of documentation, particularly since by looking at
the implementations Get() returns the value whereas GetMulti returns a Future.
Is that a correct observation?

I actually think that Get/GetMulti should both return Futures to be consistent,
though...

[cduvall, 2012-08-21 01:33:33]

https://chromiumcodereview.appspot.com/10829348/diff/1020/chrome/common/exten...
File chrome/common/extensions/docs/server2/handler.py (right):

https://chromiumcodereview.appspot.com/10829348/diff/1020/chrome/common/exten...
chrome/common/extensions/docs/server2/handler.py:185:
render_cache.GetFromFileListing(PUBLIC_TEMPLATE_PATH)
On 2012/08/21 00:30:11, kalman wrote:
> On 2012/08/20 21:28:09, cduvall wrote:
> > On 2012/08/20 05:27:10, kalman wrote:
> > > why does rendering all files from the public template path not pull in
> > > everything that the server needs? I.e. why all the
cache.GetFromFileListing
> > > stuff above?
> > > 
> > > I thought maybe it was an optimisation to prevent needing to render
anything
> > (we
> > > really just need to compile things), but yeah, this seems non-trivial.
> Easier
> > to
> > > render everything even though all that string joining is a bit wasted.
> > 
> > I just thought it would be a little faster getting all the files ahead of
time
> > in one large parallel fetch, so all the rendering could just use the files
> from
> > memcache.
> 
> Interesting. Did you find that to be the case? If it's faster to get this
stuff
> ahead of time then we should do it :\ sorry. Though comment that's why you're
> doing it. And make sure it is actually faster.

I don't think it makes too much of a difference, so I'm going with simpler is
better.

https://chromiumcodereview.appspot.com/10829348/diff/3047/chrome/common/exten...
File chrome/common/extensions/docs/server2/file_system_cache.py (right):

https://chromiumcodereview.appspot.com/10829348/diff/3047/chrome/common/exten...
chrome/common/extensions/docs/server2/file_system_cache.py:72: if version ==
cache_entry.version:
On 2012/08/21 00:30:11, kalman wrote:
> if (cache_entry is not None) and (version == cache_entry.version)?

Done.

https://chromiumcodereview.appspot.com/10829348/diff/3047/chrome/common/exten...
chrome/common/extensions/docs/server2/file_system_cache.py:94:
self._object_store.Delete(self._MakeKey(path),
On 2012/08/21 00:30:11, kalman wrote:
> same deal here, don't need to Delete since it's set afterwards?

Done.

https://chromiumcodereview.appspot.com/10829348/diff/3047/chrome/common/exten...
File chrome/common/extensions/docs/server2/github_file_system_test.py (right):

https://chromiumcodereview.appspot.com/10829348/diff/3047/chrome/common/exten...
chrome/common/extensions/docs/server2/github_file_system_test.py:9: from
in_memory_object_store import InMemoryObjectStore
On 2012/08/21 00:30:11, kalman wrote:
> import order

Done.

https://chromiumcodereview.appspot.com/10829348/diff/3047/chrome/common/exten...
File chrome/common/extensions/docs/server2/in_memory_object_store.py (right):

https://chromiumcodereview.appspot.com/10829348/diff/3047/chrome/common/exten...
chrome/common/extensions/docs/server2/in_memory_object_store.py:65: return
'ObjectStore.' + self._branch + '.' + namespace
On 2012/08/21 00:30:11, kalman wrote:
> +'ing strings in python is inefficient, use '.'.join(['ObjectStore',
> self._branch, namespace]) or %s

Done.

https://chromiumcodereview.appspot.com/10829348/diff/3047/chrome/common/exten...
chrome/common/extensions/docs/server2/in_memory_object_store.py:112: 
On 2012/08/21 00:30:11, kalman wrote:
> A lot of this code would be simplified by having Get call GetMulti and Set
call
> SetMulti.

Done.

https://chromiumcodereview.appspot.com/10829348/diff/3047/chrome/common/exten...
File chrome/common/extensions/docs/server2/intro_data_source.py (right):

https://chromiumcodereview.appspot.com/10829348/diff/3047/chrome/common/exten...
chrome/common/extensions/docs/server2/intro_data_source.py:13: _intro_regex =
re.compile('<h1[^>.]*?>.*?</h1>', flags=re.DOTALL)
On 2012/08/21 00:30:11, kalman wrote:
> should be _INTRO_REGEX
> 
> or _H1_REGEX

Done.

https://chromiumcodereview.appspot.com/10829348/diff/3047/chrome/common/exten...
File chrome/common/extensions/docs/server2/memcache_file_system.py (right):

https://chromiumcodereview.appspot.com/10829348/diff/3047/chrome/common/exten...
chrome/common/extensions/docs/server2/memcache_file_system.py:55: # is thrown so
they never get cached.
On 2012/08/21 00:30:11, kalman wrote:
> oh, that seems bad. Is that because it's looking for a json but it's an idl or
> something?

Yep, its only one fetch so its not a huge deal, but it should definitely be
fixed.

https://chromiumcodereview.appspot.com/10829348/diff/3047/chrome/common/exten...
File chrome/common/extensions/docs/server2/memcache_object_store.py (right):

https://chromiumcodereview.appspot.com/10829348/diff/3047/chrome/common/exten...
chrome/common/extensions/docs/server2/memcache_object_store.py:17:
memcache.set(key, value, namespace=namespace, time=time)
On 2012/08/21 00:30:11, kalman wrote:
> yeah, I wonder if we should call set_multi_async here anyway (or rather,
> SetMulti({key: value}, namespace, time=time))

Done.

https://chromiumcodereview.appspot.com/10829348/diff/3047/chrome/common/exten...
chrome/common/extensions/docs/server2/memcache_object_store.py:21:
client.set_multi_async(mapping, namespace=namespace, time=time)
On 2012/08/21 00:30:11, kalman wrote:
> nit: inline client? and below?

Done.

https://chromiumcodereview.appspot.com/10829348/diff/3047/chrome/common/exten...
chrome/common/extensions/docs/server2/memcache_object_store.py:24: return
memcache.get(key, namespace=namespace)
On 2012/08/21 00:30:11, kalman wrote:
> I wonder if this should also return a future (i.e. call GetMulti with [key])
> 
> In that case you could have ObjectStore implement Get/Set as delegates to
> GetMulti/SetMulti and not need to implement it in here nor in
> InMemoryObjectStore. Just a thought.

Done.

https://chromiumcodereview.appspot.com/10829348/diff/3047/chrome/common/exten...
File chrome/common/extensions/docs/server2/object_store.py (right):

https://chromiumcodereview.appspot.com/10829348/diff/3047/chrome/common/exten...
chrome/common/extensions/docs/server2/object_store.py:22: def Get(self, key,
namespace, time=CACHE_TIMEOUT):
On 2012/08/21 00:30:11, kalman wrote:
> I think these all need a bit of documentation, particularly since by looking
at
> the implementations Get() returns the value whereas GetMulti returns a Future.
> Is that a correct observation?
> 
> I actually think that Get/GetMulti should both return Futures to be
consistent,
> though...

Done.

[not at google - send to devlin, 2012-08-21 01:43:22]

lgtm

http://codereview.chromium.org/10829348/diff/5050/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/file_system_cache.py (right):

http://codereview.chromium.org/10829348/diff/5050/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/file_system_cache.py:92: if version ==
cache_entry.version:
(cache_entry is not None) and (version == cache_entry.version)

http://codereview.chromium.org/10829348/diff/5050/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/in_memory_object_store.py (right):

http://codereview.chromium.org/10829348/diff/5050/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/in_memory_object_store.py:53: def
_SetInMemory(self, key, value, namespace, time):
it's only used in 1 place (SetMulti) so i'd just inline it there.

http://codereview.chromium.org/10829348/diff/5050/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/in_memory_object_store.py:58: def
_TryBackingObjectStore(self, key, namespace, time):
not called anymore, so you can delete it?

[cduvall, 2012-08-21 01:50:10]

https://chromiumcodereview.appspot.com/10829348/diff/5050/chrome/common/exten...
File chrome/common/extensions/docs/server2/file_system_cache.py (right):

https://chromiumcodereview.appspot.com/10829348/diff/5050/chrome/common/exten...
chrome/common/extensions/docs/server2/file_system_cache.py:92: if version ==
cache_entry.version:
On 2012/08/21 01:43:22, kalman wrote:
> (cache_entry is not None) and (version == cache_entry.version)

Done.

https://chromiumcodereview.appspot.com/10829348/diff/5050/chrome/common/exten...
File chrome/common/extensions/docs/server2/in_memory_object_store.py (right):

https://chromiumcodereview.appspot.com/10829348/diff/5050/chrome/common/exten...
chrome/common/extensions/docs/server2/in_memory_object_store.py:53: def
_SetInMemory(self, key, value, namespace, time):
On 2012/08/21 01:43:22, kalman wrote:
> it's only used in 1 place (SetMulti) so i'd just inline it there.

Done.

https://chromiumcodereview.appspot.com/10829348/diff/5050/chrome/common/exten...
chrome/common/extensions/docs/server2/in_memory_object_store.py:58: def
_TryBackingObjectStore(self, key, namespace, time):
On 2012/08/21 01:43:22, kalman wrote:
> not called anymore, so you can delete it?

Done.