Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12192005:
Add new simple disk cache backend. (Closed)
|
Created:
7 years, 10 months ago by gavinp Modified:
7 years, 9 months ago Reviewers:
pasko, felipeg, rvargas (doing something else), willchan no longer on Chromium, pasko-google - do not use CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org, gavinp+disk_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd new simple disk cache backend.
Introducing the simple backend that stores one disk_cache entry per file.
At this point, entries can be created, doomed, and basic caching appears to work. It is asynchronous, performing all IO operations on a worker pool. It's careful enough about lifetime that it should be possible to delete quickly, without error or races.
It's missing many features of a real disk cache. Most notably, it can't detect corruption, it does not have any kind of IO thread bitmap/hashes to allow fast responses, and it does not have eviction at all.
This issue is downstream of https://codereview.chromium.org/12207120/ (remove bad const from disk cache interfaces), which must land before it.
It is upstream of https://codereview.chromium.org/12226095/ (Make synchronous methods reentrant), and must land before that issue.
R=rvargas@chromium.org,pasko@chromium.org
BUG=173388, 173390
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182845
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=184493
Patch Set 1 #Patch Set 2 : clean up includes, add using #Patch Set 3 : don't actually use it #Patch Set 4 : lint and constructor privatization #Patch Set 5 : more using #Patch Set 6 : conditional compilation #Patch Set 7 : rebase to trunk (::FilePath --> base::FilePath) #
Total comments: 95
Patch Set 8 : rename only upload #Patch Set 9 : remediation to review #
Total comments: 20
Patch Set 10 : pasko remediation #Patch Set 11 : move pending operation classes around #Patch Set 12 : moar logging #Patch Set 13 : moar logging #Patch Set 14 : indent #Patch Set 15 : moar indent #Patch Set 16 : clean up some NOTIMPLEMENTED call sites #Patch Set 17 : fix dcheck #
Total comments: 10
Patch Set 18 : rebase before remediation #
Total comments: 2
Patch Set 19 : remediate to felipeg review #
Total comments: 47
Patch Set 20 : remediation #
Total comments: 3
Patch Set 21 : fix dcheck #
Total comments: 6
Patch Set 22 : clear to land #Patch Set 23 : build fix, log fix #Patch Set 24 : add header #Patch Set 25 : rebase #Patch Set 26 : build fix #Messages
Total messages: 37 (0 generated)
Ricardo, I've cleaned up the patches we've been passing around, and here's the beginning of a very simple cache. I'd like to land this so that incremental changes can be properly code reviewed as we go forward.
Egor: Please take a look as well. +cc felipeg, but he's out next week, so we shouldn't block on his input.
looking only at the header there is a nit: > Introducing the very_simple backend that stores one disk_cache entry per file. one disk_cache entry corresponds to 3 files currently
I got the first look at the cache creation logic. This set of comments resulted from my fantasies on the subject of hypothetical historical intentions of the backend_impl authors. Obviously, I am not an expert in this kind of retrospection, though I certainly would like to know from current backend hackers whether my suggestions are more aligned with the their vision. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/backend_imp... File net/disk_cache/backend_impl.cc (right): https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/backend_imp... net/disk_cache/backend_impl.cc:271: return VerySimpleBackendImpl::BackendFactory(path). This discards logic of the CacheCreator that, whether we need it or not, performs cleanup over a moved cache directory. There seem to be no tests covering that, but anyway, there is no strong reason to make this behavior different on, say, a different platform. Since we are here, note for the future: When we introduce the tracing backend, we would need to govern the creation of all 4 combinations: current,simple,current+tracing,simple+tracing. At first glance it seems like CacheCreator is a better place. Encoding the necessary cache creation algorithm in CacheType would require adding 3 more types. It is certainly not the best possible idea, want to know your thoughts on this. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... File net/disk_cache/very_simple/very_simple_backend_impl.cc (right): https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_backend_impl.cc:15: using base::Bind; nit: although allowed by our style, in net/ the uses are via base::Bind, should keep it consistent https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_backend_impl.cc:52: VLOG(0) << "VerySimpleBackendImpl::BackendFactory::CreateBackend()"; identical message as below, high log level, please remove one https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_backend_impl.cc:132: return net::ERR_FAILED; Just curious, do the callers always workaround such kind of failures gracefully? If they sometimes misbehave in such situations, better put a deadly DCHECK here. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_backend_impl.cc:154: return new VerySimpleBackendImpl(path); simpler would be to do this one liner in OnCachePathCreated. I am having a constant buzz in my head with various kinds of Create, Factory, Callback etc. Reducing the number of them would help reducing the buzz. https://codereview.chromium.org/12192005/diff/9004/net/net.gyp File net/net.gyp (right): https://codereview.chromium.org/12192005/diff/9004/net/net.gyp#newcode1047 net/net.gyp:1047: 'disk_cache/very_simple/very_simple_backend_impl.cc', I prefer runtime command-line flag because: 1. no need to gclient runhooks and rebuild, easier to test on bots 2. no ugly ifdefs 3. no repeating ourselves when removing the files conditionally in gyp 4. adds only tiny amount of code Though it is not a super-strong opinion, I am OK following Ricardo's preference, I do not know his opinion on it yet
https://codereview.chromium.org/12192005/diff/9004/net/DEPS File net/DEPS (right): https://codereview.chromium.org/12192005/diff/9004/net/DEPS#newcode8 net/DEPS:8: "+third_party/smhasher", I'm fine with switching to murmur, but I have the impression that the result of the hash is not the same across architectures. Is that not the case? https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/backend_imp... File net/disk_cache/backend_impl.cc (right): https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/backend_imp... net/disk_cache/backend_impl.cc:272: CreateBackend(net_log, backend, callback); - Why is this split in two? (build factory + Create Backend) - No size limit? - No thread restriction? is |force| assumed to be true? It is better to pass everything that _may_ be needed even if the current implementation ignores it. (of course, unless we know that something is not going to be needed). https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... File net/disk_cache/very_simple/very_simple_backend_impl.cc (right): https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_backend_impl.cc:13: #include "net/disk_cache/very_simple/very_simple_entry_impl.h" How about renaming everything to simple, instead of very_simple? https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_backend_impl.cc:23: namespace { nit: can this be outside of disk_cache namespace? https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_backend_impl.cc:52: VLOG(0) << "VerySimpleBackendImpl::BackendFactory::CreateBackend()"; Don't add tracing logs https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_backend_impl.cc:64: base::WorkerPool::PostTask( nit: first argument here. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_backend_impl.cc:68: Bind(&VerySimpleBackendImpl::Create, wow, three nested Binds :( Can we just split this instead of trying to control the whole process from one place (here)? Hopefully removing BackendFactory makes things a little easier to follow. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_backend_impl.cc:76: DCHECK(thread_checker_.CalledOnValidThread()); nit: seems overkill, considering that all methods are used on the same thread, and the default for chrome code is that classes are single-thread (so it would follow that all classes would have to do this) https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_backend_impl.cc:85: return 0; Todo? https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_backend_impl.cc:110: return net::ERR_FAILED; Todo? etc https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_backend_impl.cc:149: const FilePath& path) : path_(path) { The general idea is that at this point contents of the cache are read and any access issue should be resolved/reported. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... File net/disk_cache/very_simple/very_simple_backend_impl.h (right): https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_backend_impl.h:16: #include "net/http/http_cache.h" logical layering violation https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_backend_impl.h:20: class VerySimpleBackendImpl : public Backend { Description of this class would be good... a file description would be even better. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_backend_impl.h:22: class BackendFactory : public net::HttpCache::BackendFactory { This should not be an HttpCache::BackednFactory https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_backend_impl.h:64: static VerySimpleBackendImpl* Create(const FilePath& path); Given that there is a factory method why do we need a factory class? https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... File net/disk_cache/very_simple/very_simple_disk_format.h (right): https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_disk_format.h:10: const uint64 kVerySimpleInitialMagicNumber = 0x10adab1ebeef; nit: something vegetarian? Seriously, avoid common patterns. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_disk_format.h:15: const uint32 kVerySimpleVersion = 0; nit: 0? https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... File net/disk_cache/very_simple/very_simple_entry_impl.cc (right): https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_entry_impl.cc:27: namespace { nit: can this be out of disk_cache? https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_entry_impl.cc:46: PendingCreationOperation* operation = Could you separate the implementation from the declaration of this class? (declaration on this file is totally fine) https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_entry_impl.cc:68: DCHECK_EQ(static_cast<VerySimpleSynchronousEntry*>(NULL), sync_entry); nit: DCHECK(!sync_entry) https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_entry_impl.cc:73: *out_entry_ = entry_factory_callback_.Run(sync_entry); I'm not so happy with this pattern of run a task and pass another task that should be executed on the callback, before running the user callback :( Why not a simple PostTaskAndReply, where the control flow is straightforward (at the cost of an extra method for the continuation... that in this case would be basically this method)? https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_entry_impl.cc:83: class PendingOperation { nit: class description https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_entry_impl.cc:88: PendingOperation* operation = new PendingOperation(set_sync_callback, ditto (implementation vs declaration) https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_entry_impl.cc:102: set_sync_callback_.Run(sync_entry); ditto https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_entry_impl.cc:120: WorkerPool::PostTask(FROM_HERE, You know that in general we don't like the worker pool for anything other than very basic, pure OS related stuff, right? https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_entry_impl.cc:158: Bind(&VerySimpleSynchronousEntry::DoomAndClose, DoomAndClose? https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_entry_impl.cc:213: Unretained(ReleaseSynchronousEntry()), index, is this intended to enforce exclusive access to simple members? The interface doesn't prevent issuing multiple simultaneous reads. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_entry_impl.cc:214: offset, Unretained(buf), buf_len, cannot be unretained https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_entry_impl.cc:282: key_(synchronous_entry->key()), why duplicate the key? https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_entry_impl.cc:290: DCHECK_EQ(static_cast<VerySimpleSynchronousEntry*>(NULL), synchronous_entry_); !sync_entry_ https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... File net/disk_cache/very_simple/very_simple_synchronous_entry.cc (right): https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_synchronous_entry.cc:82: callback_runner->PostTask(FROM_HERE, Bind(callback, sync_entry, net::OK)); nit: merge the two PostTasks? https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_synchronous_entry.cc:114: file_util::Delete(path.AppendASCII(GetFilenameForKeyAndIndex(key, i)), How is Read and Write expected to work after this? https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_synchronous_entry.cc:253: VLOG(8) << "VerySimpleSynchronousEntry::InitializeForOpen"; nit: general tracing is bad https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_synchronous_entry.cc:258: VLOG(9) << " no header"; ... and logging on errors is good. https://codereview.chromium.org/12192005/diff/9004/net/net.gyp File net/net.gyp (right): https://codereview.chromium.org/12192005/diff/9004/net/net.gyp#newcode10 net/net.gyp:10: 'use_very_simple_backend%': 0, do we really need a gyp variable? https://codereview.chromium.org/12192005/diff/9004/net/net.gyp#newcode1046 net/net.gyp:1046: 'sources!': [ why remove these files?
Please take a look. I have addressed most comments. https://codereview.chromium.org/12192005/diff/9004/net/DEPS File net/DEPS (right): https://codereview.chromium.org/12192005/diff/9004/net/DEPS#newcode8 net/DEPS:8: "+third_party/smhasher", On 2013/02/06 03:28:40, rvargas wrote: > I'm fine with switching to murmur, but I have the impression that the result of > the hash is not the same across architectures. Is that not the case? Well, there's different versions for different architectures x86 and x64. They both run on the other architecture, but at reduced speed. This CL just uses the x86 one, which works, but isn't optimal on the x64 platform. For endianness, we're sort of pooched anyway. Thankfully our magic number check catches that early. So to use it properly, you need to detect which CPU is in use, then use the right one, and save that in the cache, so that you use the same one in the future. That's not simple. So I'm switching back to using very fast hash from base/, which is the simple choice. A future person concerned with the last ounce of performance can move murmur3 into base/, and write up logic to help pick the right on. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/backend_imp... File net/disk_cache/backend_impl.cc (right): https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/backend_imp... net/disk_cache/backend_impl.cc:271: return VerySimpleBackendImpl::BackendFactory(path). On 2013/02/05 21:20:33, pasko wrote: > This discards logic of the CacheCreator that, whether we need it or not, > performs cleanup over a moved cache directory. > > There seem to be no tests covering that, but anyway, there is no strong reason > to make this behavior different on, say, a different platform. > > Since we are here, note for the future: > > When we introduce the tracing backend, we would need to govern the creation of > all 4 combinations: current,simple,current+tracing,simple+tracing. At first > glance it seems like CacheCreator is a better place. Encoding the necessary > cache creation algorithm in CacheType would require adding 3 more types. > > It is certainly not the best possible idea, want to know your thoughts on this. I think this is a good idea. And I think that this argues for CacheCreator moving out of backend_impl.cc into a separate file, too. It's only in backend_impl.cc just because at present it's an artifact of backend_impl.cc, after all. This seems to touch your tracing work more closely. Do you want to fix it while you do that? https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/backend_imp... net/disk_cache/backend_impl.cc:272: CreateBackend(net_log, backend, callback); On 2013/02/06 03:28:40, rvargas wrote: > - Why is this split in two? (build factory + Create Backend) > - No size limit? > - No thread restriction? is |force| assumed to be true? > > It is better to pass everything that _may_ be needed even if the current > implementation ignores it. (of course, unless we know that something is not > going to be needed). Done. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... File net/disk_cache/very_simple/very_simple_backend_impl.cc (right): https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_backend_impl.cc:13: #include "net/disk_cache/very_simple/very_simple_entry_impl.h" On 2013/02/06 03:28:40, rvargas wrote: > How about renaming everything to simple, instead of very_simple? Done. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_backend_impl.cc:15: using base::Bind; On 2013/02/05 21:20:33, pasko wrote: > nit: > > although allowed by our style, in net/ the uses are via base::Bind, should keep > it consistent Done, although with grumpiness. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_backend_impl.cc:23: namespace { On 2013/02/06 03:28:40, rvargas wrote: > nit: can this be outside of disk_cache namespace? Done. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_backend_impl.cc:52: VLOG(0) << "VerySimpleBackendImpl::BackendFactory::CreateBackend()"; On 2013/02/06 03:28:40, rvargas wrote: > Don't add tracing logs Done. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_backend_impl.cc:64: base::WorkerPool::PostTask( On 2013/02/06 03:28:40, rvargas wrote: > nit: first argument here. Done. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_backend_impl.cc:68: Bind(&VerySimpleBackendImpl::Create, On 2013/02/06 03:28:40, rvargas wrote: > wow, three nested Binds :( > > Can we just split this instead of trying to control the whole process from one > place (here)? > > Hopefully removing BackendFactory makes things a little easier to follow. Goes to show that a dedicated programmer can write a LISP program in any language. I'll make this chain more explicit and less front loaded, in the C++ style. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_backend_impl.cc:76: DCHECK(thread_checker_.CalledOnValidThread()); On 2013/02/06 03:28:40, rvargas wrote: > nit: seems overkill, considering that all methods are used on the same thread, > and the default for chrome code is that classes are single-thread (so it would > follow that all classes would have to do this) I agree it is overkill technically. I am including it for documentation purposes. Reading the cache code for threading in the past few weeks, I've found myself spending a lot of time tracing paths so I could be completely certain which thread we were on at each point. With these DCHECKs, anyone reading the code has an easy reference they can check. And it's far more reliable than a comment. If a future editor of this code is as bad at remembering which method is used on which thread as I am, then they will be happy these were here. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_backend_impl.cc:85: return 0; On 2013/02/06 03:28:40, rvargas wrote: > Todo? Todo! https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_backend_impl.cc:110: return net::ERR_FAILED; On 2013/02/06 03:28:40, rvargas wrote: > Todo? etc Done. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_backend_impl.cc:132: return net::ERR_FAILED; On 2013/02/05 21:20:33, pasko wrote: > Just curious, do the callers always workaround such kind of failures gracefully? > If they sometimes misbehave in such situations, better put a deadly DCHECK here. > I'll go deadly DCHECK, since I don't know. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_backend_impl.cc:149: const FilePath& path) : path_(path) { On 2013/02/06 03:28:40, rvargas wrote: > The general idea is that at this point contents of the cache are read and any > access issue should be resolved/reported. Yes. That was true in the patch you were reviewing, and that's true in the new upload too. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_backend_impl.cc:154: return new VerySimpleBackendImpl(path); On 2013/02/05 21:20:33, pasko wrote: > simpler would be to do this one liner in OnCachePathCreated. I am having a > constant buzz in my head with various kinds of Create, Factory, Callback etc. > Reducing the number of them would help reducing the buzz. Happy to. I think I was being too averse to making the header file complex; but adding a one line factory method just to avoid putting OnCachePathCreated in the class is... me being silly. Thanks. The new upload looks a lot more like C++. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... File net/disk_cache/very_simple/very_simple_backend_impl.h (right): https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_backend_impl.h:16: #include "net/http/http_cache.h" On 2013/02/06 03:28:40, rvargas wrote: > logical layering violation Done. Can you please tell us more about what HttpCache::BackendFactory is intended for? https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_backend_impl.h:20: class VerySimpleBackendImpl : public Backend { On 2013/02/06 03:28:40, rvargas wrote: > Description of this class would be good... a file description would be even > better. Done. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_backend_impl.h:22: class BackendFactory : public net::HttpCache::BackendFactory { On 2013/02/06 03:28:40, rvargas wrote: > This should not be an HttpCache::BackednFactory Done. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_backend_impl.h:64: static VerySimpleBackendImpl* Create(const FilePath& path); On 2013/02/06 03:28:40, rvargas wrote: > Given that there is a factory method why do we need a factory class? See my comments to Egor. I was being silly in keeping code out of the interface. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... File net/disk_cache/very_simple/very_simple_disk_format.h (right): https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_disk_format.h:10: const uint64 kVerySimpleInitialMagicNumber = 0x10adab1ebeef; On 2013/02/06 03:28:40, rvargas wrote: > nit: something vegetarian? Seriously, avoid common patterns. Not so common: http://www.bing.com/search?q=0x10adab1ebeef Now replaced with a random number. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_disk_format.h:15: const uint32 kVerySimpleVersion = 0; On 2013/02/06 03:28:40, rvargas wrote: > nit: 0? Now 1. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... File net/disk_cache/very_simple/very_simple_entry_impl.cc (right): https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_entry_impl.cc:27: namespace { On 2013/02/06 03:28:40, rvargas wrote: > nit: can this be out of disk_cache? Done. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_entry_impl.cc:46: PendingCreationOperation* operation = On 2013/02/06 03:28:40, rvargas wrote: > Could you separate the implementation from the declaration of this class? > (declaration on this file is totally fine) Done. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_entry_impl.cc:68: DCHECK_EQ(static_cast<VerySimpleSynchronousEntry*>(NULL), sync_entry); On 2013/02/06 03:28:40, rvargas wrote: > nit: DCHECK(!sync_entry) Done. I had to add some printing cruft, since otherwise debugging output would suffer. What a shame that DCHECK_EQ(NULL, foo) doesn't work. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_entry_impl.cc:73: *out_entry_ = entry_factory_callback_.Run(sync_entry); On 2013/02/06 03:28:40, rvargas wrote: > I'm not so happy with this pattern of run a task and pass another task that > should be executed on the callback, before running the user callback :( > > Why not a simple PostTaskAndReply, where the control flow is straightforward (at > the cost of an extra method for the continuation... that in this case would be > basically this method)? I'm sorry to hear you're unhappy with this pattern. Why are you unhappy with this pattern? Can you explain what is wrong with it? It can be very difficult to respond to a short comment that says something is bad or wrong; especially when I don't understand why. I will try and elaborate on my thinking below. I hope it helps. I think it would be subtle to get thread safety right if we had the SynchronousEntryImpl directly called a task provided to the backend. I also don't understand why that is virtuous. In the case of creating an entry, I used the PendingCreationOperation for two goals. I wanted to delay creating the SimpleEntryImpl object until as late as possible, and provide it to the caller's Entry** as late as possible, to avoid there being a concept of an uninitialized SimpleEntryImpl and its required checks. By creating it in the callback, the SimpleEntryImpl is constructed with a valid open SynchronousEntry or it is not constructed. Since PostTaskAndReply requires that the reply be a Closure, this pattern would not work here: how would the SynchronousEntryImpl provide its pointer to the reply? I guess we could create the SynchronousEntryImpl on the IO thread, and construct our SimpleEntryImpl early, and now we have this uninitialized state I was trying to avoid. I think that is uglier than creating a limited scope callback that follows up on completed operations. In the case of ReadData() and WriteData(), we could use PostTaskAndReply if we kept our SimpleSynchronousEntry pointer, but then we'd be introducing an interesting class of data races. The reply necessarily occurs after the worker thread task is complete, so the callback has already run on the worker thread. To make this code less LISP like, and more C++ like, I've moved the PendingOperation and PendingCreationOperation to inner classes, and made them friends. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_entry_impl.cc:83: class PendingOperation { On 2013/02/06 03:28:40, rvargas wrote: > nit: class description Done. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_entry_impl.cc:88: PendingOperation* operation = new PendingOperation(set_sync_callback, On 2013/02/06 03:28:40, rvargas wrote: > ditto (implementation vs declaration) Done. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_entry_impl.cc:120: WorkerPool::PostTask(FROM_HERE, On 2013/02/06 03:28:40, rvargas wrote: > You know that in general we don't like the worker pool for anything other than > very basic, pure OS related stuff, right? I don't know this. Can you expand on this? https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_entry_impl.cc:158: Bind(&VerySimpleSynchronousEntry::DoomAndClose, On 2013/02/06 03:28:40, rvargas wrote: > DoomAndClose? DoomAndClose! https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_entry_impl.cc:213: Unretained(ReleaseSynchronousEntry()), index, On 2013/02/06 03:28:40, rvargas wrote: > is this intended to enforce exclusive access to simple members? The interface > doesn't prevent issuing multiple simultaneous reads. This is intended to enforce thread safety, and the easiest way to do that was to enforce exclusive access using the memory barrier in PostTask for safety. The use of the WorkerPool in particular makes this work well. Yes, the interface does not prevent issuing multiple simultaneous reads. In your comment, you do not elaborate about where multiple simultaneous reads occurs when in an HttpCache::Transaction, but that can and does occur when an existing transaction has an HttpCache::ActiveEntry open only for read; a second reader is allowed in. This DCHECK will crash when that happens. I have added a TODO. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_entry_impl.cc:214: offset, Unretained(buf), buf_len, On 2013/02/06 03:28:40, rvargas wrote: > cannot be unretained Tell me more? https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_entry_impl.cc:282: key_(synchronous_entry->key()), On 2013/02/06 03:28:40, rvargas wrote: > why duplicate the key? Thread safe access. In practice, if SimpleSynchronousEntry::key_ was const, we could retain a pointer to it and use that across threads safely. However, the copy was conceptually simpler for me to deal with and didn't require that argument. Do you have another suggestion? https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_entry_impl.cc:290: DCHECK_EQ(static_cast<VerySimpleSynchronousEntry*>(NULL), synchronous_entry_); On 2013/02/06 03:28:40, rvargas wrote: > !sync_entry_ Done. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... File net/disk_cache/very_simple/very_simple_synchronous_entry.cc (right): https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_synchronous_entry.cc:82: callback_runner->PostTask(FROM_HERE, Bind(callback, sync_entry, net::OK)); On 2013/02/06 03:28:40, rvargas wrote: > nit: merge the two PostTasks? Done. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_synchronous_entry.cc:114: file_util::Delete(path.AppendASCII(GetFilenameForKeyAndIndex(key, i)), On 2013/02/06 03:28:40, rvargas wrote: > How is Read and Write expected to work after this? They are not. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_synchronous_entry.cc:253: VLOG(8) << "VerySimpleSynchronousEntry::InitializeForOpen"; On 2013/02/06 03:28:40, rvargas wrote: > nit: general tracing is bad Done. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_synchronous_entry.cc:258: VLOG(9) << " no header"; On 2013/02/06 03:28:40, rvargas wrote: > ... and logging on errors is good. Done. https://codereview.chromium.org/12192005/diff/9004/net/net.gyp File net/net.gyp (right): https://codereview.chromium.org/12192005/diff/9004/net/net.gyp#newcode10 net/net.gyp:10: 'use_very_simple_backend%': 0, On 2013/02/06 03:28:40, rvargas wrote: > do we really need a gyp variable? Can you tell me more about adding defines without using one? I don't understand the alternative.
https://codereview.chromium.org/12192005/diff/9004/net/net.gyp File net/net.gyp (right): https://codereview.chromium.org/12192005/diff/9004/net/net.gyp#newcode1046 net/net.gyp:1046: 'sources!': [ On 2013/02/06 03:28:40, rvargas wrote: > why remove these files? I copied the pattern from what our custom DNS resolver does.
Generally I like the approach. The synchronous entry acquire/release looks overly complicated at first glance, but after some time I have no _simple_ solution to that. Thanks for the TODO(pasko) in the initialization logic, more lovely work! https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... File net/disk_cache/very_simple/very_simple_backend_impl.cc (right): https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_backend_impl.cc:15: using base::Bind; On 2013/02/08 23:17:51, gavinp wrote: > On 2013/02/05 21:20:33, pasko wrote: > > nit: > > > > although allowed by our style, in net/ the uses are via base::Bind, should > keep > > it consistent > > Done, although with grumpiness. I understand your pain and grumpiness, please also understand that Grep, that poor thing, can be grumpier. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_backend_impl.cc:76: DCHECK(thread_checker_.CalledOnValidThread()); On 2013/02/08 23:17:51, gavinp wrote: > With these DCHECKs, anyone reading the code has an easy reference they can > check. And it's far more reliable than a comment. If a future editor of this > code is as bad at remembering which method is used on which thread as I am, then > they will be happy these were here. +1, these DCHECKs are good for documentation https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... File net/disk_cache/very_simple/very_simple_synchronous_entry.cc (right): https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_synchronous_entry.cc:114: file_util::Delete(path.AppendASCII(GetFilenameForKeyAndIndex(key, i)), On 2013/02/08 23:17:51, gavinp wrote: > On 2013/02/06 03:28:40, rvargas wrote: > > How is Read and Write expected to work after this? > > They are not. I think Ricardo refers to when we have pending Reads or Writes while someone decided to Doom. If so, would be good to know when that happens, I suspect some shutdown+eviction quirks? In our limited testing this approach should work, since on Linux one can read and write open files after they were deleted. https://codereview.chromium.org/12192005/diff/20012/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_backend_impl.cc (right): https://codereview.chromium.org/12192005/diff/20012/net/disk_cache/simple/sim... net/disk_cache/simple/simple_backend_impl.cc:48: EnsureCachePathExistsOnWorkerThread, nit: here and below adjust amount of leading space https://codereview.chromium.org/12192005/diff/20012/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/12192005/diff/20012/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:37: class SimpleEntryImpl::PendingOperation { maybe a PendingEntryOperation would be a better name because it suggests that an entry exists, and we are mutating it in good contrast with PendingCreationOperation. https://codereview.chromium.org/12192005/diff/20012/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:205: return synchronous_entry_->last_used(); is there a hidden assumption that last used cannot be requested while a write is in flight? If there is, someone could easily break it, I believe. I assume it will be solved by "support overlapping reads" below? https://codereview.chromium.org/12192005/diff/20012/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:228: // entry as read only. does it happen often? on what occasion? https://codereview.chromium.org/12192005/diff/20012/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:248: char c ALLOW_UNUSED = buf->data()[i]; It is hard to believe that this will be optimized out, and can be expensive. Is it data prefetching then? Hard to guess. https://codereview.chromium.org/12192005/diff/20012/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_synchronous_entry.cc (right): https://codereview.chromium.org/12192005/diff/20012/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:175: base::Bind(callback, this, net::ERR_FAILED)); I am wondering why some errors are logged and some others aren't. Should be consistent or comment where we handle these errors above the stack. https://codereview.chromium.org/12192005/diff/20012/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:232: last_used_ = std::max(last_used_, file_info.last_accessed); I am wondering how last_used is actually used. Here we probably want file_info.last_accessed although it is unsurprisingly rather last_modified on Linux and Android systems. https://codereview.chromium.org/12192005/diff/20012/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:253: DVLOG(6) << "Error: No magic number."; This is a pretty serious error, so sounds rather LOG(WARNING). Although probably needs to be put once in the log to avoid severe pollution. https://codereview.chromium.org/12192005/diff/20012/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:295: sizeof(header)) != sizeof(header)) { would be helpful: LOG(WARNING) or explanation where we log about this
Egor, Thanks for the review. This new upload has more discipline about logging, and also uses NOTIMPLEMENTED() rather than TODO(): not implemented comments. Right away I saw that usage is more asynchronous than previously thought. Very interesting! https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... File net/disk_cache/very_simple/very_simple_synchronous_entry.cc (right): https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_synchronous_entry.cc:114: file_util::Delete(path.AppendASCII(GetFilenameForKeyAndIndex(key, i)), On 2013/02/11 13:59:25, pasko wrote: > On 2013/02/08 23:17:51, gavinp wrote: > > On 2013/02/06 03:28:40, rvargas wrote: > > > How is Read and Write expected to work after this? > > > > They are not. > > I think Ricardo refers to when we have pending Reads or Writes while someone > decided to Doom. If so, would be good to know when that happens, I suspect some > shutdown+eviction quirks? In our limited testing this approach should work, > since on Linux one can read and write open files after they were deleted. Aha, yes. And we should support that, and we don't. I've gone through and added a lot more NOTIMPLEMENTED(), so it's clear what our limitations are. (I'm liking NOTIMPLEMENTED() better than "TODO(): implement this"). https://codereview.chromium.org/12192005/diff/20012/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_backend_impl.cc (right): https://codereview.chromium.org/12192005/diff/20012/net/disk_cache/simple/sim... net/disk_cache/simple/simple_backend_impl.cc:48: EnsureCachePathExistsOnWorkerThread, On 2013/02/11 13:59:25, pasko wrote: > nit: here and below adjust amount of leading space Previously I had lines like this with FROM_HERE on the next line, so the indentation of the bind was cleaner. Ricardo nitted all those FROM_HERE lines up to the line with the (. I'm happy to respond to your nit, but I don't know exactly what you're looking for, and Ricardo's earlier suggestion puts me in a "bind" here. Maybe it'd be clearest if I put the base::Bind() in a temp variable? Otherwise if you can give me some instruction so clear even I understand it, I'll be happy. :) https://codereview.chromium.org/12192005/diff/20012/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/12192005/diff/20012/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:37: class SimpleEntryImpl::PendingOperation { On 2013/02/11 13:59:25, pasko wrote: > maybe a PendingEntryOperation would be a better name because it suggests that an > entry exists, and we are mutating it in good contrast with > PendingCreationOperation. Done. https://codereview.chromium.org/12192005/diff/20012/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:205: return synchronous_entry_->last_used(); On 2013/02/11 13:59:25, pasko wrote: > is there a hidden assumption that last used cannot be requested while a write is > in flight? If there is, someone could easily break it, I believe. I assume it > will be solved by "support overlapping reads" below? Hopefully. The reads will change a bit once checksums are added; but I can see a few approaches to supporting them that work. I've made this NOTIMPLEMENTED() so it's more clear that a good answer is needed here. https://codereview.chromium.org/12192005/diff/20012/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:228: // entry as read only. On 2013/02/11 13:59:25, pasko wrote: > does it happen often? on what occasion? Yes, if for instance a page uses google served jQuery, and so does an iframe on the same page. https://codereview.chromium.org/12192005/diff/20012/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:248: char c ALLOW_UNUSED = buf->data()[i]; On 2013/02/11 13:59:25, pasko wrote: > It is hard to believe that this will be optimized out, and can be expensive. Is > it data prefetching then? Hard to guess. That is debugging code that made it into this upload. Thanks for catching it. I was trying to force core dumps in the posting thread: Ricardo's earlier comment about the buf needing to be owned was absolutely correct, and I still don't quite get why (the creator doesn't own it while waiting? I guess not!). I'll remove it. https://codereview.chromium.org/12192005/diff/20012/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_synchronous_entry.cc (right): https://codereview.chromium.org/12192005/diff/20012/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:175: base::Bind(callback, this, net::ERR_FAILED)); On 2013/02/11 13:59:25, pasko wrote: > I am wondering why some errors are logged and some others aren't. Should be > consistent or comment where we handle these errors above the stack. The rule is that there's logging in code that has required debugging work in the past, and not where there isn't. I like comments about where these errors are handled up the stack. I'll add some of those on my next upload. https://codereview.chromium.org/12192005/diff/20012/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:232: last_used_ = std::max(last_used_, file_info.last_accessed); On 2013/02/11 13:59:25, pasko wrote: > I am wondering how last_used is actually used. Here we probably want > file_info.last_accessed although it is unsurprisingly rather last_modified on > Linux and Android systems. Yeah. Tricky. Could force this issue with filesystem operations, or live with this (bad) compromise while we wait for the index to be alive. https://codereview.chromium.org/12192005/diff/20012/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:253: DVLOG(6) << "Error: No magic number."; On 2013/02/11 13:59:25, pasko wrote: > This is a pretty serious error, so sounds rather LOG(WARNING). Although probably > needs to be put once in the log to avoid severe pollution. I'll leave it LOG(WARNING) for now with a TODO. We're going to hit this a lot during development as we change the format. https://codereview.chromium.org/12192005/diff/20012/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:295: sizeof(header)) != sizeof(header)) { On 2013/02/11 13:59:25, pasko wrote: > would be helpful: LOG(WARNING) or explanation where we log about this Done.
+willchan for additional review (he's expressed an interest, and rvargas is out for a bit later this month)
https://codereview.chromium.org/12192005/diff/20012/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_backend_impl.cc (right): https://codereview.chromium.org/12192005/diff/20012/net/disk_cache/simple/sim... net/disk_cache/simple/simple_backend_impl.cc:48: EnsureCachePathExistsOnWorkerThread, On 2013/02/11 17:55:50, gavinp wrote: > On 2013/02/11 13:59:25, pasko wrote: > > nit: here and below adjust amount of leading space > > Previously I had lines like this with FROM_HERE on the next line, so the > indentation of the bind was cleaner. Ricardo nitted all those FROM_HERE lines up > to the line with the (. > > I'm happy to respond to your nit, but I don't know exactly what you're looking > for, and Ricardo's earlier suggestion puts me in a "bind" here. Maybe it'd be > clearest if I put the base::Bind() in a temp variable? Otherwise if you can give > me some instruction so clear even I understand it, I'll be happy. :) ouch sorry, this specific leading space is fine, I was referring to something rather like SimpleBackendImpl::CreateEntry(...), where you auto-renamed, but did not auto-format after that.
https://codereview.chromium.org/12192005/diff/20012/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_backend_impl.cc (right): https://codereview.chromium.org/12192005/diff/20012/net/disk_cache/simple/sim... net/disk_cache/simple/simple_backend_impl.cc:48: EnsureCachePathExistsOnWorkerThread, On 2013/02/11 18:13:44, pasko wrote: > On 2013/02/11 17:55:50, gavinp wrote: > > On 2013/02/11 13:59:25, pasko wrote: > > > nit: here and below adjust amount of leading space > > > > Previously I had lines like this with FROM_HERE on the next line, so the > > indentation of the bind was cleaner. Ricardo nitted all those FROM_HERE lines > up > > to the line with the (. > > > > I'm happy to respond to your nit, but I don't know exactly what you're looking > > for, and Ricardo's earlier suggestion puts me in a "bind" here. Maybe it'd be > > clearest if I put the base::Bind() in a temp variable? Otherwise if you can > give > > me some instruction so clear even I understand it, I'll be happy. :) > > ouch sorry, this specific leading space is fine, I was referring to something > rather like SimpleBackendImpl::CreateEntry(...), where you auto-renamed, but did > not auto-format after that. Aha! Good catch. Fixed.
Overall looks good! Only minor details. https://chromiumcodereview.appspot.com/12192005/diff/33004/net/disk_cache/bac... File net/disk_cache/backend_impl.cc (right): https://chromiumcodereview.appspot.com/12192005/diff/33004/net/disk_cache/bac... net/disk_cache/backend_impl.cc:274: #if defined(USE_SIMPLE_BACKEND) SIMPLE_BACKEND is too generic when defining in the GYP files or in the build command line. Maybe USE_SIMPLE_CACHE_BACKEND or just USE_SIMPLE_CACHE ? Not a big deal though. You can keep it if you want. https://chromiumcodereview.appspot.com/12192005/diff/33004/net/disk_cache/sim... File net/disk_cache/simple/simple_backend_impl.h (right): https://chromiumcodereview.appspot.com/12192005/diff/33004/net/disk_cache/sim... net/disk_cache/simple/simple_backend_impl.h:68: static void EnsureCachePathExistsOnWorkerThread( No need to have "OnWorkerThread" on the name of the function. If you want you can add a comment such as "May run on a worker thread." https://chromiumcodereview.appspot.com/12192005/diff/33004/net/disk_cache/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://chromiumcodereview.appspot.com/12192005/diff/33004/net/disk_cache/sim... net/disk_cache/simple/simple_entry_impl.cc:38: class SimpleEntryImpl::PendingCreationOperation { As discussed by chat, we don't really need a class and an object holding the state of PendingCreationOperation since the state is pretty simple (just a pointer to the Entry) and the base::Bind() method already provide enough framework for handling this. So, instead, you can just use a simple static method, in this case it will be the OnIOComplete, and pass it on the callback bind. https://chromiumcodereview.appspot.com/12192005/diff/33004/net/disk_cache/sim... net/disk_cache/simple/simple_entry_impl.cc:57: class SimpleEntryImpl::PendingEntryOperation { ditto. https://chromiumcodereview.appspot.com/12192005/diff/33004/net/disk_cache/sim... File net/disk_cache/simple/simple_entry_impl.h (right): https://chromiumcodereview.appspot.com/12192005/diff/33004/net/disk_cache/sim... net/disk_cache/simple/simple_entry_impl.h:87: // operations on the |synchronous_entry_| to the worker pool. This insures s/insures/ensures/
https://chromiumcodereview.appspot.com/12192005/diff/21009/net/disk_cache/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://chromiumcodereview.appspot.com/12192005/diff/21009/net/disk_cache/sim... net/disk_cache/simple/simple_entry_impl.cc:177: if (!synchronous_entry_) { as discussed, instead of checking the synchronous_entry_ pointer, you should add a bool IsOperationPending() to check if there is an operation going on. The bool has the same semantics and the intention is clearer. Also the pointer assignment and check doesn't add more thread safety than just a bool.
Thanks for the review Felipe. I've remediated to your comments. Ricardo: *ping* https://chromiumcodereview.appspot.com/12192005/diff/33004/net/disk_cache/bac... File net/disk_cache/backend_impl.cc (right): https://chromiumcodereview.appspot.com/12192005/diff/33004/net/disk_cache/bac... net/disk_cache/backend_impl.cc:274: #if defined(USE_SIMPLE_BACKEND) On 2013/02/12 13:25:20, felipeg1 wrote: > SIMPLE_BACKEND is too generic when defining in the GYP files or in the build > command line. > Maybe USE_SIMPLE_CACHE_BACKEND > or just USE_SIMPLE_CACHE ? > > Not a big deal though. You can keep it if you want. Done. https://chromiumcodereview.appspot.com/12192005/diff/33004/net/disk_cache/sim... File net/disk_cache/simple/simple_backend_impl.h (right): https://chromiumcodereview.appspot.com/12192005/diff/33004/net/disk_cache/sim... net/disk_cache/simple/simple_backend_impl.h:68: static void EnsureCachePathExistsOnWorkerThread( On 2013/02/12 13:25:20, felipeg1 wrote: > No need to have "OnWorkerThread" on the name of the function. > If you want you can add a comment such as "May run on a worker thread." Done. I added documentation to both methods. https://chromiumcodereview.appspot.com/12192005/diff/33004/net/disk_cache/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://chromiumcodereview.appspot.com/12192005/diff/33004/net/disk_cache/sim... net/disk_cache/simple/simple_entry_impl.cc:38: class SimpleEntryImpl::PendingCreationOperation { On 2013/02/12 13:25:20, felipeg1 wrote: > As discussed by chat, we don't really need a class and an object holding the > state of PendingCreationOperation since the state is pretty simple (just a > pointer to the Entry) and the base::Bind() method already provide enough > framework for handling this. > So, instead, you can just use a simple static method, in this case it will be > the OnIOComplete, and pass it on the callback bind. Great suggestion. I was being too ornate. https://chromiumcodereview.appspot.com/12192005/diff/33004/net/disk_cache/sim... net/disk_cache/simple/simple_entry_impl.cc:57: class SimpleEntryImpl::PendingEntryOperation { On 2013/02/12 13:25:20, felipeg1 wrote: > ditto. ditto! https://chromiumcodereview.appspot.com/12192005/diff/33004/net/disk_cache/sim... File net/disk_cache/simple/simple_entry_impl.h (right): https://chromiumcodereview.appspot.com/12192005/diff/33004/net/disk_cache/sim... net/disk_cache/simple/simple_entry_impl.h:87: // operations on the |synchronous_entry_| to the worker pool. This insures On 2013/02/12 13:25:20, felipeg1 wrote: > s/insures/ensures/ Done. https://chromiumcodereview.appspot.com/12192005/diff/21009/net/disk_cache/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://chromiumcodereview.appspot.com/12192005/diff/21009/net/disk_cache/sim... net/disk_cache/simple/simple_entry_impl.cc:177: if (!synchronous_entry_) { On 2013/02/12 14:50:22, felipeg1 wrote: > as discussed, > instead of checking the synchronous_entry_ pointer, you should add a bool > IsOperationPending() to check if there is an operation going on. > The bool has the same semantics and the intention is clearer. > Also the pointer assignment and check doesn't add more thread safety than just a > bool. Done.
lgtm
https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... File net/disk_cache/very_simple/very_simple_backend_impl.cc (right): https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_backend_impl.cc:76: DCHECK(thread_checker_.CalledOnValidThread()); On 2013/02/08 23:17:51, gavinp wrote: > On 2013/02/06 03:28:40, rvargas wrote: > > nit: seems overkill, considering that all methods are used on the same thread, > > and the default for chrome code is that classes are single-thread (so it would > > follow that all classes would have to do this) > > I agree it is overkill technically. I am including it for documentation > purposes. > > Reading the cache code for threading in the past few weeks, I've found myself > spending a lot of time tracing paths so I could be completely certain which > thread we were on at each point. > > With these DCHECKs, anyone reading the code has an easy reference they can > check. And it's far more reliable than a comment. If a future editor of this > code is as bad at remembering which method is used on which thread as I am, then > they will be happy these were here. I would agree with that if the class had a mixed thread use (last time I looked at it that was not the case, maybe that changed). If the class is not used from multiple threads, can't we just document the behavior with a comment on the header file? I'm seriously concerned with the logic saying that we should add a thread_checker to document the default coding convention of Chrome because that leads to most classes having a thread checker. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_backend_impl.cc:149: const FilePath& path) : path_(path) { On 2013/02/08 23:17:51, gavinp wrote: > On 2013/02/06 03:28:40, rvargas wrote: > > The general idea is that at this point contents of the cache are read and any > > access issue should be resolved/reported. > > Yes. That was true in the patch you were reviewing, and that's true in the new > upload too. What I meant is that the structure of SimpleBackendImpl::OnCachePathCreated() breaks as soon as you add the meat to this method, so it would be better to have the structure correct from the get go. But I'm OK with adding code that works today but not later on if that's what you want. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... File net/disk_cache/very_simple/very_simple_backend_impl.h (right): https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_backend_impl.h:16: #include "net/http/http_cache.h" On 2013/02/08 23:17:51, gavinp wrote: > On 2013/02/06 03:28:40, rvargas wrote: > > logical layering violation > > Done. Can you please tell us more about what HttpCache::BackendFactory is > intended for? It is intended for simplifying creation of the HttpCache. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... File net/disk_cache/very_simple/very_simple_entry_impl.cc (right): https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_entry_impl.cc:120: WorkerPool::PostTask(FROM_HERE, On 2013/02/08 23:17:51, gavinp wrote: > On 2013/02/06 03:28:40, rvargas wrote: > > You know that in general we don't like the worker pool for anything other than > > very basic, pure OS related stuff, right? > > I don't know this. Can you expand on this? That is documented on the header of worker pool. The basic reasoning is that the native worker pool has automatic thread management (which in general is a good thing), but it prevents us from controlling what happens with elements on the queue (and currently running) at shutdown. At such, we run into issues with anything else than very simple tasks. That's the reason we developed our own thread pool infrastructure. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_entry_impl.cc:158: Bind(&VerySimpleSynchronousEntry::DoomAndClose, On 2013/02/08 23:17:51, gavinp wrote: > On 2013/02/06 03:28:40, rvargas wrote: > > DoomAndClose? > > DoomAndClose! I meant it was surprising behavior. As far as I can see it is also incorrect in that an entry should remain 100% functional after doom. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_entry_impl.cc:214: offset, Unretained(buf), buf_len, On 2013/02/08 23:17:51, gavinp wrote: > On 2013/02/06 03:28:40, rvargas wrote: > > cannot be unretained > > Tell me more? that would mean keeping the buffer without grabbing a reference to it. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_entry_impl.cc:282: key_(synchronous_entry->key()), On 2013/02/08 23:17:51, gavinp wrote: > On 2013/02/06 03:28:40, rvargas wrote: > > why duplicate the key? > > Thread safe access. In practice, if SimpleSynchronousEntry::key_ was const, we > could retain a pointer to it and use that across threads safely. However, the > copy was conceptually simpler for me to deal with and didn't require that > argument. > > Do you have another suggestion? The code is happy to reach into access times and sizes(from the synchronous entry) and the key is not much different. https://codereview.chromium.org/12192005/diff/9004/net/net.gyp File net/net.gyp (right): https://codereview.chromium.org/12192005/diff/9004/net/net.gyp#newcode1046 net/net.gyp:1046: 'sources!': [ On 2013/02/08 23:27:21, gavinp wrote: > On 2013/02/06 03:28:40, rvargas wrote: > > why remove these files? > > I copied the pattern from what our custom DNS resolver does. I still think this is not the right thing to do... unless you have a strong reason to avoid compilation of the code by the bots. For example, the flash backend files compile all the time (and the unit tests run) https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:17: remove extra line https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:119: NOTREACHED(); nit: sounds like this should be removed (not a code invariant of this module) https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:164: index, offset, scoped_refptr<IOBuffer>(buf), make_scoped_refptr (if you want to force a scoped variable) https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:255: DCHECK(!synchronous_entry_) << "synchronous_entry_ = " << synchronous_entry_; In general I would not bother to add more logging to a dcheck... knowing the value doesn't add that much info anyway, and it is always possible to add specific logging to debug a hard to track issue if that ever happens. If you're wondering about the downside, that would be increased binary size. https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:281: SimpleSynchronousEntry* sync_entry, is it really worth passing sync_entry back just to have a dcheck with it? https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.h (right): https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.h:98: base::ThreadChecker thread_checker_; Doesn't the class description makes it explicit that this class is confined to a single thread (the IO thread)? https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.h:108: // threads to access the |synchronous_entry_| simultaniously. typo: simultaneously https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_synchronous_entry.cc (right): https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:50: return file_size - key_size - sizeof(disk_cache::SimpleFileHeader); There's a size mismatch here that should be handled (and signed vs unsigned). I would suggest going for 32 bit ints. https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:53: int64 FileOffsetFromDataOffset(size_t key_size, int offset) { Do you expect to have large files? https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:78: sync_entry ? net::OK : net::ERR_FAILED)); Sounds simpler (on both sides) to remove the last argument (error code). https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:109: DCHECK(delete_result); should not dcheck something coming from the OS https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:141: if (status_[index].mode != EntryStatus::ENTRY_READER) What's the purpose of this? https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:202: const std::string& key) : callback_runner_(callback_runner), nit: initialization list on the next line https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:216: flags |= PLATFORM_FILE_CREATE_ALWAYS; overwrite would be an error https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:258: << header.initial_magic_number << " expecting " nit: consider removing the saw vs expected parts. (one is a compile time constant, the other... well, it doesn't match) https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:263: LOG(INFO) << "Unreadable version. Got " << header.version nit: remove the variable parts?. Shouldn't this be at the same level of the previous log? https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:269: DCHECK_LE(header.key_length, sizeof(key)); this doesn't work (in general). https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:271: implicit_cast<int>(header.key_length)) { why is key_length not an int? https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:272: LOG(ERROR) << "Cannot read key from entry."; nit: I would expect all failures from this method to be at the same level, because the consequence of the failure is the same. https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:277: static_cast<const void*>(key), shouldn't need the casts here https://codereview.chromium.org/12192005/diff/26024/net/net.gyp File net/net.gyp (right): https://codereview.chromium.org/12192005/diff/26024/net/net.gyp#newcode56 net/net.gyp:56: '../third_party/smhasher/smhasher.gyp:murmurhash3', stale
looking at Ricardo's comments, I've got one extra fly by for you https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_synchronous_entry.cc (right): https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:221: if (error != PLATFORM_FILE_OK) { please log the error here
PTAL now! Thanks. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... File net/disk_cache/very_simple/very_simple_backend_impl.cc (right): https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_backend_impl.cc:76: DCHECK(thread_checker_.CalledOnValidThread()); On 2013/02/13 01:48:46, rvargas wrote: > On 2013/02/08 23:17:51, gavinp wrote: > > On 2013/02/06 03:28:40, rvargas wrote: > > > nit: seems overkill, considering that all methods are used on the same > thread, > > > and the default for chrome code is that classes are single-thread (so it > would > > > follow that all classes would have to do this) > > > > I agree it is overkill technically. I am including it for documentation > > purposes. > > > > Reading the cache code for threading in the past few weeks, I've found myself > > spending a lot of time tracing paths so I could be completely certain which > > thread we were on at each point. > > > > With these DCHECKs, anyone reading the code has an easy reference they can > > check. And it's far more reliable than a comment. If a future editor of this > > code is as bad at remembering which method is used on which thread as I am, > then > > they will be happy these were here. > > I would agree with that if the class had a mixed thread use (last time I looked > at it that was not the case, maybe that changed). If the class is not used from > multiple threads, can't we just document the behavior with a comment on the > header file? > > I'm seriously concerned with the logic saying that we should add a > thread_checker to document the default coding convention of Chrome because that > leads to most classes having a thread checker. Done. https://codereview.chromium.org/12192005/diff/9004/net/disk_cache/very_simple... net/disk_cache/very_simple/very_simple_backend_impl.cc:149: const FilePath& path) : path_(path) { On 2013/02/13 01:48:46, rvargas wrote: > On 2013/02/08 23:17:51, gavinp wrote: > > On 2013/02/06 03:28:40, rvargas wrote: > > > The general idea is that at this point contents of the cache are read and > any > > > access issue should be resolved/reported. > > > > Yes. That was true in the patch you were reviewing, and that's true in the new > > upload too. > > What I meant is that the structure of SimpleBackendImpl::OnCachePathCreated() > breaks as soon as you add the meat to this method, so it would be better to have > the structure correct from the get go. But I'm OK with adding code that works > today but not later on if that's what you want. Yeah. Currently Egor plans to chase this and get it right. https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:17: On 2013/02/13 01:48:46, rvargas wrote: > remove extra line Done. https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:119: NOTREACHED(); On 2013/02/13 01:48:46, rvargas wrote: > nit: sounds like this should be removed (not a code invariant of this module) Done. I replaced them with CHECK(false), which is more correct for our NOTIMPLEMENTED() cases, and doesn't carry this confusing other meaning that NOTREACHED() does. https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:164: index, offset, scoped_refptr<IOBuffer>(buf), On 2013/02/13 01:48:46, rvargas wrote: > make_scoped_refptr (if you want to force a scoped variable) Done. https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:255: DCHECK(!synchronous_entry_) << "synchronous_entry_ = " << synchronous_entry_; On 2013/02/13 01:48:46, rvargas wrote: > In general I would not bother to add more logging to a dcheck... knowing the > value doesn't add that much info anyway, and it is always possible to add > specific logging to debug a hard to track issue if that ever happens. > > If you're wondering about the downside, that would be increased binary size. Done. https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:281: SimpleSynchronousEntry* sync_entry, On 2013/02/13 01:48:46, rvargas wrote: > is it really worth passing sync_entry back just to have a dcheck with it? No. Removed. https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.h (right): https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.h:98: base::ThreadChecker thread_checker_; On 2013/02/13 01:48:46, rvargas wrote: > Doesn't the class description makes it explicit that this class is confined to a > single thread (the IO thread)? Done. https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.h:108: // threads to access the |synchronous_entry_| simultaniously. On 2013/02/13 01:48:46, rvargas wrote: > typo: simultaneously Done. https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_synchronous_entry.cc (right): https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:50: return file_size - key_size - sizeof(disk_cache::SimpleFileHeader); On 2013/02/13 01:48:46, rvargas wrote: > There's a size mismatch here that should be handled (and signed vs unsigned). I > would suggest going for 32 bit ints. Done. I used std::numeric_limits<> to guard the cast. It's not the most common Chrome style, but it's in the standard and I very much like the consistant interface. I'm open to any other suggestions. https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:53: int64 FileOffsetFromDataOffset(size_t key_size, int offset) { On 2013/02/13 01:48:46, rvargas wrote: > Do you expect to have large files? No. https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:78: sync_entry ? net::OK : net::ERR_FAILED)); On 2013/02/13 01:48:46, rvargas wrote: > Sounds simpler (on both sides) to remove the last argument (error code). Done. https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:109: DCHECK(delete_result); On 2013/02/13 01:48:46, rvargas wrote: > should not dcheck something coming from the OS Done. https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:141: if (status_[index].mode != EntryStatus::ENTRY_READER) On 2013/02/13 01:48:46, rvargas wrote: > What's the purpose of this? Removed. https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:202: const std::string& key) : callback_runner_(callback_runner), On 2013/02/13 01:48:46, rvargas wrote: > nit: initialization list on the next line Done. https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:216: flags |= PLATFORM_FILE_CREATE_ALWAYS; On 2013/02/13 01:48:46, rvargas wrote: > overwrite would be an error Done. https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:221: if (error != PLATFORM_FILE_OK) { On 2013/02/13 11:36:24, pasko wrote: > please log the error here Done. https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:258: << header.initial_magic_number << " expecting " On 2013/02/13 01:48:46, rvargas wrote: > nit: consider removing the saw vs expected parts. (one is a compile time > constant, the other... well, it doesn't match) Done. https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:263: LOG(INFO) << "Unreadable version. Got " << header.version On 2013/02/13 01:48:46, rvargas wrote: > nit: remove the variable parts?. > Shouldn't this be at the same level of the previous log? Done. https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:269: DCHECK_LE(header.key_length, sizeof(key)); On 2013/02/13 01:48:46, rvargas wrote: > this doesn't work (in general). Now replaced with the thing that works in general. https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:271: implicit_cast<int>(header.key_length)) { On 2013/02/13 01:48:46, rvargas wrote: > why is key_length not an int? Because there are no negative length keys. https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:272: LOG(ERROR) << "Cannot read key from entry."; On 2013/02/13 01:48:46, rvargas wrote: > nit: I would expect all failures from this method to be at the same level, > because the consequence of the failure is the same. Done. https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:277: static_cast<const void*>(key), On 2013/02/13 01:48:46, rvargas wrote: > shouldn't need the casts here Done. https://codereview.chromium.org/12192005/diff/26024/net/net.gyp File net/net.gyp (right): https://codereview.chromium.org/12192005/diff/26024/net/net.gyp#newcode56 net/net.gyp:56: '../third_party/smhasher/smhasher.gyp:murmurhash3', On 2013/02/13 01:48:46, rvargas wrote: > stale Done.
https://codereview.chromium.org/12192005/diff/45002/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_synchronous_entry.cc (right): https://codereview.chromium.org/12192005/diff/45002/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:260: scoped_ptr<char[]> key(new char[header.key_length]); I don't agree with Ricardo. Having a code like this: static const int kMaxURLLenght = 4096; char key[kMaxURLLenght]; Is much simpler than the scoped_ptr version and is also faster, with no memory allocation or memset() calls. Also, it do works in general. See: http://stackoverflow.com/questions/417142/what-is-the-maximum-length-of-a-url Anyway, the scoped_ptr also works. Your call.
Rev 21 lgtm https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_synchronous_entry.cc (right): https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:53: int64 FileOffsetFromDataOffset(size_t key_size, int offset) { On 2013/02/14 15:29:55, gavinp wrote: > On 2013/02/13 01:48:46, rvargas wrote: > > Do you expect to have large files? > > No. My point is that then you can return the size that you expect from this code, and the caller code would still work as is. This makes it explicit that there is no support / expectation for huge files. https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:271: implicit_cast<int>(header.key_length)) { On 2013/02/14 15:29:55, gavinp wrote: > On 2013/02/13 01:48:46, rvargas wrote: > > why is key_length not an int? > > Because there are no negative length keys. That's not the principle that determines signed vs unsigned on the net code (or in Chrome/Google). Casts and signed/unsigned mismatches seem worse than gaining one extra bit of resolution. https://codereview.chromium.org/12192005/diff/45002/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_synchronous_entry.cc (right): https://codereview.chromium.org/12192005/diff/45002/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:260: scoped_ptr<char[]> key(new char[header.key_length]); On 2013/02/14 15:51:59, felipeg1 wrote: > I don't agree with Ricardo. > > Having a code like this: > > static const int kMaxURLLenght = 4096; > char key[kMaxURLLenght]; > > Is much simpler than the scoped_ptr version and is also faster, with no memory > allocation or memset() calls. > > Also, it do works in general. > See: > http://stackoverflow.com/questions/417142/what-is-the-maximum-length-of-a-url see DiskCache.0.KeySize on the histogram dashboard. Never trust user controlled data. > > > Anyway, the scoped_ptr also works. > Your call. https://codereview.chromium.org/12192005/diff/48003/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_synchronous_entry.cc (right): https://codereview.chromium.org/12192005/diff/48003/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:221: DLOG(ERROR) << "Could not get platform file info."; nit: same level as other failures of this code (in fact, could be even lower because in this case the function doesn't even fail.
Thanks! I've uploaded a "clear to land" version for CQ. The rest of the downstream LGTM issues I'll push through as fast as I can. https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_synchronous_entry.cc (right): https://codereview.chromium.org/12192005/diff/26024/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:271: implicit_cast<int>(header.key_length)) { On 2013/02/14 21:09:30, rvargas wrote: > On 2013/02/14 15:29:55, gavinp wrote: > > On 2013/02/13 01:48:46, rvargas wrote: > > > why is key_length not an int? > > > > Because there are no negative length keys. > > That's not the principle that determines signed vs unsigned on the net code (or > in Chrome/Google). Casts and signed/unsigned mismatches seem worse than gaining > one extra bit of resolution. Good point; I"d been assuming file formats were "different", and presuming the size_t exception covered this case. That's stretching it. I'll look very carefully at this in the next rev of any of the disk formats. https://codereview.chromium.org/12192005/diff/48003/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_synchronous_entry.cc (right): https://codereview.chromium.org/12192005/diff/48003/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:221: DLOG(ERROR) << "Could not get platform file info."; On 2013/02/14 21:09:31, rvargas wrote: > nit: same level as other failures of this code (in fact, could be even lower > because in this case the function doesn't even fail. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gavinp@chromium.org/12192005/51001
Sorry for I got bad news for ya. Compile failed with a clobber build on ios_rel_device. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
https://codereview.chromium.org/12192005/diff/48003/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_synchronous_entry.cc (right): https://codereview.chromium.org/12192005/diff/48003/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:205: DLOG(INFO) << "CreatePlatformFile error " << error << " while " this fires every time there is a cache miss, it is annoying to observe these errors when running tests, better be a DVLOG(8), closer to that. https://codereview.chromium.org/12192005/diff/48003/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:210: DLOG_IF(INFO, did_close) << "Could not close file " !did_close
Doh. Thanks! https://codereview.chromium.org/12192005/diff/48003/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_synchronous_entry.cc (right): https://codereview.chromium.org/12192005/diff/48003/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:205: DLOG(INFO) << "CreatePlatformFile error " << error << " while " On 2013/02/15 16:42:01, pasko wrote: > this fires every time there is a cache miss, it is annoying to observe these > errors when running tests, better be a DVLOG(8), closer to that. Done. https://codereview.chromium.org/12192005/diff/48003/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:210: DLOG_IF(INFO, did_close) << "Could not close file " On 2013/02/15 16:42:01, pasko wrote: > !did_close Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gavinp@chromium.org/12192005/58002
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
On 2013/02/15 17:54:19, I haz the power (commit-bot) wrote: > Sorry for I got bad news for ya. > Compile failed with a clobber build on linux_chromeos. > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... > Your code is likely broken or HEAD is junk. Please ensure your > code is not broken then alert the build sheriffs. > Look at the try server FAQ for more details. Looks like a bad trunk. I wanted to add a header to simple_synchronous_entry anyway, so I'll make an upload before +cq again.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gavinp@chromium.org/12192005/65001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gavinp@chromium.org/12192005/65001
Message was sent while issue was closed.
Change committed as 182845
Message was sent while issue was closed.
On 2013/02/15 22:08:30, I haz the power (commit-bot) wrote: > Change committed as 182845 This caused: http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%2032/buil...
Message was sent while issue was closed.
Probably just needed a GG_ULONGLONG() ...
Message was sent while issue was closed.
https://codereview.chromium.org/12192005/diff/45002/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_synchronous_entry.cc (right): https://codereview.chromium.org/12192005/diff/45002/net/disk_cache/simple/sim... net/disk_cache/simple/simple_synchronous_entry.cc:260: scoped_ptr<char[]> key(new char[header.key_length]); On 2013/02/14 21:09:30, rvargas wrote: > On 2013/02/14 15:51:59, felipeg1 wrote: > > I don't agree with Ricardo. > > > > Having a code like this: > > > > static const int kMaxURLLenght = 4096; > > char key[kMaxURLLenght]; > > > > Is much simpler than the scoped_ptr version and is also faster, with no memory > > allocation or memset() calls. > > > > Also, it do works in general. > > See: > > http://stackoverflow.com/questions/417142/what-is-the-maximum-length-of-a-url > > > see DiskCache.0.KeySize on the histogram dashboard. > > Never trust user controlled data. Sure, but resource URLs that are bigger than 2K should not be valid anyway. We are not required to support it, although we must not break. > > > > > > > Anyway, the scoped_ptr also works. > > Your call. >
Message was sent while issue was closed.
Committed patchset #26 manually as r184493 (presubmit successful). |
