Disk cache: Introduce BlockBitmaps for V3.

net/disk_cache/block_files.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/disk_cache/block_files.h"
 
 #include "base/atomicops.h"
 #include "base/files/file_path.h"
 #include "base/metrics/histogram.h"
 #include "base/strings/string_util.h"
@@ skipping 159 matching lines @@
     uint32 map_block = header_->allocation_map[i];
 
     for (int j = 0; j < 8; j++, map_block >>= 4) {
       int type = GetMapBlockType(map_block);
       if (type)
         header_->empty[type -1]++;
     }
   }
 }
 
-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++) {
     empty_blocks += header_->empty[i] * (i + 1);
     if (i >= block_count - 1 && header_->empty[i])
       have_space = true;
   }
 
   if (header_->next_file && (empty_blocks < kMaxBlocks / 10)) {
     // This file is almost full but we already created another one, don't use
     // this file yet so that it is easier to find empty blocks when we start
     // using this file again.
     return true;
   }
   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++) {
     empty_blocks += header_->empty[i] * (i + 1);
     if (header_->empty[i] < 0)
       return 0;
   }
   return empty_blocks;
 }
 
@@ skipping 45 matching lines @@
     if (!RemoveEmptyFile(static_cast<FileType>(i + 1)))
       return false;
   }
 
   init_ = true;
   return true;
 }
 
 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;
 
   int file_index = address.FileNumber();
   if (static_cast<unsigned int>(file_index) >= block_files_.size() ||
       !block_files_[file_index]) {
     // We need to open the file
     if (!OpenBlockFile(file_index))
       return NULL;
   }
   DCHECK(block_files_.size() >= static_cast<unsigned int>(file_index));
   return block_files_[file_index];
 }
 
 bool BlockFiles::CreateBlock(FileType block_type, int block_count,

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

When is CreateBlock called for EXTERNAL, FILES, ENTRIES or EVICTED? Should that
be a DCHECK?

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

Done.

                              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;
 
   MappedFile* file = FileForNewBlock(block_type, block_count);
   if (!file)
     return false;
 
   ScopedFlush flush(file);
   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;
     }
   }
 
   DCHECK(target_size);
   int index;
   if (!header.CreateMapBlock(target_size, block_count, &index))
     return false;
@@ skipping 357 matching lines @@
   *load = 0;
   for (;;) {
     if (!block_files_[index] && !OpenBlockFile(index))
       return;
 
     BlockFileHeader* header =
         reinterpret_cast<BlockFileHeader*>(block_files_[index]->buffer());
 
     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);
     }
     *used_count += used;
 
     if (!header->next_file)
       break;
     index = header->next_file;
   }
   if (max_blocks)
     *load = *used_count * 100 / max_blocks;
 }
 
 base::FilePath BlockFiles::Name(int index) {
   // The file format allows for 256 files.
   DCHECK(index < 256 || index >= 0);
   std::string tmp = base::StringPrintf("%s%d", kBlockName, index);
   return path_.AppendASCII(tmp);
 }
 
 }  // namespace disk_cache