Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
521453003:
Docserver: Override Walk in CachingFileSystem (Closed)
|
Created:
6 years, 3 months ago by ahernandez Modified:
6 years, 3 months ago Reviewers:
not at google - send to devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionDocserver: Override Walk in CachingFileSystem
NOTRY=True
Committed: https://crrev.com/e95c9e298410a846a8bf941833b6aa8357c5560a
Cr-Commit-Position: refs/heads/master@{#292708}
Patch Set 1 : #
Total comments: 13
Patch Set 2 : #
Total comments: 3
Patch Set 3 : #
Total comments: 4
Patch Set 4 : #
Messages
Total messages: 18 (1 generated)
Patchset #1 (id:1) has been deleted
ahernandez.miralles@gmail.com changed reviewers: + kalman@chromium.org
Here are the requested caching changes, PTAL. https://codereview.chromium.org/521453003/diff/20001/chrome/common/extensions... File chrome/common/extensions/docs/server2/compiled_file_system.py (right): https://codereview.chromium.org/521453003/diff/20001/chrome/common/extensions... chrome/common/extensions/docs/server2/compiled_file_system.py:22: '''Functions bound to an object are separate from the unbound I don't know how to explain this concisely, but the SingleFile annotation wasn't actually doing it's job in a lot of cases because when a method is annotated, the unbound version of it is passed to the annotation function, e.g. SingleFile. But then the bound version is passed to compiled fs, e.g. c_fs_factory.Create(self._comp_func, ...), so when checking if the compilation function is in the set of single file functions, the test fails because the bound method != the unbound method. So I wrote this to ensure that the unbound version is always grabbed.
https://codereview.chromium.org/521453003/diff/20001/chrome/common/extensions... File chrome/common/extensions/docs/server2/caching_file_system.py (right): https://codereview.chromium.org/521453003/diff/20001/chrome/common/extensions... chrome/common/extensions/docs/server2/caching_file_system.py:155: cache_stat = self._stat_object_store.Get(root).Get() These don't need to be Then-ified, right?
Could you split this up into 2 CLs? I'd like to submit them separately and see the affect each has on speed and memory usage etc. The CompiledFileSystem changes basically lg but I want to play around a bit to find a good equilibrium. The Walk changes I need to think about a bit, when it's not 10pm. https://codereview.chromium.org/521453003/diff/20001/chrome/common/extensions... File chrome/common/extensions/docs/server2/caching_file_system.py (right): https://codereview.chromium.org/521453003/diff/20001/chrome/common/extensions... chrome/common/extensions/docs/server2/caching_file_system.py:32: self._walk_cache = create_object_store('walk') Should this be start_empty=False for the same reason as the 'read' cache is? https://codereview.chromium.org/521453003/diff/20001/chrome/common/extensions... chrome/common/extensions/docs/server2/caching_file_system.py:144: def Walk(self, root, depth=-1): I'm looking at this quite late at night, so, apologies if this is silly - but could you write this closer to a delegate style? Like could this be basically a wrapper around self._file_system.Walk() which cancels the walk (and runs any cached data) if it runs into anything that isn't new? Can't think of quite what that would look like, but it's a shame needing to implement the walk logic again. Or maybe you could rephrase FileSystem.Walk so that CachingFileSystem.Walk can hook into it somehow? I think it's too late at night for me to review this part of the CL :) https://codereview.chromium.org/521453003/diff/20001/chrome/common/extensions... File chrome/common/extensions/docs/server2/compiled_file_system.py (right): https://codereview.chromium.org/521453003/diff/20001/chrome/common/extensions... chrome/common/extensions/docs/server2/compiled_file_system.py:22: '''Functions bound to an object are separate from the unbound On 2014/08/29 01:49:42, ahernandez wrote: > I don't know how to explain this concisely, but the SingleFile annotation wasn't > actually doing it's job in a lot of cases because when a method is annotated, > the unbound version of it is passed to the annotation function, e.g. SingleFile. > But then the bound version is passed to compiled fs, e.g. > c_fs_factory.Create(self._comp_func, ...), so when checking if the compilation > function is in the set of single file functions, the test fails because the > bound method != the unbound method. So I wrote this to ensure that the unbound > version is always grabbed. Neat, nice catch. This nested decorator stuff in Python always messed me up. https://codereview.chromium.org/521453003/diff/20001/chrome/common/extensions... chrome/common/extensions/docs/server2/compiled_file_system.py:124: Cache(SingleFile(lambda _, data: JSON should be ... fairly cheap? Very commonly used, though, hm. Could you do some benchmarking perhaps? Starting from nothing Cached and start adding back in things? cron_servlet_test.py is actually pretty good - not only does the time running reflect well on the pure computational time it tries to do this, but if you run "top" at the same time you can see how the memory usage is affected. (I expect that Motemplate compilation will be quite expensive, but, prove me wrong!). https://codereview.chromium.org/521453003/diff/20001/chrome/common/extensions... chrome/common/extensions/docs/server2/compiled_file_system.py:244: cache_entry = self._GetFromFileStore(path).Get() These wrapper functions are nice, but if you had static _Get/_Set methods it wouldn't be too bad to write just: cache_entry = _Get(self._file_object_store, path).Get() Ditto _Set. Then we wouldn't need to add quite so many lines. https://codereview.chromium.org/521453003/diff/20001/chrome/common/extensions... chrome/common/extensions/docs/server2/compiled_file_system.py:250: self._SetToFileStore(path, _CacheEntry(cache_data, version)) (FWIW I think that "set in" reads slightly better than "set to"). https://codereview.chromium.org/521453003/diff/20001/chrome/common/extensions... File chrome/common/extensions/docs/server2/compiled_file_system_test.py (right): https://codereview.chromium.org/521453003/diff/20001/chrome/common/extensions... chrome/common/extensions/docs/server2/compiled_file_system_test.py:124: compiled_fs = _GetTestCompiledFsCreator()(Cache(identity), Is there a way to test this new functionality? https://codereview.chromium.org/521453003/diff/20001/chrome/common/extensions... File chrome/common/extensions/docs/server2/sidenav_data_source.py (right): https://codereview.chromium.org/521453003/diff/20001/chrome/common/extensions... chrome/common/extensions/docs/server2/sidenav_data_source.py:68: @Cache Why this in particular?
> Could you split this up into 2 CLs? I'd like to submit them > separately and see the affect each has on speed and memory > usage etc. Sure. I forgot to mention yesterday, but with the changes made to Walk, in cron_servlet_test.py we spend 36% less time in Walk than before (went from 3ish seconds to 2ish). https://codereview.chromium.org/521453003/diff/20001/chrome/common/extensions... File chrome/common/extensions/docs/server2/caching_file_system.py (right): https://codereview.chromium.org/521453003/diff/20001/chrome/common/extensions... chrome/common/extensions/docs/server2/caching_file_system.py:32: self._walk_cache = create_object_store('walk') On 2014/08/29 05:00:14, kalman wrote: > Should this be start_empty=False for the same reason as the 'read' cache is? Probably, I wasn't sure. https://codereview.chromium.org/521453003/diff/20001/chrome/common/extensions... File chrome/common/extensions/docs/server2/compiled_file_system_test.py (right): https://codereview.chromium.org/521453003/diff/20001/chrome/common/extensions... chrome/common/extensions/docs/server2/compiled_file_system_test.py:124: compiled_fs = _GetTestCompiledFsCreator()(Cache(identity), On 2014/08/29 05:00:14, kalman wrote: > Is there a way to test this new functionality? The tests here serve as a sort of regression test. Since CompiledFS no longer caches behind the scenes, I have to explicitly call Cache() on the comp_func whenever caching is expected. Removing the Cache() call causes these tests to fail, because they expect cached data but CompiledFS doesn't cache it. https://codereview.chromium.org/521453003/diff/20001/chrome/common/extensions... File chrome/common/extensions/docs/server2/sidenav_data_source.py (right): https://codereview.chromium.org/521453003/diff/20001/chrome/common/extensions... chrome/common/extensions/docs/server2/sidenav_data_source.py:68: @Cache On 2014/08/29 05:00:14, kalman wrote: > Why this in particular? The test for this is expecting cached data, and I thought it would be easier to keep caching assumptions the same in this CL, then in another we can cache less.
On 2014/08/29 17:20:00, ahernandez wrote: > > Could you split this up into 2 CLs? I'd like to submit them > separately and > see the affect each has on speed and memory > usage etc. > > Sure. I forgot to mention yesterday, but with the changes made to Walk, in > cron_servlet_test.py we spend 36% less time in Walk than before (went from 3ish > seconds to 2ish). > Wow - that's awesome. I was thinking this morning that providing an alternative implementation isn't so bad given it's so short.
On 2014/08/29 17:46:11, kalman wrote: > On 2014/08/29 17:20:00, ahernandez wrote: > > > Could you split this up into 2 CLs? I'd like to submit them > separately and > > see the affect each has on speed and memory > usage etc. > > > > Sure. I forgot to mention yesterday, but with the changes made to Walk, in > > cron_servlet_test.py we spend 36% less time in Walk than before (went from > 3ish > > seconds to 2ish). > > > > Wow - that's awesome. > > I was thinking this morning that providing an alternative implementation isn't > so bad given it's so short. Well, I just wrote something in delegate style so I'll send it when it's ready and you can check it out. I like the idea of re-using code, I was just being lazy :)
> Well, I just wrote something in delegate style so I'll send it when it's ready > and you can check it out. I like the idea of re-using code, I was just being > lazy :) Even better!
On 2014/08/29 17:49:51, kalman wrote: > > Well, I just wrote something in delegate style so I'll send it when it's ready > > and you can check it out. I like the idea of re-using code, I was just being > > lazy :) > > Even better! This patch is now just the changes to Walk. I modeled it in a delegate style (I think). Only potential caveat is that we have the additional function call overhead with the new walk_delegate, but I can't imagine that it'll add that much to the execution time.
https://codereview.chromium.org/521453003/diff/40001/chrome/common/extensions... File chrome/common/extensions/docs/server2/caching_file_system.py (right): https://codereview.chromium.org/521453003/diff/40001/chrome/common/extensions... chrome/common/extensions/docs/server2/caching_file_system.py:32: self._walk_cache = create_object_store('walk', start_empty=False) A more consistent name for this would be "_walk_object_store". I do like "_walk_cache" though, if you feel inspired you could just rename stat/read_object_store to stat/read_cache. https://codereview.chromium.org/521453003/diff/40001/chrome/common/extensions... chrome/common/extensions/docs/server2/caching_file_system.py:150: root_stat = self.Stat(root) StatAsync. And you could do these in parallel pretty easily using All(); but should this follow Read more closely? Like, cache the version inside the walk cache, then compare that to the version that comes back from StatAsync? Like not even bother looking up _stat_object_store yourself. https://codereview.chromium.org/521453003/diff/40001/chrome/common/extensions... File chrome/common/extensions/docs/server2/file_system.py (right): https://codereview.chromium.org/521453003/diff/40001/chrome/common/extensions... chrome/common/extensions/docs/server2/file_system.py:167: |walk_delegate|, if specified, should be a callback of signature Neat! How about a more descriptive name than |walk_delegate|? Something that expresses that it reads the files and dirs out of a single level of a walk.
PTAL. https://codereview.chromium.org/521453003/diff/80001/chrome/common/extensions... File chrome/common/extensions/docs/server2/caching_file_system.py (right): https://codereview.chromium.org/521453003/diff/80001/chrome/common/extensions... chrome/common/extensions/docs/server2/caching_file_system.py:151: if res and res[2] == root_stat.version: Sorry I keep flipping around this if condition, I'm just trying to pick the version that reads the easiest.
lgtm https://codereview.chromium.org/521453003/diff/80001/chrome/common/extensions... File chrome/common/extensions/docs/server2/caching_file_system.py (right): https://codereview.chromium.org/521453003/diff/80001/chrome/common/extensions... chrome/common/extensions/docs/server2/caching_file_system.py:147: def delegate(root): file_lister? https://codereview.chromium.org/521453003/diff/80001/chrome/common/extensions... chrome/common/extensions/docs/server2/caching_file_system.py:151: if res and res[2] == root_stat.version: On 2014/08/29 20:13:41, ahernandez wrote: > Sorry I keep flipping around this if condition, I'm just trying to pick the > version that reads the easiest. This looks good. https://codereview.chromium.org/521453003/diff/80001/chrome/common/extensions... File chrome/common/extensions/docs/server2/caching_file_system_test.py (right): https://codereview.chromium.org/521453003/diff/80001/chrome/common/extensions... chrome/common/extensions/docs/server2/caching_file_system_test.py:291: self.assertTrue(*mock_fs.CheckAndReset()) Could you also test with a new CachingFileSystem? Calling _CreateCachingFileSystem multiple times from the same test will result in the same object store being returned, so I'd expect a Stat of 1 each time. Or maybe 5 each time. Ideally 1 because Walk could be smart enough to know that the file system hasn't changed by Stat-ing the root. Maybe StatAsync already has this optimisation and it's not worth worrying about. Or you can TODO-ify this. If it still gets a 30% improvement we should submit it now.
Bump app.yaml though.
The CQ bit was checked by ahernandez.miralles@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ahernandez.miralles@gmail.com/52145300...
Message was sent while issue was closed.
Committed patchset #4 (id:100001) as 62ba33b84ac86776d0506136575a175d0afb57b1
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e95c9e298410a846a8bf941833b6aa8357c5560a Cr-Commit-Position: refs/heads/master@{#292708} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
