Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12794003:
Initialize the simple cache backend at runtime. (Closed)
|
Created:
7 years, 9 months ago by pasko-google - do not use Modified:
7 years, 8 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, gavinp+disk_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionInitialize the simple cache backend at runtime.
We will need to choose the backend at runtime for A/B testing. This change is
the first step towards backend comparisons: it moves the logic of choosing among
cache backends to a single point (cache_creator.cc) and makes it logically
separate from BackendImpl. Next step would be to inject performance-analyzer
backend into this cache backend choice logic. The analyzer is going to be under
a compile-time flag.
With this change the Simple Cache Backend operation gets enabled when the
"SimpleCacheTrial" experiment is set.
Cache Initialization.
The Simple Cache Backend now shares the common initialization/cleanup code. So
it can delete inconsistent cache, cache of a different version etc
asynchronously. It is just a positive side-effect of moving the backend choice
to a single point.
The CacheCreator with almost no change.
To see the difference:
shell> out/Debug/chrome --enable-logging=stderr --v=1 \
--user-data-dir=/tmp/unique --use-simple-cache-backend=on \
http://url 2>&1 | grep "Simple Cache"
BUG=173390, 173384
TBR=maruel
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=191830
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #Patch Set 4 : fixed a test #Patch Set 5 : . #Patch Set 6 : . #
Total comments: 28
Patch Set 7 : . #Patch Set 8 : #Patch Set 9 : #
Total comments: 1
Patch Set 10 : #Patch Set 11 : #
Total comments: 7
Patch Set 12 : #
Total comments: 32
Patch Set 13 : #Patch Set 14 : upload from git #
Total comments: 5
Patch Set 15 : #
Total comments: 57
Patch Set 16 : #
Total comments: 2
Patch Set 17 : #Patch Set 18 : #Patch Set 19 : #Patch Set 20 : #Patch Set 21 : #Patch Set 22 : uploading the same code from git for CQ happiness #Patch Set 23 : exclude ScopedAllowIO in net/disk_cache/cache_util.cc, it is not new code #Patch Set 24 : rebased to latest, auto-merged net.gyp #Messages
Total messages: 33 (0 generated)
Looks good. My requests are mostly nits, except for the about:flags, which would be nice to have. But the about:flags can also be punted to a follow up CL. https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/backend_im... File net/disk_cache/backend_impl.cc (right): https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/backend_im... net/disk_cache/backend_impl.cc:78: // is the current folder location, and name is the current folder name. I suspect you want to remove/move this comment, too. https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/cache_crea... File net/disk_cache/cache_creator.cc (right): https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/cache_crea... net/disk_cache/cache_creator.cc:21: const char kUseSimpleCacheBackend[] = "use-simple-cache-backend"; I'm a bit uncomfortable with the command line option being in this .cc file. But I think net/base/net_switches.h/cc is a silly idea too. So I guess it's fine here. Can we add this to about:flags though? Then we can all use canary and always use the simple backend very easily on desktop. https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/cache_crea... net/disk_cache/cache_creator.cc:42: base::FilePath GetTempCacheName(const base::FilePath& path, The comment for this function has been orphaned over in backend_impl.cc. Maybe move it over here? https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/cache_crea... net/disk_cache/cache_creator.cc:53: // Builds the instance of the backend depending on command-line option. Takes "Builds an instance of the backend depending on command-line options." https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/cache_crea... net/disk_cache/cache_creator.cc:57: CacheCreator(const base::FilePath& path, bool force, int max_bytes, This declaration is formally a violation of our coding style, see http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... . I know you're cutting and pasting here, so perhaps the consistency rule says we can propogate the violation. But my inclination is to use this as a chance to clean up the code. https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/cache_crea... net/disk_cache/cache_creator.cc:116: #if defined(USE_SIMPLE_CACHE_BACKEND) This #define probably needs to be renamed now to BUILD_SIMPLE_CACHE_BACKEND or something. Too confusing otherwise. https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/cache_crea... net/disk_cache/cache_creator.cc:128: disk_cache::BackendImpl* new_cache = new disk_cache::BackendImpl( Why the automatic? Why not: created_cache_ = new disk_cache::... ? https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/disk_cache.h File net/disk_cache/disk_cache.h (right): https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/disk_cache... net/disk_cache/disk_cache.h:64: // TODO(pasko): eliminate this call. I think it's best if the stuff after the : is a full sentence, so capitalize the "Eliminate." Also, to reduce repetitive redundant repetitions, eliminate all but the first sentence from the comment, and expand the Todo a bit to something like "Make cleanup changes in WebKit to eliminate this call" or something.
After a superficial review: - Moving creation code to a separate file++ - Command line-- - Concerns with tests. I don't see why a caller should worry at run time about what cache to instantiate. Ideally, not all implementations will be compiled into an executable. But of course, we need to test stuff. On that front, I would also prefer not to be able to pick a given implementation unless said implementation is already complete. (assuming that there is an explicit command line option somewhere) Let's fast forward in time and say that we are ready for real testing. At that time we'll build Chrome with more than one backend implementation, and we'll setup an A/B test to select one or the other and gather relevant data. This doesn't mean that the caller (HTTP or higher layer) would have to explicitly pass a flag or something to select the backend to use. It means that the embedder sets up the experiment and net decides what to do as needed. I think we should just write directly to that model. Just assume that a field trial will select the implementation to use. Note that there's code to select a group of a given experiment through Chrome's command line, so if what you want is to play with Chrome running something else we can do that today without introducing unwanted dependencies. Now testing. This CL also modifies tests that are explicitly selecting one implementation to go through the public API. At the end of the day we should have almost all current tests running against all implementations. That means that changes that operate on "all" tests at the same time are fine, but tests that are specific to one implementation are more troublesome... we may need to keep simple factory methods for a given implementation (note that there are tests that don't use the private factory but instead create an instance if the desired class and set flags as needed. Most of the work done by the real factory (async, self cleaning) is not needed for a test). So the testing infrastructure should be able to select a given implementation based on what the test wants. This is not appropriate for the field trial infrastructure (nor a command line), but again, tests in general don't have to go through the public interface. https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/backend_un... File net/disk_cache/backend_unittest.cc (right): https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/backend_un... net/disk_cache/backend_unittest.cc:220: // Test the private factory methods. If there's no private factory method, this part of the test must be removed. https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/cache_crea... File net/disk_cache/cache_creator.cc (right): https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/cache_crea... net/disk_cache/cache_creator.cc:4: Could you copy this file from backend_impl.cc? (svn cpy). I'd like to see an A+ instead of A on the CL so that we in the future we can navigate seamlessly across this CL. That would also help to see what are the changes to the code (I'm assuming that this is mostly a move, not new code from scratch) https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/disk_cache.h File net/disk_cache/disk_cache.h (right): https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/disk_cache... net/disk_cache/disk_cache.h:19: #include "net/disk_cache/disk_cache.h" ah? https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/disk_cache... net/disk_cache/disk_cache.h:62: // A slightly shorter version than the above. It is invoked from WebKit and what is webkit? https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/disk_cache... net/disk_cache/disk_cache.h:74: bool DelayedCacheCleanup(const base::FilePath& full_path); Why is this part of the interface?
On 2013/03/13 19:36:00, rvargas wrote: > Now testing. This CL also modifies tests that are explicitly selecting one > implementation to go through the public API. At the end of the day we should > have almost all current tests running against all implementations. That means > that changes that operate on "all" tests at the same time are fine, but tests > that are specific to one implementation are more troublesome... we may need to > keep simple factory methods for a given implementation (note that there are > tests that don't use the private factory but instead create an instance if the > desired class and set flags as needed. Most of the work done by the real factory > (async, self cleaning) is not needed for a test). > > So the testing infrastructure should be able to select a given implementation > based on what the test wants. This is not appropriate for the field trial > infrastructure (nor a command line), but again, tests in general don't have to > go through the public interface. I have left a TODO(pasko,gavinp) to sort out the tests. It is best to do in a separate CL, or maybe even more than one. Some thoughts on this. I did not find a test that checks the force/retry logic of the CreateCacheBackend(). But there are a few tests that check it via BackendImpl internals. I think those should switch to using CacheCreator directly and ask it to use the BackendImpl. Other tests should (probably) use implementation-specific switch for selecting a backend, similarly as SetNewEviction() or SetMemoryOnlyMode().
OK, I did not figure out (yet) why the new function cannot be found at link time on some bots, but is doing fine on others. I think the problem is minor, so please take a look at how I addressed the comments, updated the commit message, etc. https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/backend_im... File net/disk_cache/backend_impl.cc (right): https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/backend_im... net/disk_cache/backend_impl.cc:78: // is the current folder location, and name is the current folder name. On 2013/03/13 18:36:12, gavinp wrote: > I suspect you want to remove/move this comment, too. Oops. Done. https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/backend_un... File net/disk_cache/backend_unittest.cc (right): https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/backend_un... net/disk_cache/backend_unittest.cc:220: // Test the private factory methods. On 2013/03/13 19:36:01, rvargas wrote: > If there's no private factory method, this part of the test must be removed. Done. https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/cache_crea... File net/disk_cache/cache_creator.cc (right): https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/cache_crea... net/disk_cache/cache_creator.cc:4: On 2013/03/13 19:36:01, rvargas wrote: > Could you copy this file from backend_impl.cc? (svn cpy). I'd like to see an A+ > instead of A on the CL so that we in the future we can navigate seamlessly > across this CL. > > That would also help to see what are the changes to the code (I'm assuming that > this is mostly a move, not new code from scratch) Done. https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/cache_crea... net/disk_cache/cache_creator.cc:21: const char kUseSimpleCacheBackend[] = "use-simple-cache-backend"; On 2013/03/13 18:36:12, gavinp wrote: > I'm a bit uncomfortable with the command line option being in this .cc file. But > I think net/base/net_switches.h/cc is a silly idea too. > > So I guess it's fine here. > > Can we add this to about:flags though? Then we can all use canary and always use > the simple backend very easily on desktop. Switches are committed in the previous CL, about:flags will get in later. https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/cache_crea... net/disk_cache/cache_creator.cc:42: base::FilePath GetTempCacheName(const base::FilePath& path, On 2013/03/13 18:36:12, gavinp wrote: > The comment for this function has been orphaned over in backend_impl.cc. Maybe > move it over here? Oops. Done. https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/cache_crea... net/disk_cache/cache_creator.cc:53: // Builds the instance of the backend depending on command-line option. Takes On 2013/03/13 18:36:12, gavinp wrote: > "Builds an instance of the backend depending on command-line options." Done. https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/cache_crea... net/disk_cache/cache_creator.cc:57: CacheCreator(const base::FilePath& path, bool force, int max_bytes, On 2013/03/13 18:36:12, gavinp wrote: > This declaration is formally a violation of our coding style, see > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... > . > > I know you're cutting and pasting here, so perhaps the consistency rule says we > can propogate the violation. But my inclination is to use this as a chance to > clean up the code. Can you be more specific about the violation? https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/cache_crea... net/disk_cache/cache_creator.cc:116: #if defined(USE_SIMPLE_CACHE_BACKEND) On 2013/03/13 18:36:12, gavinp wrote: > This #define probably needs to be renamed now to BUILD_SIMPLE_CACHE_BACKEND or > something. Too confusing otherwise. Removed altogether as per other discussions. https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/cache_crea... net/disk_cache/cache_creator.cc:128: disk_cache::BackendImpl* new_cache = new disk_cache::BackendImpl( On 2013/03/13 18:36:12, gavinp wrote: > Why the automatic? Why not: > > created_cache_ = new disk_cache::... ? We use a few methods that are not present in BackendImpl's superclass. https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/disk_cache.h File net/disk_cache/disk_cache.h (right): https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/disk_cache... net/disk_cache/disk_cache.h:19: #include "net/disk_cache/disk_cache.h" On 2013/03/13 19:36:01, rvargas wrote: > ah? Done. https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/disk_cache... net/disk_cache/disk_cache.h:62: // A slightly shorter version than the above. It is invoked from WebKit and On 2013/03/13 19:36:01, rvargas wrote: > what is webkit? oh, I incorrectly interpreted the linker error message, too many words. In fact it was called from src/webkit/.. which is in the same repository and can be changed in this CL. Rephrased the comment. https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/disk_cache... net/disk_cache/disk_cache.h:64: // TODO(pasko): eliminate this call. On 2013/03/13 18:36:12, gavinp wrote: > I think it's best if the stuff after the : is a full sentence, so capitalize the > "Eliminate." Also, to reduce repetitive redundant repetitions, eliminate all but > the first sentence from the comment, and expand the Todo a bit to something like > "Make cleanup changes in WebKit to eliminate this call" or something. Agreed. Good general note. This time it does not require a cleanup though because exposing flags to all callers of the method is unnecessary exposure. If once backends start sharing flags we would probably introduce something like disk_cache_internal.h https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/disk_cache... net/disk_cache/disk_cache.h:74: bool DelayedCacheCleanup(const base::FilePath& full_path); On 2013/03/13 19:36:01, rvargas wrote: > Why is this part of the interface? It should not necessary be a part of a generic cache backend interface. I resisted creating yet another header file that would be shared among backends, and not shared with most common backend creation callsites. Good name for such internal backend header file would be: net/disk_cache/disk_cache_internal.h Suitable interfaces to reside in it: CreateCacheBackendForTesting() disk_cache::BackendFlags DelayedCacheCleanup() What do you think? https://codereview.chromium.org/12794003/diff/40011/net/disk_cache/cache_crea... File net/disk_cache/cache_creator.cc (right): https://codereview.chromium.org/12794003/diff/40011/net/disk_cache/cache_crea... net/disk_cache/cache_creator.cc:54: } // namespace I made this jumpy anonymous/disk_cache/anonymous/disk_cache namespace switch to keep the order of DelayedCacheCleanup and CacheCreator so that reviewing the difference is easier. Whew.
o-kay, .. in Russia we have common wisdom: "Things sort out better in the morning". The phrase is just three words, can you imagine that? So the tries all passed finally without me doing anything. I am flying away today, so there is no great urgency in reviewing this. Though moving code around and renaming files in masses often results in loss of intermediate commits, so I'd try to commit this as soon as possible.
At this point I don't think we should have a shared internal method for creation of the backend... we still want the one that creates public backends based on the interface, but now I don't like to reuse the same factory for tests. I think that unit tests should rely on the code from disk_cache_test_base to build the right cache. Note that the common case of creating a disk cache (with new_eviction_) goes through the path of instantiating the object and setting the flags, without even calling the static factory method of BackendImpl. And the class already keeps dedicated pointers for each type of backend, so no cast are required when a test wants to use something of a specific backend. If there are lots of tests that use the old static factory method, a new helper on the test framework is trivial to write. The only other user of a private method is crash_cache, and I prefer to pay the price of creating the cache by hand in that case. The "hard" part of creating a cache is to do cleanup, and that's only required for the "real" cache, not for tests or utilities. So what I'm saying is: CreateCacheBackend doesn't care about flags, selects the production backend to use and does cleanup if needed. Any user interested in a specific backend instantiates the object that it needs, sets flags if needed, and calls Init(). Tests just follow the same pattern of calling a method like SetMemoryOnlyMode() to select a specific backend to be built. https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/disk_cache.h File net/disk_cache/disk_cache.h (right): https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/disk_cache... net/disk_cache/disk_cache.h:74: bool DelayedCacheCleanup(const base::FilePath& full_path); On 2013/03/18 15:47:07, pasko wrote: > On 2013/03/13 19:36:01, rvargas wrote: > > Why is this part of the interface? > > It should not necessary be a part of a generic cache backend interface. I > resisted creating yet another header file that would be shared among backends, > and not shared with most common backend creation callsites. > > Good name for such internal backend header file would be: > net/disk_cache/disk_cache_internal.h > > Suitable interfaces to reside in it: > CreateCacheBackendForTesting() > disk_cache::BackendFlags > DelayedCacheCleanup() > > What do you think? How about cache_creator.h? There's already cache_util.h, disk_cache_test_base.h and disk_cache_test_util.h (if this is only for tests)... better not to add another generic sounding header. Why do you want DelayedCacheCleanup() being accessible? I'm just curious, I don't mind it (but cache_util already has something related). as for the flags... it makes sense to share them, but so far they are extremely implementation dependent (and that's why they are not exposed to formal users). In other words, especial care is needed when dealing with them. Another detail to consider is that not everything can be done with CreateCBWithFlags... there is still special purpose code in disk_cache_test_base that instantiates an object and does something with it before calling Init() (showing that this was the original intended pattern, but I somewhat changed my mind... or rather, the list of formal arguments grew over time). Which leads me to believe that the internal creator maybe needs something like: struct CreationParameters { type; size; flags; etc } CreateBackend(parameters, &backend, callback); So we would have the freedom to add as many things to the _internal_ parameters as we want. On a related note, I'm not sure I like to see automatic cleanup (force) being part of such interface... I would prefer explicit users of the internal interface to always have to deal with failures. But I guess it is just easier to leave it there waiting for some weird use case to appear. And I just realized that even this model doesn't work that well... what type would we return? having to cast to get an _impl version is not good. I guess I better read the rest of the CL. https://codereview.chromium.org/12794003/diff/53004/net/disk_cache/cache_crea... File net/disk_cache/cache_creator.cc (right): https://codereview.chromium.org/12794003/diff/53004/net/disk_cache/cache_crea... net/disk_cache/cache_creator.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. nit: don't change the year https://codereview.chromium.org/12794003/diff/53004/net/disk_cache/cache_crea... net/disk_cache/cache_creator.cc:102: // Builds an instance of the backend depending on command-line option. Takes nit: command line? https://codereview.chromium.org/12794003/diff/53004/net/disk_cache/cache_crea... net/disk_cache/cache_creator.cc:154: net_log_(net_log) { nit: any reason to move empty methods offline? https://codereview.chromium.org/12794003/diff/53004/net/disk_cache/cache_crea... net/disk_cache/cache_creator.cc:161: if (type_ == net::MEMORY_CACHE) { nit: do this directly on CreateCacheBackend. https://codereview.chromium.org/12794003/diff/53004/net/disk_cache/cache_crea... net/disk_cache/cache_creator.cc:229: int max_bytes, bool force, uint32 flags, base::MessageLoopProxy* thread, nit: indent under the first arg. https://codereview.chromium.org/12794003/diff/53004/net/disk_cache/disk_cache.h File net/disk_cache/disk_cache.h (right): https://codereview.chromium.org/12794003/diff/53004/net/disk_cache/disk_cache... net/disk_cache/disk_cache.h:59: NET_EXPORT int CreateCacheBackendWithFlags( Regardless of the file, the correct declaration would be NEXT_EXPORT_PRIVATE https://codereview.chromium.org/12794003/diff/53004/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_backend_impl.h (right): https://codereview.chromium.org/12794003/diff/53004/net/disk_cache/simple/sim... net/disk_cache/simple/simple_backend_impl.h:32: static int CreateBackend(const base::FilePath& full_path, this method also goes away, right?
https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/disk_cache.h File net/disk_cache/disk_cache.h (right): https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/disk_cache... net/disk_cache/disk_cache.h:74: bool DelayedCacheCleanup(const base::FilePath& full_path); In case it is not clear, this comment is basically stale given my last reply.
Ricardo, with this upload I skipped smaller nits and mainly addressed the testing suggestions and privatizing the internal backend creation interface. Please take a top-level scan. The testing code is going to have less state and will do less unnecessary work, which should make it easier to read. The part of the description added to the CL: ========================== The CacheCreator was moved with a minor change: make it also decide about choosing in-memory cache. All the cleanup-retry logic is intact. Tests were sorted out in a way: * Now it is possible to enable some tests with the Simple Backend by issuing SetSimpleCacheMode(). * The DirectMode (implementation_ == true) is now the default, since most of the time we want to avoid testing the cache restart logic. * Some tests verify CacheCreator functionality, but use the knowledge about BackendImpl internals to do so, like DeleteOld test. That one was split into two tests: 1. check that BackendImpl fails on encountering a cache with wrong version 2. check that the CacheCreator is able to cleanup and restart ========================== On 2013/03/18 22:25:25, rvargas wrote: > At this point I don't think we should have a shared internal method for creation > of the backend... we still want the one that creates public backends based on > the interface, but now I don't like to reuse the same factory for tests. Agreed. Internally the tests would instantiate the CacheCreator directly if they want to, and that should be discourages except cases when we test cache restarts (which is small compared to everything else). > I think that unit tests should rely on the code from disk_cache_test_base to > build the right cache. Note that the common case of creating a disk cache (with > new_eviction_) goes through the path of instantiating the object and setting the > flags, without even calling the static factory method of BackendImpl. And the > class already keeps dedicated pointers for each type of backend, so no cast are > required when a test wants to use something of a specific backend. Yes. Added CreateBackendAsModeSuggests() utility function to create the backend according to the test settings. > If there are lots of tests that use the old static factory method, a new helper > on the test framework is trivial to write. > > The only other user of a private method is crash_cache, and I prefer to pay the > price of creating the cache by hand in that case. The "hard" part of creating a > cache is to do cleanup, and that's only required for the "real" cache, not for > tests or utilities. crash_cache is BackendImpl-specific, manually cerating the BackendImpl sounds like the right choice. > So what I'm saying is: CreateCacheBackend doesn't care about flags, selects the > production backend to use and does cleanup if needed. > > Any user interested in a specific backend instantiates the object that it needs, > sets flags if needed, and calls Init(). > > Tests just follow the same pattern of calling a method like SetMemoryOnlyMode() > to select a specific backend to be built. Indeed. Added SetSimpleCacheMode(), using that is going to see light in next CLs. > https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/disk_cache.h > File net/disk_cache/disk_cache.h (right): > > https://codereview.chromium.org/12794003/diff/19001/net/disk_cache/disk_cache... > net/disk_cache/disk_cache.h:74: bool DelayedCacheCleanup(const base::FilePath& > full_path); > On 2013/03/18 15:47:07, pasko wrote: > > On 2013/03/13 19:36:01, rvargas wrote: > > > Why is this part of the interface? > > > > It should not necessary be a part of a generic cache backend interface. I > > resisted creating yet another header file that would be shared among backends, > > and not shared with most common backend creation callsites. > > > > Good name for such internal backend header file would be: > > net/disk_cache/disk_cache_internal.h > > > > Suitable interfaces to reside in it: > > CreateCacheBackendForTesting() > > disk_cache::BackendFlags > > DelayedCacheCleanup() > > > > What do you think? > > How about cache_creator.h? There's already cache_util.h, disk_cache_test_base.h > and disk_cache_test_util.h (if this is only for tests)... better not to add > another generic sounding header. I've put it into net/disk_cache/cache_util.h, I am also fine with cache_creator.h, though addind a new file for it did not sound very nice. > Why do you want DelayedCacheCleanup() being accessible? I'm just curious, I > don't mind it (but cache_util already has something related). The BackendImpl is using it occasionally, so I thought it is a useful utility method for cache backends generally. > as for the flags... it makes sense to share them, but so far they are extremely > implementation dependent (and that's why they are not exposed to formal users). > In other words, especial care is needed when dealing with them. Yes, I did not decide yet what to do with flags. Luckily the simple cache backend does not have ways to alter behavior. > Another detail to consider is that not everything can be done with > CreateCBWithFlags... there is still special purpose code in disk_cache_test_base > that instantiates an object and does something with it before calling Init() > (showing that this was the original intended pattern, but I somewhat changed my > mind... or rather, the list of formal arguments grew over time). > > Which leads me to believe that the internal creator maybe needs something like: > > struct CreationParameters { type; size; flags; etc } > CreateBackend(parameters, &backend, callback); > > So we would have the freedom to add as many things to the _internal_ parameters > as we want. I think for testing we can just get along with modes, and then backend creation logic in disk_cache_test_base will take care about it. > On a related note, I'm not sure I like to see automatic cleanup (force) being > part of such interface... I would prefer explicit users of the internal > interface to always have to deal with failures. But I guess it is just easier to > leave it there waiting for some weird use case to appear. Same here. Got rid of automatic cleanup wherever I could. > And I just realized that even this model doesn't work that well... what type > would we return? having to cast to get an _impl version is not good. I guess I > better read the rest of the CL. We will have cache_, cache_impl_, and cache_simple_ for performing generic/specific operations, should work in most cases.
> * Now it is possible to enable some tests with the Simple > Backend by issuing SetSimpleCacheMode(). For the new backend I went for UseVersion3()(*). I know it diverges from the SetXX, but I think it results in more clear tests. Just saying... (*) yes, I have a local version of disk_cache_test_base.* https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/cache_creat... File net/disk_cache/cache_creator.cc (right): https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/cache_creat... net/disk_cache/cache_creator.cc:64: bool DelayedCacheCleanup(const base::FilePath& full_path) { move this to cache_util.cc https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/cache_creat... net/disk_cache/cache_creator.cc:97: CacheCreator::CacheCreator( Missing the class declaration https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/cache_creat... net/disk_cache/cache_creator.cc:189: CacheCreator* creator = new CacheCreator(path, force, max_bytes, type, kNone, nit: I'd remove flags from the CacheCreator interface. This should be the only place where a CacheCreator is used, and kNone is the only value used... which is a convoluted way to avoid calling SetFlags() https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/disk_cache.h File net/disk_cache/disk_cache.h (right): https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/disk_cache.... net/disk_cache/disk_cache.h:59: bool DelayedCacheCleanup(const base::FilePath& full_path); move this to cache_util.h https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/disk_cache_... File net/disk_cache/disk_cache_test_base.cc (left): https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/disk_cache_... net/disk_cache/disk_cache_test_base.cc:286: cache_ = cache_impl_; missing this line https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/disk_cache_... File net/disk_cache/disk_cache_test_base.cc (right): https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/disk_cache_... net/disk_cache/disk_cache_test_base.cc:69: disk_cache::Backend* DiskCacheTestWithCache::CreateBackendAsModeSuggests( looks like the header is not part of the CL... without knowing if I'm missing something, don't move the code (keep it where InitDiskCacheImpl was), to follow changes easier. https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/disk_cache_... net/disk_cache/disk_cache_test_base.cc:72: if (use_current_thread_ || !thread) !thread looks like a critical error that should not be masked https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/disk_cache_... net/disk_cache/disk_cache_test_base.cc:77: if (simple_cache_mode_ && type_ == net::DISK_CACHE) { The type should be under control of the specific test, not the framework. https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/disk_cache_... net/disk_cache/disk_cache_test_base.cc:80: int rv = disk_cache::SimpleBackendImpl::CreateBackend(cache_path_, size_, Why a static factory method instead of following the same pattern of ctor + init? https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/disk_cache_... net/disk_cache/disk_cache_test_base.cc:81: type_, disk_cache::kNone, make_scoped_refptr(runner), NULL, I think we should assume for the time being that flags are not shared between implementations. (doesn't have to be fixed by this CL) https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/disk_cache_... net/disk_cache/disk_cache_test_base.cc:124: cache_ = CreateBackendAsModeSuggests(disk_cache::kNoRandom, &cache_thread_); Why exposing the flags if all callers pass the same value? https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/disk_cache_... net/disk_cache/disk_cache_test_base.cc:283: EXPECT_TRUE(CleanupCacheDir()); Why change this to an EXPECT? Seems better to skip the rest of the test at this point. https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/disk_cache_... net/disk_cache/disk_cache_test_base.cc:289: EXPECT_TRUE(cache_thread_.message_loop() != NULL); Definitely an assert https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/disk_cache_... net/disk_cache/disk_cache_test_base.cc:306: disk_cache::CacheCreator* creator = new disk_cache::CacheCreator(cache_path_, Call CreateCacheBackend() instead.
https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/cache_creat... File net/disk_cache/cache_creator.cc (right): https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/cache_creat... net/disk_cache/cache_creator.cc:64: bool DelayedCacheCleanup(const base::FilePath& full_path) { On 2013/03/25 22:06:04, rvargas wrote: > move this to cache_util.cc Done. https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/cache_creat... net/disk_cache/cache_creator.cc:97: CacheCreator::CacheCreator( On 2013/03/25 22:06:04, rvargas wrote: > Missing the class declaration I've put the declaration to the net/disk_cache/cache_util.h, but appears it was not intuitive, let me know and I'll revive the cache_creator.h https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/cache_creat... net/disk_cache/cache_creator.cc:189: CacheCreator* creator = new CacheCreator(path, force, max_bytes, type, kNone, On 2013/03/25 22:06:04, rvargas wrote: > nit: I'd remove flags from the CacheCreator interface. This should be the only > place where a CacheCreator is used, and kNone is the only value used... which is > a convoluted way to avoid calling SetFlags() I'm always happy to remove extra unneeded args! We have some tests for CacheCreator cleanup functionality that use kNoRandom which I thought was essential. If it's not I'll go ahead and remove the flags with no regret. https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/disk_cache.h File net/disk_cache/disk_cache.h (right): https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/disk_cache.... net/disk_cache/disk_cache.h:59: bool DelayedCacheCleanup(const base::FilePath& full_path); On 2013/03/25 22:06:04, rvargas wrote: > move this to cache_util.h Done. https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/disk_cache_... File net/disk_cache/disk_cache_test_base.cc (left): https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/disk_cache_... net/disk_cache/disk_cache_test_base.cc:286: cache_ = cache_impl_; On 2013/03/25 22:06:04, rvargas wrote: > missing this line Good catch! Done. https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/disk_cache_... File net/disk_cache/disk_cache_test_base.cc (right): https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/disk_cache_... net/disk_cache/disk_cache_test_base.cc:69: disk_cache::Backend* DiskCacheTestWithCache::CreateBackendAsModeSuggests( On 2013/03/25 22:06:04, rvargas wrote: > looks like the header is not part of the CL... without knowing if I'm missing > something, don't move the code (keep it where InitDiskCacheImpl was), to follow > changes easier. argh, I forgot that I need to add files manually with svn-based checkout, sorry about that. Uploading the better one. Oh, and moving the code down in the file so that the difference is more visible. https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/disk_cache_... net/disk_cache/disk_cache_test_base.cc:72: if (use_current_thread_ || !thread) On 2013/03/25 22:06:04, rvargas wrote: > !thread looks like a critical error that should not be masked BackendShutdownWithPendingFileIO was using this codepath for reason unknown to me, and you are right, it is better to as use_current_thread_. Done. https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/disk_cache_... net/disk_cache/disk_cache_test_base.cc:77: if (simple_cache_mode_ && type_ == net::DISK_CACHE) { On 2013/03/25 22:06:04, rvargas wrote: > The type should be under control of the specific test, not the framework. Done. https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/disk_cache_... net/disk_cache/disk_cache_test_base.cc:80: int rv = disk_cache::SimpleBackendImpl::CreateBackend(cache_path_, size_, On 2013/03/25 22:06:04, rvargas wrote: > Why a static factory method instead of following the same pattern of ctor + > init? There is no real reason, I just avoided changing with this CL to be less in conflict with that Felipe is doing. Added a TODO. https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/disk_cache_... net/disk_cache/disk_cache_test_base.cc:81: type_, disk_cache::kNone, make_scoped_refptr(runner), NULL, On 2013/03/25 22:06:04, rvargas wrote: > I think we should assume for the time being that flags are not shared between > implementations. (doesn't have to be fixed by this CL) Yes. I am seeing this as .. for testing reasons we would probably have 'struct BackendCreationParams' that may contain both kind of flags if a test needs analogous things to request from both backend variants. Let's wait for first interesting parameters for the Simple Cache. https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/disk_cache_... net/disk_cache/disk_cache_test_base.cc:124: cache_ = CreateBackendAsModeSuggests(disk_cache::kNoRandom, &cache_thread_); On 2013/03/25 22:06:04, rvargas wrote: > Why exposing the flags if all callers pass the same value? In BackendShutdownWithPending* we sometimes add kNoBuffering to flags, the simple cache ignores that. I did not create yet another layer of flags just because there was no immediate use of it for the simple cache. https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/disk_cache_... net/disk_cache/disk_cache_test_base.cc:283: EXPECT_TRUE(CleanupCacheDir()); On 2013/03/25 22:06:04, rvargas wrote: > Why change this to an EXPECT? Seems better to skip the rest of the test at this > point. I recently discovered that ASSERT_TRUE does only return from the current function, and reports an error, but it does not wind down the whole test. Also it fails to compile if invoked from non-void helper functions because it does a 'return something()' where 'something()' returns void. I can leave ASSERT_TRUE here if it looks better, since this function returns void, it would work, though it would be roughly equivalent to EXPECT_TRUE. So for consistency I prefer EXPECT_* in helper functions if we want to check rare/unlikely failures in helper functions intended only for testing. Cleaner way would be to return success/failure from the helper functions, but it is a little too heavy for this CL already. https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/disk_cache_... net/disk_cache/disk_cache_test_base.cc:289: EXPECT_TRUE(cache_thread_.message_loop() != NULL); On 2013/03/25 22:06:04, rvargas wrote: > Definitely an assert Same as above, if you want me to revert, I am OK with that, just bringing attention to irrelevant detail :) https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/disk_cache_... net/disk_cache/disk_cache_test_base.cc:306: disk_cache::CacheCreator* creator = new disk_cache::CacheCreator(cache_path_, On 2013/03/25 22:06:04, rvargas wrote: > Call CreateCacheBackend() instead. My main motivation was in the need of disk_cache::kNoRandom. Is it still needed by the tests (to be non-flaky?) or the flag is obsolete?
oh shoot, please don't look at this yet, my svn/gcl client is busted, I got so used to the git workflow, let me fix it now..
On 2013/03/26 16:57:21, pasko wrote: > oh shoot, please don't look at this yet, my svn/gcl client is busted, I got so > used to the git workflow, let me fix it now.. OK, re-creating an SVN client is so much pain, I uploaded my version from git, it does not have the nice 'svn cp' look yet. I'll make another attempt with SVN this evening. Please take a speculative look sooner :)
https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/cache_creat... File net/disk_cache/cache_creator.cc (right): https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/cache_creat... net/disk_cache/cache_creator.cc:97: CacheCreator::CacheCreator( On 2013/03/26 16:54:57, pasko wrote: > On 2013/03/25 22:06:04, rvargas wrote: > > Missing the class declaration > > I've put the declaration to the net/disk_cache/cache_util.h, but appears it was > not intuitive, let me know and I'll revive the cache_creator.h But this class can be private again, right?. If we restrict the creator to be an implementation detail of the public interface (CreateCacheBackend), and all tests to use either the public interface or (new xx) + SetFlags + Init then there is no need to expose this. Which leads me back to the flags. There are two concepts that are probably global: kUnitTestMode (which means "if there's something that you should do only for tests, do it now", but we really want to run most tests without the flag so that we test the "real" code paths), and kNoRandom (which means don't use experimental code or other stuff that depends on a random number generator, and all unit tests should set this). By the way, from the point of view of disk_cache_test_base, it doesn't really matter if the flags are really the same numerical value or if each implementation has its own set of values because it has to apply that to a specific member... so there is an explicit translation from a bool to whatever makes sense for a backend. What I'm doing so far is redefining BackendFlags as a private enum from my new implementation class, with the same values, but that is not even a medium term solution... I expect at some point to extract a new "implementation" interface that both backends use, probably with a new subset of common flags, but we probably should wait before we go there. In any case, having an implementation defined Init method that receives a uint32 seems good enough even if we have to redefine the same constants for now. https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/disk_cache_... File net/disk_cache/disk_cache_test_base.cc (right): https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/disk_cache_... net/disk_cache/disk_cache_test_base.cc:283: EXPECT_TRUE(CleanupCacheDir()); On 2013/03/26 16:54:57, pasko wrote: > On 2013/03/25 22:06:04, rvargas wrote: > > Why change this to an EXPECT? Seems better to skip the rest of the test at > this > > point. > > I recently discovered that ASSERT_TRUE does only return from the current > function, and reports an error, but it does not wind down the whole test. Also > it fails to compile if invoked from non-void helper functions because it does a > 'return something()' where 'something()' returns void. > > I can leave ASSERT_TRUE here if it looks better, since this function returns > void, it would work, though it would be roughly equivalent to EXPECT_TRUE. > > So for consistency I prefer EXPECT_* in helper functions if we want to check > rare/unlikely failures in helper functions intended only for testing. Cleaner > way would be to return success/failure from the helper functions, but it is a > little too heavy for this CL already. Yes, asserting inside this method doesn't prevent the rest of the test from running... and this method returns void to be able to assert. I believe that logically we want an assert, even if today it doesn't work that well. We can leave the assert and fix the rest later, or leave the assert and look for the error at return, or convert the method to return a failure and assert on return (slightly worse because we miss the exact line that failed). https://codereview.chromium.org/12794003/diff/62002/net/disk_cache/backend_un... File net/disk_cache/backend_unittest.cc (left): https://codereview.chromium.org/12794003/diff/62002/net/disk_cache/backend_un... net/disk_cache/backend_unittest.cc:303: flags |= disk_cache::kNoRandom; This code was just using the simplest (at the time) way to create a backend... call the static factory method. Now that we have a new utility method for that (on the test class), we should just pass kNoRandom from there, because all tests should use that flag to avoid experiment's variations. https://codereview.chromium.org/12794003/diff/62002/net/disk_cache/backend_un... File net/disk_cache/backend_unittest.cc (right): https://codereview.chromium.org/12794003/diff/62002/net/disk_cache/backend_un... net/disk_cache/backend_unittest.cc:373: uint32 flags = disk_cache::kNoBuffering; This one requires more surgery. The |fast| codepath probably should be replaced with !kUnitTestMode because that's really what is testing. kNoBuffering is usually set directly by the test code, but it requires an impl_ object, not just a generic cache_... so probably this method should go through the regular creation path for tests, not CreateBackendAsModeSuggests... it is using the factory directly because we want destruction to be scoped inside the test body (but the new code has to deal with that today anyway). Up to you... you can fix this now, or punt it (leave flags as an arg) and I'll fix it later.
PTAL please Now it is two files that has code moved to: cache_creator.cc, cache_util.cc. Re-creating SVN client helped, not it is visible very well. I have high hopes that this CL is near the final stage and there would be only minor comments (besides our forward-going thoughts). P.S.: some trybots are failing and some not, the errors are not found symbols at link-time, should I do a clobber build? is that common for you? https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/cache_creat... File net/disk_cache/cache_creator.cc (right): https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/cache_creat... net/disk_cache/cache_creator.cc:97: CacheCreator::CacheCreator( On 2013/03/26 18:30:00, rvargas wrote: > On 2013/03/26 16:54:57, pasko wrote: > > On 2013/03/25 22:06:04, rvargas wrote: > > > Missing the class declaration > > > > I've put the declaration to the net/disk_cache/cache_util.h, but appears it > was > > not intuitive, let me know and I'll revive the cache_creator.h > > But this class can be private again, right?. If we restrict the creator to be an > implementation detail of the public interface (CreateCacheBackend), and all > tests to use either the public interface or (new xx) + SetFlags + Init then > there is no need to expose this. The CacheCreator is private right now. It is used only in tests for testing the CacheCreator internals (i.e. |force|). For this purpose the CacheCreator declaration is not living anonymously in a cache_creator.cc. For other purposes we do not need CacheCreator and we do not use it. The question is where to put class CacheCreator declaration. I followed your advice and put it in cache_util.h, I can also put it into cache_creator.h, though I slightly tend to prefer less header files. Also, cache_util sounds kind of private enough. > Which leads me back to the flags. There are two concepts that are probably > global: kUnitTestMode (which means "if there's something that you should do only > for tests, do it now", but we really want to run most tests without the flag so > that we test the "real" code paths), and kNoRandom (which means don't use > experimental code or other stuff that depends on a random number generator, and > all unit tests should set this). On these two global flags the responsibility should be to selectively enable/disable them if they want to differ from the default. Which I believe would be rare. Global mechanism SetUnitTestMode() does the job quite nicely without flags. > By the way, from the point of view of disk_cache_test_base, it doesn't really > matter if the flags are really the same numerical value or if each > implementation has its own set of values because it has to apply that to a > specific member... so there is an explicit translation from a bool to whatever > makes sense for a backend. If we can decide about flags in disk_cache_test_base, that's easy. What if we want to set flags in the test itself? I do not have any specific test in mind, but it could possibly be that a test would rely on some backend properties that are set with one flag on backend1 and two flags on backend2. We would probably need some flag translation layer from what tests think about flags to what backend can take. Whether it be a uint32 bitset or a struct passed down to a specific backend is a minor detail. > What I'm doing so far is redefining BackendFlags as a private enum from my new > implementation class, with the same values, but that is not even a medium term > solution... I expect at some point to extract a new "implementation" interface > that both backends use, probably with a new subset of common flags, but we > probably should wait before we go there. Good to know. Likely something simple as ConvertTestFlagsToBackendFlags(uint32) invoked in disk_cache_test_base would be enough in the cases you think about. > In any case, having an implementation defined Init method that receives a uint32 > seems good enough even if we have to redefine the same constants for now. Yes. https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/disk_cache_... File net/disk_cache/disk_cache_test_base.cc (right): https://codereview.chromium.org/12794003/diff/7011/net/disk_cache/disk_cache_... net/disk_cache/disk_cache_test_base.cc:283: EXPECT_TRUE(CleanupCacheDir()); On 2013/03/26 18:30:00, rvargas wrote: > On 2013/03/26 16:54:57, pasko wrote: > > On 2013/03/25 22:06:04, rvargas wrote: > > > Why change this to an EXPECT? Seems better to skip the rest of the test at > > this > > > point. > > > > I recently discovered that ASSERT_TRUE does only return from the current > > function, and reports an error, but it does not wind down the whole test. Also > > it fails to compile if invoked from non-void helper functions because it does > a > > 'return something()' where 'something()' returns void. > > > > I can leave ASSERT_TRUE here if it looks better, since this function returns > > void, it would work, though it would be roughly equivalent to EXPECT_TRUE. > > > > So for consistency I prefer EXPECT_* in helper functions if we want to check > > rare/unlikely failures in helper functions intended only for testing. Cleaner > > way would be to return success/failure from the helper functions, but it is a > > little too heavy for this CL already. > > Yes, asserting inside this method doesn't prevent the rest of the test from > running... and this method returns void to be able to assert. I believe that > logically we want an assert, even if today it doesn't work that well. We can > leave the assert and fix the rest later, or leave the assert and look for the > error at return, or convert the method to return a failure and assert on return > (slightly worse because we miss the exact line that failed). I won't be stubborn, reverting back to using ASSERT, even making some previous EXPECTs to be ASSERTs for consistency. Though I think ASSERT will not in any close future prevent the whole test from running. It's just complicated. EXPECT is a good way to be more clear about what the code is doing. But since not many people look at this specific portion of code, quite OK to have it the way it is now. Done. https://codereview.chromium.org/12794003/diff/62002/net/disk_cache/backend_un... File net/disk_cache/backend_unittest.cc (left): https://codereview.chromium.org/12794003/diff/62002/net/disk_cache/backend_un... net/disk_cache/backend_unittest.cc:303: flags |= disk_cache::kNoRandom; On 2013/03/26 18:30:00, rvargas wrote: > This code was just using the simplest (at the time) way to create a backend... > call the static factory method. Now that we have a new utility method for that > (on the test class), we should just pass kNoRandom from there, because all tests > should use that flag to avoid experiment's variations. Good to know! Thanks! I tried running all tests with kNoRandom, only one fails: ShutdownWithPendingFileIO_Fast, I am tempted to clear the bit just there in test specifics and force kNoRandom everywhere in DiskCacheTestWithCache. Does it sound right to you? https://codereview.chromium.org/12794003/diff/62002/net/disk_cache/backend_un... File net/disk_cache/backend_unittest.cc (right): https://codereview.chromium.org/12794003/diff/62002/net/disk_cache/backend_un... net/disk_cache/backend_unittest.cc:373: uint32 flags = disk_cache::kNoBuffering; On 2013/03/26 18:30:00, rvargas wrote: > This one requires more surgery. The |fast| codepath probably should be replaced > with !kUnitTestMode because that's really what is testing. Doing a (!kUnitTestMode) codepath in a unit test sounds quite fun :) > kNoBuffering is usually set directly by the test code, but it requires an impl_ > object, not just a generic cache_... so probably this method should go through > the regular creation path for tests, not CreateBackendAsModeSuggests... it is > using the factory directly because we want destruction to be scoped inside the > test body (but the new code has to deal with that today anyway). > > Up to you... you can fix this now, or punt it (leave flags as an arg) and I'll > fix it later. Going through the usual initialization codepath instead of BackendShutdownWithPendingIO() looks good to me. Really good. Though I am a little afraid to touch it here, since I might bloat the CL unreasonably, and will probably spend much time understanding what the test intentions are, why we need the factory method, etc. So I would appreciate if we could postpone that.
nit on the description: "Decide to initialize the simple cache" -> "Initialize the " (I would normally just go ahead and change it myself, except that I have a another nit). nit 2: If you really care that much about people adding a long command line and forgetting to add =on at the end, a better option is to change the command line to --simple-cache-backend so that it doesn't convey the idea of enabling an option. Which, by the way, is still a few layers above this code so adding that comment is not exactly kosher. So we're down to mostly style nits. https://codereview.chromium.org/12794003/diff/62002/net/disk_cache/backend_un... File net/disk_cache/backend_unittest.cc (right): https://codereview.chromium.org/12794003/diff/62002/net/disk_cache/backend_un... net/disk_cache/backend_unittest.cc:373: uint32 flags = disk_cache::kNoBuffering; On 2013/03/27 20:36:08, pasko wrote: > On 2013/03/26 18:30:00, rvargas wrote: > > This one requires more surgery. The |fast| codepath probably should be > replaced > > with !kUnitTestMode because that's really what is testing. > > Doing a (!kUnitTestMode) codepath in a unit test sounds quite fun :) What I meant is that the production code being tested should use kUnitTestMode instead of what it uses today (kNoRandom) (~BackendImpl()) > > > kNoBuffering is usually set directly by the test code, but it requires an > impl_ > > object, not just a generic cache_... so probably this method should go through > > the regular creation path for tests, not CreateBackendAsModeSuggests... it is > > using the factory directly because we want destruction to be scoped inside the > > test body (but the new code has to deal with that today anyway). > > > > Up to you... you can fix this now, or punt it (leave flags as an arg) and I'll > > fix it later. > > Going through the usual initialization codepath instead of > BackendShutdownWithPendingIO() looks good to me. Really good. Though I am a > little afraid to touch it here, since I might bloat the CL unreasonably, and > will probably spend much time understanding what the test intentions are, why we > need the factory method, etc. > > So I would appreciate if we could postpone that. Yes, I'm fine postponing that. https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/backend_un... File net/disk_cache/backend_unittest.cc (right): https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/backend_un... net/disk_cache/backend_unittest.cc:369: base::Thread::Options(MessageLoop::TYPE_IO, 0))); nit: indent 4 more spaces (this is not the second argument of aasert, it is the first arg of Start) https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_crea... File net/disk_cache/cache_creator.cc (right): https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_crea... net/disk_cache/cache_creator.cc:43: // TODO(pasko): The two caches should never coexist. Set up cache cleanup in nit: I don't understand the todo. Do you mean that we should attempt to create a backend only to see if we can, and then delete the files? Or do manual inspection to see if the files look like belonging to a cache, and then delete them? I don't think we should do either of thing. Seems better if each backend just fails if something else is in place (and all that's needed is having the index named "index") https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_crea... net/disk_cache/cache_creator.cc:52: return disk_cache::SimpleBackendImpl::CreateBackend(path_, max_bytes_, nit: indentation (as in now the first two args have to go down) https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_crea... net/disk_cache/cache_creator.cc:57: disk_cache::BackendImpl* new_cache = new disk_cache::BackendImpl( nit: indentation (as in break the line at the = ) https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_crea... net/disk_cache/cache_creator.cc:103: int max_bytes, bool force, base::MessageLoopProxy* thread, indentation https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_crea... net/disk_cache/cache_creator.cc:108: CacheCreator* creator = new CacheCreator(path, force, max_bytes, type, kNone, nit: I should have mentioned before that it is overkill to instantiate a cache creator* for a memory cache, with very little (no?) gain. (*) given that the purpose of the CacheCreator is to deal with threads and cleanup. That said, if you really prefer it this way that's fine Note: I have not seen the test that you mention about forcing cleanup, but my reaction so far is to test the interface and not CacheCreator (so make CacheCreator private in the long run). https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_util.cc File net/disk_cache/cache_util.cc (right): https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_util... net/disk_cache/cache_util.cc:55: // directory), and spawn a worker thread to delete all the files on all the nit: spawn -> use https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_util.h File net/disk_cache/cache_util.h (right): https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_util... net/disk_cache/cache_util.h:37: NET_EXPORT_PRIVATE bool DelayedCacheCleanup(const base::FilePath& full_path); why export this?. I don't think we should (unless you are adding a unit test for it). https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_util... net/disk_cache/cache_util.h:39: // Builds an instance of the backend depending on command-line option(s), Don't mention command line options. The _implementation_ uses an experiment (not a command line) but experiments are not specified on headers. The important part about this class is that it performs retries after cleaning up the target directory. https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_util... net/disk_cache/cache_util.h:42: class NET_EXPORT_PRIVATE CacheCreator { As I said somewhere else, I think we should be testing the real interface. However, if this is here just to avoid making the CL more complex that's fine. https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/disk_cache... File net/disk_cache/disk_cache_test_base.cc (right): https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/disk_cache... net/disk_cache/disk_cache_test_base.cc:244: ASSERT_TRUE(mem_cache_->SetMaxSize(size_)); The idea was that failing to set the size is normnally not critical... yes, the test will probably fail, but nothing will crash. On the other hand, moving on without a cache_, or after Init() fails most likely will crash net_unittests (yes, it is still broken 'cause we indeed move on, and we indeed crash) https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/disk_cache... net/disk_cache/disk_cache_test_base.cc:274: disk_cache::CacheCreator* creator = new disk_cache::CacheCreator(cache_path_, nit: argument must go down https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/disk_cache... net/disk_cache/disk_cache_test_base.cc:274: disk_cache::CacheCreator* creator = new disk_cache::CacheCreator(cache_path_, Use CreateCacheBackend instead. https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/disk_cache... net/disk_cache/disk_cache_test_base.cc:282: uint32 flags, base::Thread* thread) { first arg on the previous line, second one indented under the first one. https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/disk_cache... net/disk_cache/disk_cache_test_base.cc:308: ASSERT_TRUE(cache_impl_->SetMaxSize(size_)); same thing about SetMaxSize (non fatal) https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/disk_cache... net/disk_cache/disk_cache_test_base.cc:318: nit: is this an extra line? https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/disk_cache... File net/disk_cache/disk_cache_test_base.h (right): https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/disk_cache... net/disk_cache/disk_cache_test_base.h:62: void CreateBackendAsModeSuggests(uint32 flags, base::Thread* thread); nit: CreateBackend() ? (does it make sense to CreateBackendIgnoringTheMode ?) https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/disk_cache... net/disk_cache/disk_cache_test_base.h:65: void InitDefaultCacheViaCreator(); nit: InitCacheViaCreator ? InitDefaultCache ? (probably #2 given that it should not really use the creator :) ) https://codereview.chromium.org/12794003/diff/67001/net/tools/crash_cache/cra... File net/tools/crash_cache/crash_cache.cc (right): https://codereview.chromium.org/12794003/diff/67001/net/tools/crash_cache/cra... net/tools/crash_cache/crash_cache.cc:141: disk_cache::BackendImpl* backend = new disk_cache::BackendImpl(path, nit: first arg goes down
I am going to address the low-level comments now. Publishing the high-level thoughts now without any changes to the code. On 2013/03/28 03:12:45, rvargas wrote: > nit on the description: "Decide to initialize the simple cache" -> "Initialize > the " (I would normally just go ahead and change it myself, except that I have a > another nit). Agreed. Will do. > nit 2: If you really care that much about people adding a long command line and > forgetting to add =on at the end, a better option is to change the command line > to --simple-cache-backend so that it doesn't convey the idea of enabling an > option. Which, by the way, is still a few layers above this code so adding that > comment is not exactly kosher. Agreed that the option is a few layers above. With the 3 modes (on|off|experiment) I did not like the idea of making "on" a synonym of "". Probably should have done it this way. I'll remove the comment. https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_crea... File net/disk_cache/cache_creator.cc (right): https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_crea... net/disk_cache/cache_creator.cc:43: // TODO(pasko): The two caches should never coexist. Set up cache cleanup in On 2013/03/28 03:12:45, rvargas wrote: > nit: I don't understand the todo. Do you mean that we should attempt to create a > backend only to see if we can, and then delete the files? Or do manual > inspection to see if the files look like belonging to a cache, and then delete > them? I don't think we should do either of thing. > > Seems better if each backend just fails if something else is in place (and all > that's needed is having the index named "index") Yes, a backend should not start successfully if the cache files are from the other backend. Breaking as little encapsulation as possible would be best achieved as you say, each backend would compare its magic with the one in file "index". Although I did not fully verify it works with BackendImpl. I should correct the TODO to be more clear.. https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_crea... net/disk_cache/cache_creator.cc:108: CacheCreator* creator = new CacheCreator(path, force, max_bytes, type, kNone, On 2013/03/28 03:12:45, rvargas wrote: > nit: I should have mentioned before that it is overkill to instantiate a cache > creator* for a memory cache, with very little (no?) gain. > > (*) given that the purpose of the CacheCreator is to deal with threads and > cleanup. Oh yes, I forgot to reply on your comment regarding this. My intention was to keep backend choice local in a single function given all the parameters. That is mainly for readability: all aspects of the choice are local. But I agree that doing unnecessary instantiation work may confuse the reader even more. I am now going to move it back to CreateCacheBackend(). > That said, if you really prefer it this way that's fine > > Note: I have not seen the test that you mention about forcing cleanup, but my > reaction so far is to test the interface and not CacheCreator (so make > CacheCreator private in the long run). There one test for forcing cleanup: DiskCacheTest.DeleteOld I split it into two: one that tests BackendImpl initialization failure, and another that tests the force cleanup itself. There is also: DiskCacheBackendTest.CreateBackend_MissingFile The latter seems to also test that the cache creator does not perform disk access on the IO thread. I did not look closely at that before. It would be logical in this situation to separate it out into 2 tests: 1. removing of "data_1", init with BackendImpl, with the SetIOAllowed(false), get error 2. test CacheCreator force cleanup with SetIOAllowed(false) In all cases of force cleanup testing I lean towards using InitDefaultCacheViaCreator() that is a simple wrapper around CacheCreator providing kNoRandom. I thought using CreateCacheBAckend and loosing kNoRandom is not completely kosher. Though the benefit would be making CacheCreator completely private. I can do it in this CL if you like the idea.
btw, I would not expect this change to require clobbering of bots because all uses of the new exported functions should trigger recompilation. https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_crea... File net/disk_cache/cache_creator.cc (right): https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_crea... net/disk_cache/cache_creator.cc:108: CacheCreator* creator = new CacheCreator(path, force, max_bytes, type, kNone, > In all cases of force cleanup testing I lean towards using > InitDefaultCacheViaCreator() that is a simple wrapper around CacheCreator > providing kNoRandom. I thought using CreateCacheBAckend and loosing kNoRandom is > not completely kosher. Though the benefit would be making CacheCreator > completely private. I can do it in this CL if you like the idea. I still think it is better to write unit tests against the code interface as opposed to the implementation details (or classes). At this point it probably makes more sense to leave this CL as is and land it. We can do cleanup later (we need to change the current code in order to fix the use of flags on a few tests).
On 2013/03/28 18:47:30, rvargas wrote: > I still think it is better to write unit tests against the code interface as > opposed to the implementation details (or classes). I think if the interfaces are very close to each other and the equivalence is visually verifiable easily, then I am OK testing the test- side of the interface. Moving memory cache creation to the cache creator was part of making the two interfaces very close to equivalent. I like being able to mock out parts of the interface, hence more love to classes than global functions. As of current state the force create logic is not possible to test independently from the backend, part of that is global nature of the CreateCacheBackend, another part is CacheCreator/BackendImpl coupling. > At this point it probably makes more sense to leave this CL as is and land it. > We can do cleanup later (we need to change the current code in order to fix the > use of flags on a few tests). My formulation of work to do is move all backend creation to the test fixture while flags remain set to correct values. Something may pop out on the way to this though.
https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/backend_un... File net/disk_cache/backend_unittest.cc (right): https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/backend_un... net/disk_cache/backend_unittest.cc:369: base::Thread::Options(MessageLoop::TYPE_IO, 0))); On 2013/03/28 03:12:45, rvargas wrote: > nit: indent 4 more spaces (this is not the second argument of aasert, it is the > first arg of Start) ouch, not sure how it appeared this way. Done. .. although I always thought it should be 4 spaces to the right from where the line begins if we have a call inside a call. https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_crea... File net/disk_cache/cache_creator.cc (right): https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_crea... net/disk_cache/cache_creator.cc:52: return disk_cache::SimpleBackendImpl::CreateBackend(path_, max_bytes_, On 2013/03/28 03:12:45, rvargas wrote: > nit: indentation (as in now the first two args have to go down) Done. https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_crea... net/disk_cache/cache_creator.cc:57: disk_cache::BackendImpl* new_cache = new disk_cache::BackendImpl( On 2013/03/28 03:12:45, rvargas wrote: > nit: indentation (as in break the line at the = ) Done. https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_crea... net/disk_cache/cache_creator.cc:103: int max_bytes, bool force, base::MessageLoopProxy* thread, On 2013/03/28 03:12:45, rvargas wrote: > indentation Done. https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_crea... net/disk_cache/cache_creator.cc:108: CacheCreator* creator = new CacheCreator(path, force, max_bytes, type, kNone, On 2013/03/28 18:47:30, rvargas wrote: > > In all cases of force cleanup testing I lean towards using > > InitDefaultCacheViaCreator() that is a simple wrapper around CacheCreator > > providing kNoRandom. I thought using CreateCacheBAckend and loosing kNoRandom > is > > not completely kosher. Though the benefit would be making CacheCreator > > completely private. I can do it in this CL if you like the idea. > > I still think it is better to write unit tests against the code interface as > opposed to the implementation details (or classes). > > At this point it probably makes more sense to leave this CL as is and land it. > We can do cleanup later (we need to change the current code in order to fix the > use of flags on a few tests). Done. Moved to the memory cache creation here, although then I had the afterthought that it increases the difference between the CacheCreator and CreateCacheBackend() interfaces. https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_util.cc File net/disk_cache/cache_util.cc (right): https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_util... net/disk_cache/cache_util.cc:55: // directory), and spawn a worker thread to delete all the files on all the On 2013/03/28 03:12:45, rvargas wrote: > nit: spawn -> use Done. https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_util.h File net/disk_cache/cache_util.h (right): https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_util... net/disk_cache/cache_util.h:37: NET_EXPORT_PRIVATE bool DelayedCacheCleanup(const base::FilePath& full_path); On 2013/03/28 03:12:45, rvargas wrote: > why export this?. I don't think we should (unless you are adding a unit test for > it). I am confused. NET_EXPORT and NET_EXPORT_PRIVATE have the same definition in net_export.h, on any platform/configuration. Is it supposed to change? I removed the NET_EXPORT_PRIVATE here. https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_util... net/disk_cache/cache_util.h:39: // Builds an instance of the backend depending on command-line option(s), On 2013/03/28 03:12:45, rvargas wrote: > Don't mention command line options. The _implementation_ uses an experiment (not > a command line) but experiments are not specified on headers. The important part > about this class is that it performs retries after cleaning up the target > directory. Replaced commandline with experiment in the description. I think it is important to note that this is almost the single point of backend choice. https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_util... net/disk_cache/cache_util.h:42: class NET_EXPORT_PRIVATE CacheCreator { On 2013/03/28 03:12:45, rvargas wrote: > As I said somewhere else, I think we should be testing the real interface. > However, if this is here just to avoid making the CL more complex that's fine. I will try to ignore all the flags on the few force retry tests that we have, if ignoring flags would be fine there, I'll just privatize the cache creator. Though I am not knowing enough about BackendImpl, whether it can make the test flaky. Probably should not. https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/disk_cache... File net/disk_cache/disk_cache_test_base.cc (right): https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/disk_cache... net/disk_cache/disk_cache_test_base.cc:244: ASSERT_TRUE(mem_cache_->SetMaxSize(size_)); On 2013/03/28 03:12:45, rvargas wrote: > The idea was that failing to set the size is normnally not critical... yes, the > test will probably fail, but nothing will crash. > > On the other hand, moving on without a cache_, or after Init() fails most likely > will crash net_unittests (yes, it is still broken 'cause we indeed move on, and > we indeed crash) OK, I was assuming all standard fixture initialization should always succeed, and if it does not succeed the problem is not in the test but rather in the testing framework. Reverted to the original since it is your choice about the importance of this very specific failure. https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/disk_cache... net/disk_cache/disk_cache_test_base.cc:274: disk_cache::CacheCreator* creator = new disk_cache::CacheCreator(cache_path_, On 2013/03/28 03:12:45, rvargas wrote: > nit: argument must go down Done. https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/disk_cache... net/disk_cache/disk_cache_test_base.cc:274: disk_cache::CacheCreator* creator = new disk_cache::CacheCreator(cache_path_, On 2013/03/28 03:12:45, rvargas wrote: > Use CreateCacheBackend instead. OK, I'll try to do it in the next smaller CL, hopefully it won't make the tests flaky. https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/disk_cache... net/disk_cache/disk_cache_test_base.cc:282: uint32 flags, base::Thread* thread) { On 2013/03/28 03:12:45, rvargas wrote: > first arg on the previous line, second one indented under the first one. How do you know it would fit the line? :) https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/disk_cache... net/disk_cache/disk_cache_test_base.cc:308: ASSERT_TRUE(cache_impl_->SetMaxSize(size_)); On 2013/03/28 03:12:45, rvargas wrote: > same thing about SetMaxSize (non fatal) Done. https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/disk_cache... net/disk_cache/disk_cache_test_base.cc:318: On 2013/03/28 03:12:45, rvargas wrote: > nit: is this an extra line? Done. https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/disk_cache... File net/disk_cache/disk_cache_test_base.h (right): https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/disk_cache... net/disk_cache/disk_cache_test_base.h:62: void CreateBackendAsModeSuggests(uint32 flags, base::Thread* thread); On 2013/03/28 03:12:45, rvargas wrote: > nit: CreateBackend() ? (does it make sense to CreateBackendIgnoringTheMode ?) I made something that is easier to grep through sources. https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/disk_cache... net/disk_cache/disk_cache_test_base.h:65: void InitDefaultCacheViaCreator(); On 2013/03/28 03:12:45, rvargas wrote: > nit: InitCacheViaCreator ? InitDefaultCache ? > > (probably #2 given that it should not really use the creator :) ) The whole point of this function is to test via Creator. I'll remove it if we can do the same with CreateCacheBackend(). The word Default suggests that we cannot control which backend to create, it chooses the platform's default. https://codereview.chromium.org/12794003/diff/67001/net/tools/crash_cache/cra... File net/tools/crash_cache/crash_cache.cc (right): https://codereview.chromium.org/12794003/diff/67001/net/tools/crash_cache/cra... net/tools/crash_cache/crash_cache.cc:141: disk_cache::BackendImpl* backend = new disk_cache::BackendImpl(path, On 2013/03/28 03:12:45, rvargas wrote: > nit: first arg goes down sry, that style probably comes from Java, I do not remember where I got it from
On 2013/03/28 19:23:49, pasko wrote: > On 2013/03/28 18:47:30, rvargas wrote: > > I still think it is better to write unit tests against the code interface as > > opposed to the implementation details (or classes). > > I think if the interfaces are very close to each other and the equivalence is > visually verifiable easily, then I am OK testing the test- side of the > interface. Moving memory cache creation to the cache creator was part of making > the two interfaces very close to equivalent. > > I like being able to mock out parts of the interface, hence more love to classes > than global functions. As of current state the force create logic is not > possible to test independently from the backend, part of that is global nature > of the CreateCacheBackend, another part is CacheCreator/BackendImpl coupling. > > > At this point it probably makes more sense to leave this CL as is and land it. > > We can do cleanup later (we need to change the current code in order to fix > the > > use of flags on a few tests). > > My formulation of work to do is move all backend creation to the test fixture > while flags remain set to correct values. Something may pop out on the way to > this though. My understanding was wrong, since this comment referred to the case of force retry, where we probably do not need flags.
LGTM https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_crea... File net/disk_cache/cache_creator.cc (right): https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_crea... net/disk_cache/cache_creator.cc:108: CacheCreator* creator = new CacheCreator(path, force, max_bytes, type, kNone, On 2013/03/28 21:41:42, pasko wrote: > On 2013/03/28 18:47:30, rvargas wrote: > > > In all cases of force cleanup testing I lean towards using > > > InitDefaultCacheViaCreator() that is a simple wrapper around CacheCreator > > > providing kNoRandom. I thought using CreateCacheBAckend and loosing > kNoRandom > > is > > > not completely kosher. Though the benefit would be making CacheCreator > > > completely private. I can do it in this CL if you like the idea. > > > > I still think it is better to write unit tests against the code interface as > > opposed to the implementation details (or classes). > > > > At this point it probably makes more sense to leave this CL as is and land it. > > We can do cleanup later (we need to change the current code in order to fix > the > > use of flags on a few tests). > > Done. > > Moved to the memory cache creation here, although then I had the afterthought > that it increases the difference between the CacheCreator and > CreateCacheBackend() interfaces. I still don't consider CacheCreator an interface :). I hear your point about classes and mocks and testability, but layers of code that don't do anything other than forward work somewhere else is something that I don't like... there has to be a strong reason for that pattern. https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_util.h File net/disk_cache/cache_util.h (right): https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_util... net/disk_cache/cache_util.h:37: NET_EXPORT_PRIVATE bool DelayedCacheCleanup(const base::FilePath& full_path); On 2013/03/28 21:41:42, pasko wrote: > On 2013/03/28 03:12:45, rvargas wrote: > > why export this?. I don't think we should (unless you are adding a unit test > for > > it). > > I am confused. NET_EXPORT and NET_EXPORT_PRIVATE have the same definition in > net_export.h, on any platform/configuration. Is it supposed to change? > > I removed the NET_EXPORT_PRIVATE here. They are always the same thing... the idea is that hopefully people will notice the "private" part and avoid it outside of net. https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_util... net/disk_cache/cache_util.h:39: // Builds an instance of the backend depending on command-line option(s), On 2013/03/28 21:41:42, pasko wrote: > On 2013/03/28 03:12:45, rvargas wrote: > > Don't mention command line options. The _implementation_ uses an experiment > (not > > a command line) but experiments are not specified on headers. The important > part > > about this class is that it performs retries after cleaning up the target > > directory. > > Replaced commandline with experiment in the description. I think it is important > to note that this is almost the single point of backend choice. but headers are meant to document interfaces and an experiment is (or should not be) part of an interface. https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/disk_cache... File net/disk_cache/disk_cache_test_base.cc (right): https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/disk_cache... net/disk_cache/disk_cache_test_base.cc:282: uint32 flags, base::Thread* thread) { On 2013/03/28 21:41:42, pasko wrote: > On 2013/03/28 03:12:45, rvargas wrote: > > first arg on the previous line, second one indented under the first one. > > How do you know it would fit the line? :) Sometimes I try things before adding comments. https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/disk_cache... File net/disk_cache/disk_cache_test_base.h (right): https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/disk_cache... net/disk_cache/disk_cache_test_base.h:62: void CreateBackendAsModeSuggests(uint32 flags, base::Thread* thread); On 2013/03/28 21:41:42, pasko wrote: > On 2013/03/28 03:12:45, rvargas wrote: > > nit: CreateBackend() ? (does it make sense to CreateBackendIgnoringTheMode ?) > > I made something that is easier to grep through sources. hmmm... I really don't consider the uniqueness of names for search to be a quality of the code. The second part of the name doesn't make any sense to me: yes, it is something that the code does, but it doesn't provide any information at all because it is something that the code should be doing. It adds as much value as CreateBackendAndTryHardNotToFail, and that would also be unique for a search, but it hurts readability of the code because it is noise that makes me wonder if there's something funky going on with the code. I think it is more important for the code to read naturally, without extraneous decorations. https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/disk_cache... net/disk_cache/disk_cache_test_base.h:65: void InitDefaultCacheViaCreator(); On 2013/03/28 21:41:42, pasko wrote: > On 2013/03/28 03:12:45, rvargas wrote: > > nit: InitCacheViaCreator ? InitDefaultCache ? > > > > (probably #2 given that it should not really use the creator :) ) > > The whole point of this function is to test via Creator. I'll remove it if we I don't agree there. The point is to use the regular interface that clients use. > can do the same with CreateCacheBackend(). The word Default suggests that we > cannot control which backend to create, it chooses the platform's default. And we cannot control it beyond what a regular caller should be able to control. We agree that the word default is right, no? https://codereview.chromium.org/12794003/diff/81001/net/tools/crash_cache/cra... File net/tools/crash_cache/crash_cache.cc (right): https://codereview.chromium.org/12794003/diff/81001/net/tools/crash_cache/cra... net/tools/crash_cache/crash_cache.cc:142: path, thread->message_loop_proxy(), NULL); nit: I'm really sorry for not seeing this earlier, but could you break the line at the = instead.
trying to get more input earlier, posting some replies without changes changes to the code.. https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_crea... File net/disk_cache/cache_creator.cc (right): https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_crea... net/disk_cache/cache_creator.cc:108: CacheCreator* creator = new CacheCreator(path, force, max_bytes, type, kNone, On 2013/03/28 22:30:21, rvargas wrote: > > Moved to the memory cache creation here, although then I had the afterthought > > that it increases the difference between the CacheCreator and > > CreateCacheBackend() interfaces. > > I still don't consider CacheCreator an interface :). I hear your point about > classes and mocks and testability, but layers of code that don't do anything > other than forward work somewhere else is something that I don't like... there > has to be a strong reason for that pattern. We should be different somewhere, after all :) The reason is often in: 1. making tests run faster by mocking out expensive operations 2. removing platform difference in tests Which are often more ugly than a few more layers. I agree there needs to be balance. On the CreateCacheBackend() land, I am worried about having to handle platform differences with #ifdefs in tests if the default becomes simple-cache-backend=on. That seems to be uglier than making CacheCreator layered out of specific backend factory. Don't get me wrong, I am not a proponent of endless patterns, like those common in Java. Factory factory factory is my nightmare. I am just having some specific testing scenarios in mind. Actually, dropping force retry tests is not a big deal, saves so much time, decreases coverage by what .. 10 lines of code? https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_util.h File net/disk_cache/cache_util.h (right): https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_util... net/disk_cache/cache_util.h:37: NET_EXPORT_PRIVATE bool DelayedCacheCleanup(const base::FilePath& full_path); On 2013/03/28 22:30:21, rvargas wrote: > On 2013/03/28 21:41:42, pasko wrote: > > On 2013/03/28 03:12:45, rvargas wrote: > > > why export this?. I don't think we should (unless you are adding a unit test > > for > > > it). > > > > I am confused. NET_EXPORT and NET_EXPORT_PRIVATE have the same definition in > > net_export.h, on any platform/configuration. Is it supposed to change? > > > > I removed the NET_EXPORT_PRIVATE here. > > They are always the same thing... the idea is that hopefully people will notice > the "private" part and avoid it outside of net. OK, you eliminated my confusion, and I might only have a suggestion to put an extra line at the top of net_export.h to say that NET_EXPORT_PRIVATE is purely advisory, there is no real usage prevention behind it. Although we could probably make this prevention in the component build, do we have bots running the component build regularly? https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_util... net/disk_cache/cache_util.h:39: // Builds an instance of the backend depending on command-line option(s), On 2013/03/28 22:30:21, rvargas wrote: > On 2013/03/28 21:41:42, pasko wrote: > > On 2013/03/28 03:12:45, rvargas wrote: > > > Don't mention command line options. The _implementation_ uses an experiment > > (not > > > a command line) but experiments are not specified on headers. The important > > part > > > about this class is that it performs retries after cleaning up the target > > > directory. > > > > Replaced commandline with experiment in the description. I think it is > important > > to note that this is almost the single point of backend choice. > > but headers are meant to document interfaces and an experiment is (or should not > be) part of an interface. Ah, I see. The comment was intended to suggest a class of sources where backend decision may come from, it was not for telling about the exact behavior, the "etc." part is taking care of being inexact here. Talking about experiments in the interface is indeed a little strange, I should remove it. https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/disk_cache... File net/disk_cache/disk_cache_test_base.h (right): https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/disk_cache... net/disk_cache/disk_cache_test_base.h:62: void CreateBackendAsModeSuggests(uint32 flags, base::Thread* thread); On 2013/03/28 22:30:21, rvargas wrote: > On 2013/03/28 21:41:42, pasko wrote: > > On 2013/03/28 03:12:45, rvargas wrote: > > > nit: CreateBackend() ? (does it make sense to CreateBackendIgnoringTheMode > ?) > > > > I made something that is easier to grep through sources. > > hmmm... I really don't consider the uniqueness of names for search to be a > quality of the code. The second part of the name doesn't make any sense to me: > yes, it is something that the code does, but it doesn't provide any information > at all because it is something that the code should be doing. It adds as much > value as CreateBackendAndTryHardNotToFail, and that would also be unique for a > search, but it hurts readability of the code because it is noise that makes me > wonder if there's something funky going on with the code. Ok, does CreateCacheBackendForTesting sound better? I still like the uniqueness, even if it adds clutter in the name, it gets much easier to navigate the code. I often read the code in non-linear ways in vim with exuberant-ctags (yeah, I know, sounds ancient), so the 10 variants of CreateBackend are bothering in similar ways as multiple wrapper layers do. Am I not cooking it right? > I think it is more important for the code to read naturally, without extraneous > decorations. I have this argument about code spanning across multiple files, where it is easier to read by jumping across files by exact named pointers rather than very common names. There are obviously very common names like Run() which I did not learn how to deal with. https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/disk_cache... net/disk_cache/disk_cache_test_base.h:65: void InitDefaultCacheViaCreator(); On 2013/03/28 22:30:21, rvargas wrote: > On 2013/03/28 21:41:42, pasko wrote: > > On 2013/03/28 03:12:45, rvargas wrote: > > > nit: InitCacheViaCreator ? InitDefaultCache ? > > > > > > (probably #2 given that it should not really use the creator :) ) > > > > The whole point of this function is to test via Creator. I'll remove it if we > > I don't agree there. The point is to use the regular interface that clients use. I agree that we should test the regular interface when possible. And very soon the CreateCacheBackend may become not possible to use on all platforms in the same way. Disabling tests for those platforms is probably your suggestion. That would certainly work. > > can do the same with CreateCacheBackend(). The word Default suggests that we > > cannot control which backend to create, it chooses the platform's default. > > And we cannot control it beyond what a regular caller should be able to control. > We agree that the word default is right, no? I did not understand this last sentence :) OK, in the next CL I am trying to use the official CreateCacheBackend(), to replace all calls to InitDefaultCacheViaCreator().
https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_crea... File net/disk_cache/cache_creator.cc (right): https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_crea... net/disk_cache/cache_creator.cc:108: CacheCreator* creator = new CacheCreator(path, force, max_bytes, type, kNone, On 2013/03/29 02:20:26, pasko wrote: > On 2013/03/28 22:30:21, rvargas wrote: > > > Moved to the memory cache creation here, although then I had the > afterthought > > > that it increases the difference between the CacheCreator and > > > CreateCacheBackend() interfaces. > > > > I still don't consider CacheCreator an interface :). I hear your point about > > classes and mocks and testability, but layers of code that don't do anything > > other than forward work somewhere else is something that I don't like... there > > has to be a strong reason for that pattern. > > We should be different somewhere, after all :) > > The reason is often in: > 1. making tests run faster by mocking out expensive operations > 2. removing platform difference in tests > > Which are often more ugly than a few more layers. I agree there needs to be > balance. > > On the CreateCacheBackend() land, I am worried about having to handle platform > differences with #ifdefs in tests if the default becomes > simple-cache-backend=on. That seems to be uglier than making CacheCreator > layered out of specific backend factory. > > Don't get me wrong, I am not a proponent of endless patterns, like those common > in Java. Factory factory factory is my nightmare. I am just having some specific > testing scenarios in mind. Actually, dropping force retry tests is not a big > deal, saves so much time, decreases coverage by what .. 10 lines of code? Yes, in general I would agree that being able to mock is a good thing, and encapsulation of platform differences is also a good thing. However, for this specific case I don't agree. I guess I was not clear enough before, but the main reason for most of the tests that were using directly the backend factory and not the class provided by the test base was simply because it was easier to write it that way (and easier to see what the test was doing), as opposed to creating an object and waiting on a test callback (not that it was particularly hard)... the point being that destruction was supposed to happen in the middle of the test, not during test shutdown (as usual with other tests). However, by changing the test base so that we use the same members to store the pointer of the cache, now we have to do something to avoid a double deletion (and you added that code to the tests)... so there is no reason to use a special way to create the object. It can be morphed into basically the same construction as all other tests. Except for one test that actually verifies the cleanup of files. But that should test the public interface (because it has to be tested somewhere anyway)... there is no need to test an internal class instead of the public interface. And if we only cover a single backend for that test that's fine, because what we are testing is the cleanup code (the only feature not covered by other tests), and that is (now) independent of the backend... so there is no platform specific code or ifdefs to take care of. In other words, it is nice to do all tests for every possible backend, but it's not really a requirement... some tests just make sense for a given backend, and other tests would be just killing cycles on the bots testing again the same thing. ... and that's why I don't see value on a thin wrapper in this case, or on worrying about mocking the behavior of this public static constructor by using a class whose purpose is very specific: handle the callback and the cleanup/retry. https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_util.h File net/disk_cache/cache_util.h (right): https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/cache_util... net/disk_cache/cache_util.h:37: NET_EXPORT_PRIVATE bool DelayedCacheCleanup(const base::FilePath& full_path); On 2013/03/29 02:20:26, pasko wrote: > On 2013/03/28 22:30:21, rvargas wrote: > > On 2013/03/28 21:41:42, pasko wrote: > > > On 2013/03/28 03:12:45, rvargas wrote: > > > > why export this?. I don't think we should (unless you are adding a unit > test > > > for > > > > it). > > > > > > I am confused. NET_EXPORT and NET_EXPORT_PRIVATE have the same definition in > > > net_export.h, on any platform/configuration. Is it supposed to change? > > > > > > I removed the NET_EXPORT_PRIVATE here. > > > > They are always the same thing... the idea is that hopefully people will > notice > > the "private" part and avoid it outside of net. > > OK, you eliminated my confusion, and I might only have a suggestion to put an > extra line at the top of net_export.h to say that NET_EXPORT_PRIVATE is purely > advisory, there is no real usage prevention behind it. > > Although we could probably make this prevention in the component build, do we > have bots running the component build regularly? We have more bots building the component build than bots building the static-lib build. https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/disk_cache... File net/disk_cache/disk_cache_test_base.h (right): https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/disk_cache... net/disk_cache/disk_cache_test_base.h:62: void CreateBackendAsModeSuggests(uint32 flags, base::Thread* thread); On 2013/03/29 02:20:26, pasko wrote: > On 2013/03/28 22:30:21, rvargas wrote: > > On 2013/03/28 21:41:42, pasko wrote: > > > On 2013/03/28 03:12:45, rvargas wrote: > > > > nit: CreateBackend() ? (does it make sense to CreateBackendIgnoringTheMode > > ?) > > > > > > I made something that is easier to grep through sources. > > > > hmmm... I really don't consider the uniqueness of names for search to be a > > quality of the code. The second part of the name doesn't make any sense to me: > > yes, it is something that the code does, but it doesn't provide any > information > > at all because it is something that the code should be doing. It adds as much > > value as CreateBackendAndTryHardNotToFail, and that would also be unique for a > > search, but it hurts readability of the code because it is noise that makes me > > wonder if there's something funky going on with the code. > > Ok, does CreateCacheBackendForTesting sound better? I still like the uniqueness, You can go with something that doesn't say "CreateBackend" if you are worried about that being mixed with another method. I'm not that happy either with "ForTesting" here because this is not a production class (where "ForTesting" has a very specific meaning)... and this class is already test specific... everything it does is for tests. > even if it adds clutter in the name, it gets much easier to navigate the code. I > often read the code in non-linear ways in vim with exuberant-ctags (yeah, I > know, sounds ancient), so the 10 variants of CreateBackend are bothering in > similar ways as multiple wrapper layers do. Am I not cooking it right? I thought we were going to have only one version (after removing the factories from all backends) :) This is interesting because we have almost opposite use cases. I'm either following the code, in which case something like "go to declaration" is what I would use (and the name doesn't matter), or all I know is that I want to go to a function that does foo and lives in a given file, in which case I go to that file and start looking for the name (and this is a more common use case). The problem is that having unique names doesn't work that well in that case, and I much rather have the same functionality methods have the same names because then it is easier to know or remember what to look for: I'm at foo in file x, now look for foo on file y and that's it, instead of trying to remember a synonym of "foo" because file y doesn't use the same name but rather something that is almost like it. I thought you were talking about doing global searches for foo and having 10 pages of results to wade through to get to the one that you were after... and I sympathize in that case but I much rather have too many results and have to filter for path, or see less context for each result than having no result because I cannot remember the proper incantation used on the class that I'm after. And note that varying post-fixes doesn't help because it's helpful to look for methods by including the open parenthesis at the end of the name. > > > I think it is more important for the code to read naturally, without > extraneous > > decorations. > > I have this argument about code spanning across multiple files, where it is > easier to read by jumping across files by exact named pointers rather than very > common names. There are obviously very common names like Run() which I did not > learn how to deal with. https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/disk_cache... net/disk_cache/disk_cache_test_base.h:65: void InitDefaultCacheViaCreator(); On 2013/03/29 02:20:26, pasko wrote: > On 2013/03/28 22:30:21, rvargas wrote: > > On 2013/03/28 21:41:42, pasko wrote: > > > On 2013/03/28 03:12:45, rvargas wrote: > > > > nit: InitCacheViaCreator ? InitDefaultCache ? > > > > > > > > (probably #2 given that it should not really use the creator :) ) > > > > > > The whole point of this function is to test via Creator. I'll remove it if > we > > > > I don't agree there. The point is to use the regular interface that clients > use. > > I agree that we should test the regular interface when possible. And very soon I meant the point of this method, not the point in general... in general, we are moving almost all tests to diverge from testing the interface... to the point of eliminating UseImplementation() on this CL, and making it the default. Use of this method should be very uncommon. > the CreateCacheBackend may become not possible to use on all platforms in the > same way. Disabling tests for those platforms is probably your suggestion. That > would certainly work. > > > > can do the same with CreateCacheBackend(). The word Default suggests that we > > > cannot control which backend to create, it chooses the platform's default. > > > > And we cannot control it beyond what a regular caller should be able to > control. > > We agree that the word default is right, no? > > I did not understand this last sentence :) yeah... I was just saying that I got lost because we ended up talking about the meaning of the word "default" as being part of the name and I thought you were fine with it (as I am). Let's move on :) > > OK, in the next CL I am trying to use the official CreateCacheBackend(), to > replace all calls to InitDefaultCacheViaCreator().
I made a few small changes: naming and expoerting the SimpleBackendImpl for windows. Letting the CQ decide. Looking now at eliminating the InitDefaultCacheViaCreator(). https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/disk_cache... File net/disk_cache/disk_cache_test_base.h (right): https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/disk_cache... net/disk_cache/disk_cache_test_base.h:62: void CreateBackendAsModeSuggests(uint32 flags, base::Thread* thread); renamed to DiskCacheTestWithCache::CreateBackend https://codereview.chromium.org/12794003/diff/67001/net/disk_cache/disk_cache... net/disk_cache/disk_cache_test_base.h:62: void CreateBackendAsModeSuggests(uint32 flags, base::Thread* thread); On 2013/03/29 06:49:22, rvargas wrote: > > even if it adds clutter in the name, it gets much easier to navigate the code. > I > > often read the code in non-linear ways in vim with exuberant-ctags (yeah, I > > know, sounds ancient), so the 10 variants of CreateBackend are bothering in > > similar ways as multiple wrapper layers do. Am I not cooking it right? > > I thought we were going to have only one version (after removing the factories > from all backends) :) > > This is interesting because we have almost opposite use cases. [...] I want to go to a > function that does foo and lives in a given file, in which case I go to that > file and start looking for the name (and this is a more common use case). Interesting. That's the difference, I normally ignore file names, I jump to function declaration or grep for its callsites. Every time there is a different function with the same name, it adds a mental barrier, following the code gets down from a fraction of a second to dozens of seconds or even minutes. I only notice file names occasionally. The code is important, filenames are secondary. As far as I understand these days the main reason for separating code into multiple files is parallel build. https://codereview.chromium.org/12794003/diff/81001/net/tools/crash_cache/cra... File net/tools/crash_cache/crash_cache.cc (right): https://codereview.chromium.org/12794003/diff/81001/net/tools/crash_cache/cra... net/tools/crash_cache/crash_cache.cc:142: path, thread->message_loop_proxy(), NULL); On 2013/03/28 22:30:21, rvargas wrote: > nit: I'm really sorry for not seeing this earlier, but could you break the line > at the = instead. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pasko@google.com/12794003/125001
Failed to apply patch for net/disk_cache/cache_creator.cc:
While running patch -p0 --forward --force --no-backup-if-mismatch;
patching file net/disk_cache/cache_creator.cc
Hunk #1 FAILED at 2.
Hunk #2 FAILED at 219.
Hunk #3 FAILED at 236.
Hunk #4 FAILED at 247.
4 out of 4 hunks FAILED -- saving rejects to file
net/disk_cache/cache_creator.cc.rej
Patch: N net/disk_cache/cache_creator.cc
Index: net/disk_cache/cache_creator.cc
===================================================================
--- net/disk_cache/cache_creator.cc (revision 190930)
+++ net/disk_cache/cache_creator.cc (working copy)
@@ -2,215 +2,62 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
-#include "net/disk_cache/backend_impl.h"
-
-#include "base/bind.h"
-#include "base/bind_helpers.h"
#include "base/file_util.h"
-#include "base/files/file_path.h"
-#include "base/hash.h"
-#include "base/message_loop.h"
#include "base/metrics/field_trial.h"
-#include "base/metrics/histogram.h"
-#include "base/metrics/stats_counters.h"
-#include "base/rand_util.h"
-#include "base/string_util.h"
#include "base/stringprintf.h"
-#include "base/sys_info.h"
-#include "base/threading/thread_restrictions.h"
-#include "base/threading/worker_pool.h"
-#include "base/time.h"
-#include "base/timer.h"
#include "net/base/net_errors.h"
+#include "net/disk_cache/backend_impl.h"
#include "net/disk_cache/cache_util.h"
-#include "net/disk_cache/entry_impl.h"
-#include "net/disk_cache/errors.h"
-#include "net/disk_cache/experiments.h"
-#include "net/disk_cache/file.h"
#include "net/disk_cache/mem_backend_impl.h"
#include "net/disk_cache/simple/simple_backend_impl.h"
-// This has to be defined before including histogram_macros.h from this file.
-#define NET_DISK_CACHE_BACKEND_IMPL_CC_
-#include "net/disk_cache/histogram_macros.h"
+namespace disk_cache {
-using base::Time;
-using base::TimeDelta;
-using base::TimeTicks;
-
-namespace {
-
-const char* kIndexName = "index";
-const int kMaxOldFolders = 100;
-
-// Seems like ~240 MB correspond to less than 50k entries for 99% of the
people.
-// Note that the actual target is to keep the index table load factor under 55%
-// for most users.
-const int k64kEntriesStore = 240 * 1000 * 1000;
-const int kBaseTableLen = 64 * 1024;
-const int kDefaultCacheSize = 80 * 1024 * 1024;
-
-// Avoid trimming the cache for the first 5 minutes (10 timer ticks).
-const int kTrimDelay = 10;
-
-int DesiredIndexTableLen(int32 storage_size) {
- if (storage_size <= k64kEntriesStore)
- return kBaseTableLen;
- if (storage_size <= k64kEntriesStore * 2)
- return kBaseTableLen * 2;
- if (storage_size <= k64kEntriesStore * 4)
- return kBaseTableLen * 4;
- if (storage_size <= k64kEntriesStore * 8)
- return kBaseTableLen * 8;
-
- // The biggest storage_size for int32 requires a 4 MB table.
- return kBaseTableLen * 16;
-}
-
-int MaxStorageSizeForTable(int table_len) {
- return table_len * (k64kEntriesStore / kBaseTableLen);
-}
-
-size_t GetIndexSize(int table_len) {
- size_t table_size = sizeof(disk_cache::CacheAddr) * table_len;
- return sizeof(disk_cache::IndexHeader) + table_size;
-}
-
-// ------------------------------------------------------------------------
-
-// Returns a fully qualified name from path and name, using a given name prefix
-// and index number. For instance, if the arguments are "/foo", "bar" and 5, it
-// will return "/foo/old_bar_005".
-base::FilePath GetPrefixedName(const base::FilePath& path,
- const std::string& name,
- int index) {
- std::string tmp = base::StringPrintf("%s%s_%03d", "old_",
- name.c_str(), index);
- return path.AppendASCII(tmp);
-}
-
-// This is a simple callback to cleanup old caches.
-void CleanupCallback(const base::FilePath& path, const std::string& name) {
- for (int i = 0; i < kMaxOldFolders; i++) {
- base::FilePath to_delete = GetPrefixedName(path, name, i);
- disk_cache::DeleteCache(to_delete, true);
+CacheCreator::CacheCreator(
+ const base::FilePath& path, bool force, int max_bytes,
+ net::CacheType type, uint32 flags,
+ base::MessageLoopProxy* thread, net::NetLog* net_log,
+ disk_cache::Backend** backend,
+ const net::CompletionCallback& callback)
+ : path_(path),
+ force_(force),
+ retry_(false),
+ max_bytes_(max_bytes),
+ type_(type),
+ flags_(flags),
+ thread_(thread),
+ backend_(backend),
+ callback_(callback),
+ created_cache_(NULL),
+ net_log_(net_log) {
}
-}
-// Returns a full path to rename the current cache, in order to delete it. path
-// is the current folder location, and name is the current folder name.
-base::FilePath GetTempCacheName(const base::FilePath& path,
- const std::string& name) {
- // We'll attempt to have up to kMaxOldFolders folders for deletion.
- for (int i = 0; i < kMaxOldFolders; i++) {
- base::FilePath to_delete = GetPrefixedName(path, name, i);
- if (!file_util::PathExists(to_delete))
- return to_delete;
- }
- return base::FilePath();
+CacheCreator::~CacheCreator() {
}
-// Moves the cache files to a new folder and creates a task to delete them.
-bool DelayedCacheCleanup(const base::FilePath& full_path) {
- // GetTempCacheName() and MoveCache() use synchronous file
- // operations.
- base::ThreadRestrictions::ScopedAllowIO allow_io;
-
- base::FilePath current_path = full_path.StripTrailingSeparators();
-
- base::FilePath path = current_path.DirName();
- base::FilePath name = current_path.BaseName();
-#if defined(OS_POSIX)
- std::string name_str = name.value();
-#elif defined(OS_WIN)
- // We created this file so it should only contain ASCII.
- std::string name_str = WideToASCII(name.value());
-#endif
-
- base::FilePath to_delete = GetTempCacheName(path, name_str);
- if (to_delete.empty()) {
- LOG(ERROR) << "Unable to get another cache folder";
- return false;
- }
-
- if (!disk_cache::MoveCache(full_path, to_delete)) {
- LOG(ERROR) << "Unable to move cache folder " << full_path.value() << " to "
- << to_delete.value();
- return false;
- }
-
- base::WorkerPool::PostTask(
- FROM_HERE, base::Bind(&CleanupCallback, path, name_str), true);
- return true;
-}
-
-// Sets group for the current experiment. Returns false if the files should be
-// discarded.
-bool InitExperiment(disk_cache::IndexHeader* header) {
- if (header->experiment == disk_cache::EXPERIMENT_OLD_FILE1 ||
- header->experiment == disk_cache::EXPERIMENT_OLD_FILE2) {
- // Discard current cache.
- return false;
- }
-
- header->experiment = disk_cache::NO_EXPERIMENT;
- return true;
-}
-
-// ------------------------------------------------------------------------
-
-// This class takes care of building an instance of the backend.
-class CacheCreator {
- public:
- CacheCreator(const base::FilePath& path, bool force, int max_bytes,
- net::CacheType type, uint32 flags,
- base::MessageLoopProxy* thread, net::NetLog* net_log,
- disk_cache::Backend** backend,
- const net::CompletionCallback& callback)
- : path_(path),
- force_(force),
- retry_(false),
- max_bytes_(max_bytes),
- type_(type),
- flags_(flags),
- thread_(thread),
- backend_(backend),
- callback_(callback),
- cache_(NULL),
- net_log_(net_log) {
- }
- ~CacheCreator() {}
-
- // Creates the backend.
- int Run();
-
- private:
- void DoCallback(int result);
-
- // Callback implementation.
- void OnIOComplete(int result);
-
- const base::FilePath& path_;
- bool force_;
- bool retry_;
- int max_bytes_;
- net::CacheType type_;
- uint32 flags_;
- scoped_refptr<base::MessageLoopProxy> thread_;
- disk_cache::Backend** backend_;
- net::CompletionCallback callback_;
- disk_cache::BackendImpl* cache_;
- net::NetLog* net_log_;
-
- DISALLOW_COPY_AND_ASSIGN(CacheCreator);
-};
-
int CacheCreator::Run() {
- cache_ = new disk_cache::BackendImpl(path_, thread_, net_log_);
- cache_->SetMaxSize(max_bytes_);
- cache_->SetType(type_);
- cache_->SetFlags(flags_);
- int rv = cache_->Init(
+ // TODO(pasko): The two caches should never coexist on disk. Each individual
+ // cache backend should fail to initialize if it observes the index that does
+ // not belong to it.
+ if (base::FieldTrialList::FindFullName("SimpleCacheTrial") == "Yes") {
+ // TODO(gavinp,pasko): While simple backend development proceeds, we're
only
+ // testing it against net::DISK_CACHE. Turn it on for more cache types as
+ // appropriate.
+ if (type_ == net::DISK_CACHE) {
+ VLOG(1) << "Using the Simple Cache Backend.";
+ return disk_cache::SimpleBackendImpl::CreateBackend(
+ path_, max_bytes_, type_, disk_cache::kNone, thread_, net_log_,
+ backend_, base::Bind(&CacheCreator::OnIOComplete,
+ base::Unretained(this)));
+ }
+ }
+ disk_cache::BackendImpl* new_cache =
+ new disk_cache::BackendImpl(path_, thread_, net_log_);
+ created_cache_ = new_cache;
+ new_cache->SetMaxSize(max_bytes_);
+ new_cache->SetType(type_);
+ new_cache->SetFlags(flags_);
+ int rv = new_cache->Init(
base::Bind(&CacheCreator::OnIOComplete, base::Unretained(this)));
DCHECK_EQ(net::ERR_IO_PENDING, rv);
return rv;
@@ -219,16 +66,18 @@
void CacheCreator::DoCallback(int result) {
DCHECK_NE(net::ERR_IO_PENDING, result);
if (result == net::OK) {
- *backend_ = cache_;
+ *backend_ = created_cache_;
} else {
LOG(ERROR) << "Unable to create cache";
*backend_ = NULL;
- delete cache_;
+ delete created_cache_;
}
callback_.Run(result);
delete this;
}
+// If the initialization of the cache fails, and |force| is true, we will
+// discard the whole cache and cre…
(message too large)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pasko@google.com/12794003/140001
Presubmit check for 12794003-140001 failed and returned exit status 1.
INFO:root:Found 13 file(s).
Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/net/PRESUBMIT.py
** Presubmit ERRORS **
Banned functions were used.
net/disk_cache/cache_util.cc:63:
New code should not use ScopedAllowIO. Post a task to the blocking
pool or the FILE thread instead.
Presubmit checks took 1.7s to calculate.
Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!
On 2013/04/01 12:56:02, I haz the power (commit-bot) wrote: > Presubmit check for 12794003-140001 failed and returned exit status 1. > > INFO:root:Found 13 file(s). > > Running presubmit commit checks ... > Running /b/commit-queue/workdir/chromium/PRESUBMIT.py > Running /b/commit-queue/workdir/chromium/net/PRESUBMIT.py > > ** Presubmit ERRORS ** > Banned functions were used. > net/disk_cache/cache_util.cc:63: > New code should not use ScopedAllowIO. Post a task to the blocking > pool or the FILE thread instead. This is not new code, just moving the function to a separate utility file. I added the file to the blacklist for now. Adding maruel@ to review this tiny part.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pasko@google.com/12794003/146001
Message was sent while issue was closed.
Change committed as 191830
Message was sent while issue was closed.
PRESUBMIT.py change lgtm |
