Simple Cache: DoomEntriesBetween()

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 c87b3c8a7019ca72061ae9e8d34686006ac6164b..996635a8253732afc93e14eac215a4305c3bf6d8 100644
--- a/net/disk_cache/simple/simple_index.cc
+++ b/net/disk_cache/simple/simple_index.cc
@@ -77,11 +77,16 @@ 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 = initialized_waitlist_.begin(),
+       end = initialized_waitlist_.end(); it != end; ++it)
+    it->Run(net::ERR_ABORTED);
 }
 
 void SimpleIndex::Initialize() {
@@ -96,6 +101,40 @@ 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
+    initialized_waitlist_.push_back(task);

[gavinp, 2013/04/17 07:41:18]

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.

[pasko-google - do not use, 2013/04/17 19:47:52]

May I please to it in a followup CL?

+  return net::ERR_IO_PENDING;
+}
+
+std::set<uint64>* SimpleIndex::PullEntriesBetween(

[felipeg, 2013/04/17 16:35:04]

I would call it RemoveEntriesBetween
or just RemoveBetween
to be consistent with Remove()

[pasko-google - do not use, 2013/04/17 19:47:52]

Followed Gavin's suggestion: ExtractEntriesBetween()

+    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);
+  std::set<uint64>* ret_hashes = new std::set<uint64>();
+  for (EntrySet::iterator it = entries_set_.begin(), end = entries_set_.end();

[gavinp, 2013/04/17 07:41:18]

Why take the copy of end?

[pasko-google - do not use, 2013/04/17 19:47:52]

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?

[gavinp, 2013/04/18 06:11:35]

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...

[pasko-google - do not use, 2013/04/18 09:29:54]

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.

+       it != end;) {
+    EntryMetadata metadata = it->second;
+    base::Time entry_time = metadata.GetLastUsedTime();
+    if (initial_time <= entry_time && entry_time < extended_end_time) {
+      ret_hashes->insert(metadata.GetHashKey());
+      entries_set_.erase(it++);
+    } else

[gavinp, 2013/04/17 07:41:18]

Style: } else {

[pasko-google - do not use, 2013/04/17 19:47:52]

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.

[gavinp, 2013/04/18 06:11:35]

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.

+      it++;
+  }
+  return ret_hashes;
+}
+
+int32 SimpleIndex::GetEntryCount() const {
+  DCHECK(initialized_);

[gavinp, 2013/04/17 07:41:18]

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.

[pasko-google - do not use, 2013/04/17 19:47:52]

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.
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

+  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.
@@ -272,6 +311,13 @@ void SimpleIndex::MergeInitializingSet(
   }
 
   initialized_ = true;
+
+  // Run all callbacks waiting for the index to come up.
+  for (CallbackList::iterator it = initialized_waitlist_.begin(),
+       end = initialized_waitlist_.end(); it != end; ++it) {
+    io_thread_->PostTask(FROM_HERE, base::Bind((*it), net::OK));
+  }
+  initialized_waitlist_.clear();
 }
 
 void SimpleIndex::WriteToDisk() {