Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14295013:
Simple Cache: DoomEntriesBetween() (Closed)
|
Created:
7 years, 8 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:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSimple Cache: DoomEntriesBetween()
The steps are:
* wait for the index to come up
* pull the key hashes from the index according to the time range
* go to the worker pool and delete all the corresponding entry files
BUG=230306
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194941
Patch Set 1 #
Total comments: 49
Patch Set 2 : first round of comments #
Total comments: 8
Patch Set 3 : more comments addressed #Patch Set 4 : vector #
Total comments: 2
Patch Set 5 : RemoveEntriesBetween() and rebase #Patch Set 6 : more rebases #Patch Set 7 : even more rebasing #Patch Set 8 : fix an actual bug in DoomEntry #
Messages
Total messages: 21 (0 generated)
I wrote all these nits before I decided to raise these architectural concerns;
so please don't pay much attention to the nits and instead focus on these two
questions:
1) What's the right thing to do if we haven't run GC yet? I think we may need to
force GC in this situation. GC isn't landed yet, but this deserves a big TODO
and some thought about correctness.
2) Could we eliminate the queue in the Index by instead having an IndexClient:
class IndexClient {
public:
virtual void OnIndexIsReady() = 0;
};
The Backend could implement this, and pass it to the Index on construction?
We'll still need a queue, but it's entirely in the Backend now.
What do you think?
https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_...
File net/disk_cache/simple/simple_backend_impl.cc (right):
https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_...
net/disk_cache/simple/simple_backend_impl.cc:75: return index_->GetEntryCount();
Nice. Can we perhaps do better still though: isn't the directory entry count
useful too, especially if we don't have an index?
(Perhaps another CL)
https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_...
net/disk_cache/simple/simple_backend_impl.cc:153: index_->UseIfExists(key);
Nice!
https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_...
File net/disk_cache/simple/simple_backend_impl.h (right):
https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_...
net/disk_cache/simple/simple_backend_impl.h:35: public
base::SupportsWeakPtr<SimpleBackendImpl> {
Is this indented correctly?
https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_...
net/disk_cache/simple/simple_backend_impl.h:77: void IndexReadyForDoom(const
base::Time initial_time,
this const has no meaning. I wouldn't pass a base::Time by reference (but a
grepfight shows that's definitely done), so might as well just use base::Time
here.
https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_...
File net/disk_cache/simple/simple_entry_impl.cc (right):
https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_...
net/disk_cache/simple/simple_entry_impl.cc:37: int
SimpleEntryImpl::OpenEntry(WeakPtr<SimpleIndex> index,
Do we need this parameter now? I think leave it alone though, since this has a
big conflict with a CL I'm working on (add refcounting).
https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_...
File net/disk_cache/simple/simple_index.cc (right):
https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_...
net/disk_cache/simple/simple_index.cc:109:
initialized_waitlist_.push_back(task);
I think we probably want UMA on the size of this list. It shouldn't be big,
right? I'm scared that we'll do something weird like fire off 15 async
operations immediately on index startup.
https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_...
net/disk_cache/simple/simple_index.cc:120: for (EntrySet::iterator it =
entries_set_.begin(), end = entries_set_.end();
Why take the copy of end?
https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_...
net/disk_cache/simple/simple_index.cc:127: } else
Style: } else {
https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_...
net/disk_cache/simple/simple_index.cc:134: DCHECK(initialized_);
This is going to make us crash in debug builds a lot if you browse shortly after
startup... I commented on this in the backend. I think it's better to return 0
than DCHECK in this case.
Better still, perhaps backend initialization can take the dir entry count and
use that until it's initialized?
Better still, dead reckoning using the dirent count until initialization.
https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_...
File net/disk_cache/simple/simple_index.h (right):
https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_...
net/disk_cache/simple/simple_index.h:9: #include <map>
While we're here, I believe this isn't needed any more.
https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_...
net/disk_cache/simple/simple_index.h:115: std::set<uint64>*
PullEntriesBetween(const base::Time initial_time,
Rather than referencing the definition in |DoomEntriesBetween()| maybe it's best
to just repeat it, in case of change?
Naming suggestions: ExtractEntriesBetween ? RemoveAndReturnEntriesBetween ?
Hrrm. I am not a fan of PullEntriesBetween.
https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_...
net/disk_cache/simple/simple_index.h:118: // Returns amount of indexed entries.
// Returns number of indexed entries.
https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_...
net/disk_cache/simple/simple_index.h:156: typedef
std::list<net::CompletionCallback> CallbackList;
Take it or leave it suggestion: lose this typedef.
https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_...
net/disk_cache/simple/simple_index.h:157: CallbackList initialized_waitlist_;
Naming suggestion: to_run_when_initialized_ (with or without list).
https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_...
File net/disk_cache/simple/simple_synchronous_entry.cc (right):
https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_...
net/disk_cache/simple/simple_synchronous_entry.cc:336: DoomEntry(path_, key_,
Should this call DeleteFilesForEntry now?
https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_...
File net/disk_cache/simple/simple_synchronous_entry.h (right):
https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_...
net/disk_cache/simple/simple_synchronous_entry.h:13: #include
"base/memory/ref_counted.h"
Need scoped_ptr.h too.
https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_...
net/disk_cache/simple/simple_synchronous_entry.h:67: // Like |DoomEntry()|
above. Deletes entry files corresponding to the set of
// Like |DoomEntry()| above. Deletes all entries corresponding to the
|key_hashes|. Succeeds only when all entries are deleted.
https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_...
File net/disk_cache/simple/simple_util.h (right):
https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_...
net/disk_cache/simple/simple_util.h:41: // Same as |GetFilenameFromKeyAndIndex|
above, but using a ready hex string.
I think "ready" doesn't work here.
https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_...
net/disk_cache/simple/simple_util.h:45: // Given the size of a file holding a
stream in the simple backend and the size
Eek: many of these comments are wrong now that these functions take a key rather
than the size of a key. While you're here, care to fix it?
I can't seem to use the UI to start try jobs. Can you please run a good selection of try jobs? It's especially vital today given the CQ, but even without that, try jobs run more tests. Note that if you launch from the command line, you can specify a base revision (if you depend on things ahead of LKGR) and you can specify a base for diffs other than your immediate upstream (in case you're working on a CL well downstream of trunk).
https://chromiumcodereview.appspot.com/14295013/diff/1/net/disk_cache/simple/... File net/disk_cache/simple/simple_backend_impl.cc (right): https://chromiumcodereview.appspot.com/14295013/diff/1/net/disk_cache/simple/... net/disk_cache/simple/simple_backend_impl.cc:75: return index_->GetEntryCount(); On 2013/04/17 07:41:18, gavinp wrote: > Nice. Can we perhaps do better still though: isn't the directory entry count > useful too, especially if we don't have an index? > > (Perhaps another CL) In another CL, not this one, please. Also, I could not find an elegant/fast way of getting the file count in a directory in unix. man 2 stat doesnt have that. https://chromiumcodereview.appspot.com/14295013/diff/1/net/disk_cache/simple/... File net/disk_cache/simple/simple_entry_impl.cc (left): https://chromiumcodereview.appspot.com/14295013/diff/1/net/disk_cache/simple/... net/disk_cache/simple/simple_entry_impl.cc:44: if (!index || index->UseIfExists(key)) { please keep this and use the method index->Has(key) instead https://chromiumcodereview.appspot.com/14295013/diff/1/net/disk_cache/simple/... File net/disk_cache/simple/simple_index.cc (right): https://chromiumcodereview.appspot.com/14295013/diff/1/net/disk_cache/simple/... net/disk_cache/simple/simple_index.cc:113: std::set<uint64>* SimpleIndex::PullEntriesBetween( I would call it RemoveEntriesBetween or just RemoveBetween to be consistent with Remove() https://chromiumcodereview.appspot.com/14295013/diff/1/net/disk_cache/simple/... File net/disk_cache/simple/simple_util.cc (right): https://chromiumcodereview.appspot.com/14295013/diff/1/net/disk_cache/simple/... net/disk_cache/simple/simple_util.cc:61: return GetEntryHashKeyAsHexString(key) + base::StringPrintf("_%1d", index); Reuse your newly added function
PTAL I think the new histogram and a test for stalled initialization should go in the next CLs. This change survived two rebases, one of which was not trivial. https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_... File net/disk_cache/simple/simple_backend_impl.cc (right): https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_... net/disk_cache/simple/simple_backend_impl.cc:75: return index_->GetEntryCount(); On 2013/04/17 16:35:04, felipeg1 wrote: > On 2013/04/17 07:41:18, gavinp wrote: > > Nice. Can we perhaps do better still though: isn't the directory entry count > > useful too, especially if we don't have an index? > > > > (Perhaps another CL) > In another CL, not this one, please. > > Also, I could not find an elegant/fast way of getting the file count in a > directory in unix. > man 2 stat doesnt have that. This should work pretty fast: DirReaderPosix reader(path_); while (reader.Next()) { count++; } Though it requires an ifdef. So I suggest do nothing of this kind unless we realize it is absolutely necessary. Cross the fingers and try to pass all tests. https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_... File net/disk_cache/simple/simple_backend_impl.h (right): https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_... net/disk_cache/simple/simple_backend_impl.h:35: public base::SupportsWeakPtr<SimpleBackendImpl> { On 2013/04/17 07:41:18, gavinp wrote: > Is this indented correctly? unspecified? I've indented it to 4 spaces, let me know if your preference is different. https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_... net/disk_cache/simple/simple_backend_impl.h:77: void IndexReadyForDoom(const base::Time initial_time, On 2013/04/17 07:41:18, gavinp wrote: > this const has no meaning. I wouldn't pass a base::Time by reference (but a > grepfight shows that's definitely done), so might as well just use base::Time > here. Sure, I just blindly copied it from somewhere. Done. https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_... File net/disk_cache/simple/simple_entry_impl.cc (left): https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_... net/disk_cache/simple/simple_entry_impl.cc:44: if (!index || index->UseIfExists(key)) { On 2013/04/17 16:35:04, felipeg1 wrote: > please keep this > and use the method index->Has(key) instead Oh yes, that's the whole purpose of the index: quickly answer if we do not have stuff. I inverted the condition since it looks nicer that way, also removed the TODO(gavinp) above since we have basically the same TODO(felipeg) below in a more relevant place. Done. This reminds me of stale index (not knowing about new entries added), that would fail the open, which may be not ideal for offline reading. Detecting a stale index is not trivial, so I would not even add a TODO for adding a histogram here. Thanks, now I have a creepy feeling that is not easy to fix. https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_... net/disk_cache/simple/simple_entry_impl.cc:37: int SimpleEntryImpl::OpenEntry(WeakPtr<SimpleIndex> index, On 2013/04/17 07:41:18, gavinp wrote: > Do we need this parameter now? I think leave it alone though, since this has a > big conflict with a CL I'm working on (add refcounting). As Felipe suggests in another comment we should look at the index here. https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_... File net/disk_cache/simple/simple_index.cc (right): https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_... net/disk_cache/simple/simple_index.cc:109: initialized_waitlist_.push_back(task); On 2013/04/17 07:41:18, gavinp wrote: > I think we probably want UMA on the size of this list. It shouldn't be big, > right? I'm scared that we'll do something weird like fire off 15 async > operations immediately on index startup. May I please to it in a followup CL? https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_... net/disk_cache/simple/simple_index.cc:113: std::set<uint64>* SimpleIndex::PullEntriesBetween( On 2013/04/17 16:35:04, felipeg1 wrote: > I would call it RemoveEntriesBetween > or just RemoveBetween > to be consistent with Remove() Followed Gavin's suggestion: ExtractEntriesBetween() https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_... net/disk_cache/simple/simple_index.cc:120: for (EntrySet::iterator it = entries_set_.begin(), end = entries_set_.end(); On 2013/04/17 07:41:18, gavinp wrote: > Why take the copy of end? This is a premature optimization: in the hope that comparing to a copy is cheaper than computing end(). Which is probably of same complexity these days. Would you prefer to lift it? https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_... net/disk_cache/simple/simple_index.cc:127: } else On 2013/04/17 07:41:18, gavinp wrote: > Style: } else { that is good, I like that, did not know that the guide is on my side on this. I never liked omitting the braces, the style in net/ is pressing on me to do this. Done. https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_... net/disk_cache/simple/simple_index.cc:134: DCHECK(initialized_); On 2013/04/17 07:41:18, gavinp wrote: > This is going to make us crash in debug builds a lot if you browse shortly after > startup... I commented on this in the backend. I think it's better to return 0 > than DCHECK in this case. had the same thought very soon after uploading. Sadly we do not have macros to make a DCHECK only with tests, those would allow to make sure we always wait enough before doing entry count assertions. > Better still, perhaps backend initialization can take the dir entry count and > use that until it's initialized? That's a good idea, two ways to do this: 1. make the initialized_ publicly accessible (less preferable) 2. provide an initial count estimate on SimpleIndex::Initialize() which it should return until it gets initialized (more preferable) added a TODO for now https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_... File net/disk_cache/simple/simple_index.h (right): https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_... net/disk_cache/simple/simple_index.h:9: #include <map> On 2013/04/17 07:41:18, gavinp wrote: > While we're here, I believe this isn't needed any more. Done. https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_... net/disk_cache/simple/simple_index.h:115: std::set<uint64>* PullEntriesBetween(const base::Time initial_time, On 2013/04/17 07:41:18, gavinp wrote: > Rather than referencing the definition in |DoomEntriesBetween()| maybe it's best > to just repeat it, in case of change? That? Will change? C'mon, backend interface changes only for Christmas. I am not a fan of repetitions. > Naming suggestions: ExtractEntriesBetween ? RemoveAndReturnEntriesBetween ? > Hrrm. I am not a fan of PullEntriesBetween. Thanks, extract sounds OK, and my ear is not sophisticated enough to make the call. Done. https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_... net/disk_cache/simple/simple_index.h:118: // Returns amount of indexed entries. On 2013/04/17 07:41:18, gavinp wrote: > // Returns number of indexed entries. Done. https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_... net/disk_cache/simple/simple_index.h:156: typedef std::list<net::CompletionCallback> CallbackList; On 2013/04/17 07:41:18, gavinp wrote: > Take it or leave it suggestion: lose this typedef. Please remind me when we transition to C++11, I'd be glad to lose this then. https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_... net/disk_cache/simple/simple_index.h:157: CallbackList initialized_waitlist_; On 2013/04/17 07:41:18, gavinp wrote: > Naming suggestion: to_run_when_initialized_ (with or without list). OK, taking this. I liked how waitlist sounded, it was short .. and confusing. https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_... File net/disk_cache/simple/simple_synchronous_entry.cc (right): https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_... net/disk_cache/simple/simple_synchronous_entry.cc:336: DoomEntry(path_, key_, On 2013/04/17 07:41:18, gavinp wrote: > Should this call DeleteFilesForEntry now? Yes. Nice. Done. https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_... File net/disk_cache/simple/simple_synchronous_entry.h (right): https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_... net/disk_cache/simple/simple_synchronous_entry.h:13: #include "base/memory/ref_counted.h" On 2013/04/17 07:41:18, gavinp wrote: > Need scoped_ptr.h too. why? https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_... net/disk_cache/simple/simple_synchronous_entry.h:67: // Like |DoomEntry()| above. Deletes entry files corresponding to the set of On 2013/04/17 07:41:18, gavinp wrote: > // Like |DoomEntry()| above. Deletes all entries corresponding to the > |key_hashes|. Succeeds only when all entries are deleted. I wanted to stress the word 'files', but OK. Done. https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_... File net/disk_cache/simple/simple_util.cc (right): https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_... net/disk_cache/simple/simple_util.cc:61: return GetEntryHashKeyAsHexString(key) + base::StringPrintf("_%1d", index); On 2013/04/17 16:35:04, felipeg1 wrote: > Reuse your newly added function I thought about this too and then decided that this way it is more readable and is shorter by 1 line. Sorry. https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_... File net/disk_cache/simple/simple_util.h (right): https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_... net/disk_cache/simple/simple_util.h:41: // Same as |GetFilenameFromKeyAndIndex| above, but using a ready hex string. On 2013/04/17 07:41:18, gavinp wrote: > I think "ready" doesn't work here. Done. https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_... net/disk_cache/simple/simple_util.h:45: // Given the size of a file holding a stream in the simple backend and the size On 2013/04/17 07:41:18, gavinp wrote: > Eek: many of these comments are wrong now that these functions take a key rather > than the size of a key. While you're here, care to fix it? three is many, you are right. Done.
Looking great; we're down to nits. https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_... File net/disk_cache/simple/simple_index.cc (right): https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_... net/disk_cache/simple/simple_index.cc:120: for (EntrySet::iterator it = entries_set_.begin(), end = entries_set_.end(); On 2013/04/17 19:47:52, pasko wrote: > On 2013/04/17 07:41:18, gavinp wrote: > > Why take the copy of end? > > This is a premature optimization: in the hope that comparing to a copy is > cheaper than computing end(). Which is probably of same complexity these days. > > Would you prefer to lift it? It's fine, and plenty readable. I do think it's premature though. The standard requires that end() be constant time, and it's probably a very quick inline, likely easily proven idempotent. Copying and allocating an automatic is also constant time. It's easy to believe the only result of this optimization is a slightly more complex constant subexpression elimination phase in the compiler... https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_... net/disk_cache/simple/simple_index.cc:127: } else On 2013/04/17 19:47:52, pasko wrote: > On 2013/04/17 07:41:18, gavinp wrote: > > Style: } else { > > that is good, I like that, did not know that the guide is on my side on this. I > never liked omitting the braces, the style in net/ is pressing on me to do this. > > Done. Google style says braces are OK if you want them, but required on multi line if() statements. This is multi-line, so required. In Chromium, the bias is strongly against them in situations where they are optional. So, to use braces, just remember to give all your variables long names. https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_... File net/disk_cache/simple/simple_synchronous_entry.h (right): https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_... net/disk_cache/simple/simple_synchronous_entry.h:55: const scoped_refptr<base::TaskRunner>& callback_runner, This is why! https://codereview.chromium.org/14295013/diff/13001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_backend_impl.h (right): https://codereview.chromium.org/14295013/diff/13001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_backend_impl.h:76: void IndexReadyForDoom(base::Time initial_time, base::Time end_time, Chromium style says these should be on separate lines. https://codereview.chromium.org/14295013/diff/13001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_index.cc (right): https://codereview.chromium.org/14295013/diff/13001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_index.cc:88: end = to_run_when_initialized_.end(); it != end; ++it) This for loop needs braces. https://codereview.chromium.org/14295013/diff/13001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_index.cc:119: std::set<uint64>* ret_hashes = new std::set<uint64>(); Can we make ret_hashes a scoped_ptr? And it might be best to make the return type of ExtractEntriesBetween a scoped_ptr, too.
https://codereview.chromium.org/14295013/diff/13001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_backend_impl.h (right): https://codereview.chromium.org/14295013/diff/13001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_backend_impl.h:34: public base::SupportsWeakPtr<SimpleBackendImpl> { This requires some thought. The Index can be destructed on the worker pool, right? Perhaps not: as long as the Backend is alive, it cannot? Subtle.
Addressed comments, will also replace set with vector as Phillippe suggested. https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_... File net/disk_cache/simple/simple_index.cc (right): https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_... net/disk_cache/simple/simple_index.cc:120: for (EntrySet::iterator it = entries_set_.begin(), end = entries_set_.end(); On 2013/04/18 06:11:35, gavinp wrote: > On 2013/04/17 19:47:52, pasko wrote: > > On 2013/04/17 07:41:18, gavinp wrote: > > > Why take the copy of end? > > > > This is a premature optimization: in the hope that comparing to a copy is > > cheaper than computing end(). Which is probably of same complexity these days. > > > > Would you prefer to lift it? > > It's fine, and plenty readable. I do think it's premature though. The standard > requires that end() be constant time, and it's probably a very quick inline, > likely easily proven idempotent. Copying and allocating an automatic is also > constant time. It's easy to believe the only result of this optimization is a > slightly more complex constant subexpression elimination phase in the > compiler... you probably mean common subexpression elimination. I do not want to be inconsistent with local practices because readability matters more. The readability is better when if I do the most common thing. https://codereview.chromium.org/14295013/diff/13001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_backend_impl.h (right): https://codereview.chromium.org/14295013/diff/13001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_backend_impl.h:34: public base::SupportsWeakPtr<SimpleBackendImpl> { On 2013/04/18 08:16:06, gavinp wrote: > This requires some thought. The Index can be destructed on the worker pool, > right? > > Perhaps not: as long as the Backend is alive, it cannot? > > Subtle. It was written in the assumption that the index is always destroyed in the backend destructor, and now it is scoped_refptr. Anyway, per offline discussion: we can destroy the weak pointer on any thread, so destructing the callbacks (containing the weak pointer) in the queue of callbacks with the index destruction should be fine. https://codereview.chromium.org/14295013/diff/13001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_backend_impl.h:76: void IndexReadyForDoom(base::Time initial_time, base::Time end_time, On 2013/04/18 06:11:35, gavinp wrote: > Chromium style says these should be on separate lines. Argh. Done. https://codereview.chromium.org/14295013/diff/13001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_index.cc (right): https://codereview.chromium.org/14295013/diff/13001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_index.cc:88: end = to_run_when_initialized_.end(); it != end; ++it) On 2013/04/18 06:11:35, gavinp wrote: > This for loop needs braces. Not required though. And I had an impression that Chromium prefers not to put braces when it is not required. Apparently the rule is different. And not documented. Thank you, Chromium reviewers. It was so much easier to always put them, aaa letmealwaysputthebraces! Done. https://codereview.chromium.org/14295013/diff/13001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_index.cc:119: std::set<uint64>* ret_hashes = new std::set<uint64>(); On 2013/04/18 06:11:35, gavinp wrote: > Can we make ret_hashes a scoped_ptr? > > And it might be best to make the return type of ExtractEntriesBetween a > scoped_ptr, too. Sure, I understand it may feel more cozy. Done.
updated with vector, PTAL
LGTM.
https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_... File net/disk_cache/simple/simple_entry_impl.cc (left): https://codereview.chromium.org/14295013/diff/1/net/disk_cache/simple/simple_... net/disk_cache/simple/simple_entry_impl.cc:44: if (!index || index->UseIfExists(key)) { On 2013/04/17 19:47:52, pasko wrote: > On 2013/04/17 16:35:04, felipeg1 wrote: > > please keep this > > and use the method index->Has(key) instead > > Oh yes, that's the whole purpose of the index: quickly answer if we do not have > stuff. > > I inverted the condition since it looks nicer that way, also removed the > TODO(gavinp) above since we have basically the same TODO(felipeg) below in a > more relevant place. > > Done. > > This reminds me of stale index (not knowing about new entries added), that would > fail the open, which may be not ideal for offline reading. Detecting a stale > index is not trivial, so I would not even add a TODO for adding a histogram > here. Thanks, now I have a creepy feeling that is not easy to fix. A stale and valid index (with the correct file entries and CRC) should be really really rare. And our garbage collector will take care of getting rid of it from time to time. (I am working in the GC CL). And I strongly suggest we have a histogram here, we need numbers to justify having the index.
LGTM https://codereview.chromium.org/14295013/diff/32001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/14295013/diff/32001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:43: if (index && !index->Has(key)) This change is not related and is not necessary to your CL. I would suggest have that in a new CL, but I am ok with having it submitted here. Your call. LGTM
https://codereview.chromium.org/14295013/diff/32001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/14295013/diff/32001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:43: if (index && !index->Has(key)) On 2013/04/18 10:05:05, felipeg1 wrote: > This change is not related and is not necessary to your CL. > I would suggest have that in a new CL, but I am ok with having it submitted > here. thanks for review, dodged a bug here. Since I had to change this place anyway (to make tests pass), I decided to make a closely related side change in this CL, because in the other CL it would have required restoring the whole review context in your mind, which is suboptimal :)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pasko@google.com/14295013/40001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pasko@google.com/14295013/54001
Failed to apply patch for net/disk_cache/simple/simple_index.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file net/disk_cache/simple/simple_index.cc
Hunk #1 FAILED at 76.
Hunk #2 succeeded at 105 (offset 11 lines).
Hunk #3 FAILED at 305.
2 out of 3 hunks FAILED -- saving rejects to file
net/disk_cache/simple/simple_index.cc.rej
Patch: net/disk_cache/simple/simple_index.cc
Index: net/disk_cache/simple/simple_index.cc
diff --git a/net/disk_cache/simple/simple_index.cc
b/net/disk_cache/simple/simple_index.cc
index
2f5c6255d729d0d87db23dfc52824808cb556175..f3da57e3ae4a42e4da0129e621f2e383ec98ab2c
100644
--- a/net/disk_cache/simple/simple_index.cc
+++ b/net/disk_cache/simple/simple_index.cc
@@ -76,10 +76,17 @@ SimpleIndex::SimpleIndex(
initialized_(false),
index_filename_(path.AppendASCII("simple-index")),
cache_thread_(cache_thread),
- io_thread_(io_thread) {}
+ io_thread_(io_thread) {
+}
SimpleIndex::~SimpleIndex() {
DCHECK(io_thread_checker_.CalledOnValidThread());
+
+ // Fail all callbacks waiting for the index to come up.
+ for (CallbackList::iterator it = to_run_when_initialized_.begin(),
+ end = to_run_when_initialized_.end(); it != end; ++it) {
+ it->Run(net::ERR_ABORTED);
+ }
}
void SimpleIndex::Initialize() {
@@ -94,6 +101,41 @@ void SimpleIndex::Initialize() {
true);
}
+int SimpleIndex::ExecuteWhenReady(const net::CompletionCallback& task) {
+ DCHECK(io_thread_checker_.CalledOnValidThread());
+ if (initialized_)
+ io_thread_->PostTask(FROM_HERE, base::Bind(task, net::OK));
+ else
+ to_run_when_initialized_.push_back(task);
+ return net::ERR_IO_PENDING;
+}
+
+scoped_ptr<std::vector<uint64> > SimpleIndex::RemoveEntriesBetween(
+ const base::Time initial_time, const base::Time end_time) {
+ DCHECK_EQ(true, initialized_);
+ const base::Time extended_end_time =
+ end_time.is_null() ? base::Time::Max() : end_time;
+ DCHECK(extended_end_time >= initial_time);
+ scoped_ptr<std::vector<uint64> > ret_hashes(new std::vector<uint64>());
+ for (EntrySet::iterator it = entries_set_.begin(), end = entries_set_.end();
+ it != end;) {
+ EntryMetadata metadata = it->second;
+ base::Time entry_time = metadata.GetLastUsedTime();
+ if (initial_time <= entry_time && entry_time < extended_end_time) {
+ ret_hashes->push_back(metadata.GetHashKey());
+ entries_set_.erase(it++);
+ } else {
+ it++;
+ }
+ }
+ return ret_hashes.Pass();
+}
+
+int32 SimpleIndex::GetEntryCount() const {
+ // TODO(pasko): return a meaningful initial estimate before initialized.
+ return entries_set_.size();
+}
+
void SimpleIndex::Insert(const std::string& key) {
DCHECK(io_thread_checker_.CalledOnValidThread());
// Upon insert we don't know yet the size of the entry.
@@ -270,6 +312,13 @@ void SimpleIndex::MergeInitializingSet(
}
initialized_ = true;
+
+ // Run all callbacks waiting for the index to come up.
+ for (CallbackList::iterator it = to_run_when_initialized_.begin(),
+ end = to_run_when_initialized_.end(); it != end; ++it) {
+ io_thread_->PostTask(FROM_HERE, base::Bind((*it), net::OK));
+ }
+ to_run_when_initialized_.clear();
}
void SimpleIndex::WriteToDisk() {
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pasko@google.com/14295013/54004
Retried try job too often on ios_dbg_simulator for step(s) net_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pasko@google.com/14295013/61013
Message was sent while issue was closed.
Change committed as 194941 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
