Disk cache: Introduce BlockBitmaps for V3.

net/disk_cache/block_files.cc

Index: net/disk_cache/block_files.cc
===================================================================
--- net/disk_cache/block_files.cc	(revision 209202)
+++ net/disk_cache/block_files.cc	(working copy)
@@ -177,7 +177,7 @@
   }
 }
 
-bool BlockHeader::NeedToGrowBlockFile(int block_count) {
+bool BlockHeader::NeedToGrowBlockFile(int block_count) const {
   bool have_space = false;
   int empty_blocks = 0;
   for (int i = 0; i < kMaxNumBlocks; i++) {
@@ -195,6 +195,18 @@
   return !have_space;
 }
 
+bool BlockHeader::CanAllocate(int block_count) const {
+  bool have_space = false;
+  int empty_blocks = 0;

[gavinp, 2013/06/28 21:47:45]

What's empty_blocks for?

[rvargas (doing something else), 2013/07/11 19:54:55]

left overs... sorry about that.

+  for (int i = 0; i < kMaxNumBlocks; i++) {
+    empty_blocks += header_->empty[i] * (i + 1);
+    if (i >= (block_count - 1) && header_->empty[i])
+      have_space = true;

[gavinp, 2013/06/28 21:47:45]

Can we just return true here?

+  }
+
+  return have_space;

[gavinp, 2013/06/28 21:47:45]

And return false here?

+}
+
 int BlockHeader::EmptyBlocks() const {
   int empty_blocks = 0;
   for (int i = 0; i < disk_cache::kMaxNumBlocks; i++) {
@@ -260,7 +272,8 @@
 
 MappedFile* BlockFiles::GetFile(Addr address) {
   DCHECK(thread_checker_->CalledOnValidThread());
-  DCHECK(block_files_.size() >= 4);
+  DCHECK_GE(block_files_.size(),
+            static_cast<size_t>(kFirstAdditionalBlockFile));
   DCHECK(address.is_block_file() || !address.is_initialized());
   if (!address.is_initialized())
     return NULL;
@@ -280,8 +293,9 @@
                              Addr* block_address) {
   DCHECK(thread_checker_->CalledOnValidThread());
   if (block_type < RANKINGS || block_type > BLOCK_4K ||

[gavinp, 2013/06/28 21:47:45]

These inequalities are hard for me to read, the ordering of the enum isn't
really easy to keep paged in for someone studying this code.

Could we consider cleaning this up a bit while we're here? Perhaps:

if ((block_type != RANKINGS && block_type != BLOCK_256 && block_type != BLOCK_1K
&& block_type != BLOCK_4K) || ...

Which has the virtue of listing precisely the list of blocks that can do this.

[rvargas (doing something else), 2013/07/11 19:54:55]

Done.

-      block_count < 1 || block_count > 4)
+      block_count < 1 || block_count > kMaxNumBlocks) {
     return false;
+  }
   if (!init_)

[gavinp, 2013/06/28 21:47:45]

Nit: could just add this to the earlier test, and it would be a lot more
readable if it was the first condition tested.

[rvargas (doing something else), 2013/07/11 19:54:55]

In my mind the two checks do two different things: the first one is for being
asked to do something that doesn't make much sense, the second one is for being
accessed at an improper time. The typical answer to the former is in the order
of "invalid arguments", which tends to be more serious than using the object
correctly but at a bad time (and that's why that test goes first... not that it
really matters)

I find multiple conditions on an if harder to follow that multiple individual
statements.

     return false;
 
@@ -293,7 +307,7 @@
   BlockHeader header(file);
 
   int target_size = 0;
-  for (int i = block_count; i <= 4; i++) {
+  for (int i = block_count; i <= kMaxNumBlocks; i++) {
     if (header->empty[i - 1]) {
       target_size = i;
       break;
@@ -671,7 +685,7 @@
 
     max_blocks += header->max_entries;
     int used = header->max_entries;
-    for (int i = 0; i < 4; i++) {
+    for (int i = 0; i < kMaxNumBlocks; i++) {
       used -= header->empty[i] * (i + 1);
       DCHECK_GE(used, 0);
     }