Disk cache: Introduce BlockBitmaps for V3.

net/disk_cache/v3/block_bitmaps_unittest.cc

Index: net/disk_cache/v3/block_bitmaps_unittest.cc
===================================================================
--- net/disk_cache/v3/block_bitmaps_unittest.cc	(revision 208996)
+++ net/disk_cache/v3/block_bitmaps_unittest.cc	(working copy)
@@ -2,319 +2,40 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "base/file_util.h"
-#include "base/files/file_enumerator.h"
+#include "net/disk_cache/addr.h"
 #include "net/disk_cache/block_files.h"
-#include "net/disk_cache/disk_cache.h"
-#include "net/disk_cache/disk_cache_test_base.h"
-#include "net/disk_cache/disk_cache_test_util.h"
+#include "net/disk_cache/disk_format_base.h"
+#include "net/disk_cache/v3/block_bitmaps.h"
 #include "testing/gtest/include/gtest/gtest.h"
 

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

I think this whole file can be put in an anonymous namespace.

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

Tests usually don't go into namespaces.

[gavinp, 2013/07/15 18:23:35]

Yes, they do. For instance, take a peek at tcp_client_socket_unittest.cc,
https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/tcp_cli...


On a per file basis for all chrome:

    ~/chrome/src$ git grep -n -e '^[^ ]*TEST' -l -- '*test.cc' | wc
       3334    3334  177565
    ~/chrome/src$ TESTS=`git grep -n -e '^[^ ]*TEST' -l -- '*test.cc'`
    ~/chrome/src$ git grep -n -e '^\([^ ]*TEST\|}  // namespace$\)' -- $TESTS |
tac | awk -F :  -- '{if (last == $1) next; last = $1; print}' | grep namespace |
wc
        577    1731   40215
  
So it looks like about 577/3334 = 17% of tests are in the anonymous namespace.

    ~/chrome/src$ git grep -n -e '^\([^ ]*TEST\|}  // namespace \)' -- $TESTS |
tac | awk -F :  -- '{if (last == $1) next; last = $1; print}' | grep namespace |
wc
        2094    8376  166222

Another 2094/3334 = 63% of tests are in named namespaces.

Only 20% of tests are not in a namespace. That's the unusual case.

But even if a namespace wasn't usual, it'd be the right thing to do. Putting it
in an anonymous namespace isolates it. And you also learn that none of the tests
require friendship. If we're not doing it elsewhere, that's a mistake. Let's get
it right here.

[rvargas (doing something else), 2013/08/05 19:50:45]

What problem are you trying to solve?

The usual guidance of unit test files for chrome was to avoid namespaces so that
the test was not using unintentionally any code inside the namespace under test.
The second reason was to somewhat use the same pattern that actual consumers
would have to use when dealing with the code, and using the explicit namespace
is one of those things that consumers have to deal with.

A clear problem with that is when tests need to friend the code under test, so a
good number of test files started getting that namespace. In that sense, it is
kind of funny to have a written rule saying "don't use namespaces" when half of
the files would have to anyway.

Given that these tests don't _have to_ live inside net, I still think it is a
good idea to follow the old guidance.

Now, about an anonymous namespace... what problem would that be solving,
exactly? Name collisions with another, unrelated test? Isn't that the reason to
be very explicit about the name used for the tests? I would prefer a world
without name collisions than a world when collisions are handled by virtue of
anonymous namespaces.

[gavinp, 2013/08/06 01:11:02]

The anonymous namespace is the only one I advocated, so I will only address
that.

- A test in an anonymous namespace is guaranteed not to be a friend of another
class. That's a huge virtue that helps readability: it's a strong assertion
about the relationship of the test to the published interfaces of the classes it
includes.

- Name collisions at linkage are bad, indeed. Further, they have undefined
results as far as I'm aware. Having translation units export symbols that aren't
even in a header is just asking for trouble down the line. It would be wonderful
if we never collided in names, but many tests mock similar things, many tests
create similar test classes, it just happens. Defined results win over undefined
results every time.

- Linking is not free. The general linking problem is O(n^2), and reducing n is
a great path to reducing link time.

- The Google C++ style guide tells me that using namespaces is encouraged. See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...

- The Chromium style guide tells me that "Items local to a .cc file should be
wrapped in an unnamed namespace", which is stronger, and appears fairly
definitive in the case of these tests.

[rvargas (doing something else), 2013/08/08 02:50:01]

file_a.cc:
namespace {
TEST(a, foo){
}
}

file_b.cc:
namespace {
TEST(a, foo){
}
}

That should compile and link alright... what happens when I run tests with
--gtest_filter=a.foo ?

I honestly don't want to delve into gtest and/or gmock implementation to see
what it does.

If you are arguing about placing common infrastructure (mocks etc) inside an
anonymous namespace, I'm fine with that (see http_cache_unittest.cc for an
example). This file has only tests.

If you're still not convinced that's fine. I'm not asking you (right now) to
change the code you are writing. However, as a whole, your point is not
enforceable at the moment. See:

https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/test$2...

-using base::Time;
+// Tests that we add and remove blocks correctly.
+TEST(DiskCacheBlockBitmaps, V3AllocationMap) {
+  disk_cache::BlockBitmaps block_bitmaps;
+  disk_cache::BlockFilesBitmaps bitmaps;
 
-namespace {
-
-// Returns the number of files in this folder.
-int NumberOfFiles(const base::FilePath& path) {
-  base::FileEnumerator iter(path, false, base::FileEnumerator::FILES);
-  int count = 0;
-  for (base::FilePath file = iter.Next(); !file.value().empty();
-       file = iter.Next()) {
-    count++;
+  const int kNumHeaders = 10;  // Just a bunch of "files".
+  disk_cache::BlockFileHeader headers[kNumHeaders];
+  for (int i = 0; i < kNumHeaders; i++) {
+    memset(&headers[i], 0, sizeof(headers[i]));
+    headers[i].magic = disk_cache::kBlockMagic;
+    headers[i].version = disk_cache::kBlockCurrentVersion;
+    headers[i].this_file = static_cast<int16>(i);
+    headers[i].empty[3] = 200;
+    headers[i].max_entries = 800;
+    bitmaps.push_back(disk_cache::BlockHeader(&headers[i]));
   }
-  return count;
-}
 
-}  // namespace;
+  block_bitmaps.Init(bitmaps);
 
-namespace disk_cache {
-
-TEST_F(DiskCacheTest, BlockFiles_Grow) {
-  ASSERT_TRUE(CleanupCacheDir());
-  ASSERT_TRUE(file_util::CreateDirectory(cache_path_));
-
-  BlockFiles files(cache_path_);
-  ASSERT_TRUE(files.Init(true));
-
-  const int kMaxSize = 35000;
-  Addr address[kMaxSize];
-
-  // Fill up the 32-byte block file (use three files).
-  for (int i = 0; i < kMaxSize; i++) {
-    EXPECT_TRUE(files.CreateBlock(RANKINGS, 4, &address[i]));
-  }
-  EXPECT_EQ(6, NumberOfFiles(cache_path_));
-
-  // Make sure we don't keep adding files.
-  for (int i = 0; i < kMaxSize * 4; i += 2) {
-    int target = i % kMaxSize;
-    files.DeleteBlock(address[target], false);
-    EXPECT_TRUE(files.CreateBlock(RANKINGS, 4, &address[target]));
-  }
-  EXPECT_EQ(6, NumberOfFiles(cache_path_));
-}
-
-// We should be able to delete empty block files.
-TEST_F(DiskCacheTest, BlockFiles_Shrink) {
-  ASSERT_TRUE(CleanupCacheDir());
-  ASSERT_TRUE(file_util::CreateDirectory(cache_path_));
-
-  BlockFiles files(cache_path_);
-  ASSERT_TRUE(files.Init(true));
-
-  const int kMaxSize = 35000;
-  Addr address[kMaxSize];
-
-  // Fill up the 32-byte block file (use three files).
-  for (int i = 0; i < kMaxSize; i++) {
-    EXPECT_TRUE(files.CreateBlock(RANKINGS, 4, &address[i]));
-  }
-
-  // Now delete all the blocks, so that we can delete the two extra files.
-  for (int i = 0; i < kMaxSize; i++) {
-    files.DeleteBlock(address[i], false);
-  }
-  EXPECT_EQ(4, NumberOfFiles(cache_path_));
-}
-
-// Handling of block files not properly closed.
-TEST_F(DiskCacheTest, BlockFiles_Recover) {
-  ASSERT_TRUE(CleanupCacheDir());
-  ASSERT_TRUE(file_util::CreateDirectory(cache_path_));
-
-  BlockFiles files(cache_path_);
-  ASSERT_TRUE(files.Init(true));
-
-  const int kNumEntries = 2000;
-  CacheAddr entries[kNumEntries];
-
-  int seed = static_cast<int>(Time::Now().ToInternalValue());
-  srand(seed);
-  for (int i = 0; i < kNumEntries; i++) {
-    Addr address(0);
-    int size = (rand() % 4) + 1;
-    EXPECT_TRUE(files.CreateBlock(RANKINGS, size, &address));
-    entries[i] = address.value();
-  }
-
-  for (int i = 0; i < kNumEntries; i++) {
-    int source1 = rand() % kNumEntries;
-    int source2 = rand() % kNumEntries;
-    CacheAddr temp = entries[source1];
-    entries[source1] = entries[source2];
-    entries[source2] = temp;
-  }
-
-  for (int i = 0; i < kNumEntries / 2; i++) {
-    Addr address(entries[i]);
-    files.DeleteBlock(address, false);
-  }
-
-  // At this point, there are kNumEntries / 2 entries on the file, randomly
-  // distributed both on location and size.
-
-  Addr address(entries[kNumEntries / 2]);
-  MappedFile* file = files.GetFile(address);
-  ASSERT_TRUE(NULL != file);
-
-  BlockFileHeader* header =
-      reinterpret_cast<BlockFileHeader*>(file->buffer());
-  ASSERT_TRUE(NULL != header);
-
-  ASSERT_EQ(0, header->updating);
-
-  int max_entries = header->max_entries;
-  int empty_1 = header->empty[0];
-  int empty_2 = header->empty[1];
-  int empty_3 = header->empty[2];
-  int empty_4 = header->empty[3];
-
-  // Corrupt the file.
-  header->max_entries = header->empty[0] = 0;
-  header->empty[1] = header->empty[2] = header->empty[3] = 0;
-  header->updating = -1;
-
-  files.CloseFiles();
-
-  ASSERT_TRUE(files.Init(false));
-
-  // The file must have been fixed.
-  file = files.GetFile(address);
-  ASSERT_TRUE(NULL != file);
-
-  header = reinterpret_cast<BlockFileHeader*>(file->buffer());
-  ASSERT_TRUE(NULL != header);
-
-  ASSERT_EQ(0, header->updating);
-
-  EXPECT_EQ(max_entries, header->max_entries);
-  EXPECT_EQ(empty_1, header->empty[0]);
-  EXPECT_EQ(empty_2, header->empty[1]);
-  EXPECT_EQ(empty_3, header->empty[2]);
-  EXPECT_EQ(empty_4, header->empty[3]);
-}
-
-// Handling of truncated files.
-TEST_F(DiskCacheTest, BlockFiles_ZeroSizeFile) {
-  ASSERT_TRUE(CleanupCacheDir());
-  ASSERT_TRUE(file_util::CreateDirectory(cache_path_));
-
-  BlockFiles files(cache_path_);
-  ASSERT_TRUE(files.Init(true));
-
-  base::FilePath filename = files.Name(0);
-  files.CloseFiles();
-  // Truncate one of the files.
-  {
-    scoped_refptr<File> file(new File);
-    ASSERT_TRUE(file->Init(filename));
-    EXPECT_TRUE(file->SetLength(0));
-  }
-
-  // Initializing should fail, not crash.
-  ASSERT_FALSE(files.Init(false));
-}
-
-// Handling of truncated files (non empty).
-TEST_F(DiskCacheTest, BlockFiles_TruncatedFile) {
-  ASSERT_TRUE(CleanupCacheDir());
-  ASSERT_TRUE(file_util::CreateDirectory(cache_path_));
-
-  BlockFiles files(cache_path_);
-  ASSERT_TRUE(files.Init(true));
-  Addr address;
-  EXPECT_TRUE(files.CreateBlock(RANKINGS, 2, &address));
-
-  base::FilePath filename = files.Name(0);
-  files.CloseFiles();
-  // Truncate one of the files.
-  {
-    scoped_refptr<File> file(new File);
-    ASSERT_TRUE(file->Init(filename));
-    EXPECT_TRUE(file->SetLength(15000));
-  }
-
-  // Initializing should fail, not crash.
-  ASSERT_FALSE(files.Init(false));
-}
-
-// Tests detection of out of sync counters.
-TEST_F(DiskCacheTest, BlockFiles_Counters) {
-  ASSERT_TRUE(CleanupCacheDir());
-  ASSERT_TRUE(file_util::CreateDirectory(cache_path_));
-
-  BlockFiles files(cache_path_);
-  ASSERT_TRUE(files.Init(true));
-
-  // Create a block of size 2.
-  Addr address(0);
-  EXPECT_TRUE(files.CreateBlock(RANKINGS, 2, &address));
-
-  MappedFile* file = files.GetFile(address);
-  ASSERT_TRUE(NULL != file);
-
-  BlockFileHeader* header = reinterpret_cast<BlockFileHeader*>(file->buffer());
-  ASSERT_TRUE(NULL != header);
-  ASSERT_EQ(0, header->updating);
-
-  // Alter the counters so that the free space doesn't add up.
-  header->empty[2] = 50;  // 50 free blocks of size 3.
-  files.CloseFiles();
-
-  ASSERT_TRUE(files.Init(false));
-  file = files.GetFile(address);
-  ASSERT_TRUE(NULL != file);
-  header = reinterpret_cast<BlockFileHeader*>(file->buffer());
-  ASSERT_TRUE(NULL != header);
-
-  // The file must have been fixed.
-  ASSERT_EQ(0, header->empty[2]);
-
-  // Change the number of entries.
-  header->num_entries = 3;
-  header->updating = 1;
-  files.CloseFiles();
-
-  ASSERT_TRUE(files.Init(false));
-  file = files.GetFile(address);
-  ASSERT_TRUE(NULL != file);
-  header = reinterpret_cast<BlockFileHeader*>(file->buffer());
-  ASSERT_TRUE(NULL != header);
-
-  // The file must have been "fixed".
-  ASSERT_EQ(2, header->num_entries);
-
-  // Change the number of entries.
-  header->num_entries = -1;
-  header->updating = 1;
-  files.CloseFiles();
-
-  // Detect the error.
-  ASSERT_FALSE(files.Init(false));
-}
-
-// An invalid file can be detected after init.
-TEST_F(DiskCacheTest, BlockFiles_InvalidFile) {
-  ASSERT_TRUE(CleanupCacheDir());
-  ASSERT_TRUE(file_util::CreateDirectory(cache_path_));
-
-  BlockFiles files(cache_path_);
-  ASSERT_TRUE(files.Init(true));
-
-  // Let's access block 10 of file 5. (There is no file).
-  Addr addr(BLOCK_256, 1, 5, 10);
-  EXPECT_TRUE(NULL == files.GetFile(addr));
-
-  // Let's create an invalid file.
-  base::FilePath filename(files.Name(5));
-  char header[kBlockHeaderSize];
-  memset(header, 'a', kBlockHeaderSize);
-  EXPECT_EQ(kBlockHeaderSize,
-            file_util::WriteFile(filename, header, kBlockHeaderSize));
-
-  EXPECT_TRUE(NULL == files.GetFile(addr));
-
-  // The file should not have been changed (it is still invalid).
-  EXPECT_TRUE(NULL == files.GetFile(addr));
-}
-
-// Tests that we generate the correct file stats.
-TEST_F(DiskCacheTest, BlockFiles_Stats) {
-  ASSERT_TRUE(CopyTestCache("remove_load1"));
-
-  BlockFiles files(cache_path_);
-  ASSERT_TRUE(files.Init(false));
-  int used, load;
-
-  files.GetFileStats(0, &used, &load);
-  EXPECT_EQ(101, used);
-  EXPECT_EQ(9, load);
-
-  files.GetFileStats(1, &used, &load);
-  EXPECT_EQ(203, used);
-  EXPECT_EQ(19, load);
-
-  files.GetFileStats(2, &used, &load);
-  EXPECT_EQ(0, used);
-  EXPECT_EQ(0, load);
-}
-
-// Tests that we add and remove blocks correctly.
-TEST_F(DiskCacheTest, AllocationMap) {
-  ASSERT_TRUE(CleanupCacheDir());
-  ASSERT_TRUE(file_util::CreateDirectory(cache_path_));
-
-  BlockFiles files(cache_path_);
-  ASSERT_TRUE(files.Init(true));
-
   // Create a bunch of entries.
   const int kSize = 100;
-  Addr address[kSize];
+  disk_cache::Addr address[kSize];
   for (int i = 0; i < kSize; i++) {
     SCOPED_TRACE(i);
     int block_size = i % 4 + 1;
-    EXPECT_TRUE(files.CreateBlock(BLOCK_1K, block_size, &address[i]));
-    EXPECT_EQ(BLOCK_1K, address[i].file_type());
+    EXPECT_TRUE(block_bitmaps.CreateBlock(disk_cache::BLOCK_1K, block_size,
+                                          &address[i]));
+    EXPECT_EQ(disk_cache::BLOCK_1K, address[i].file_type());
     EXPECT_EQ(block_size, address[i].num_blocks());
     int start = address[i].start_block();
     EXPECT_EQ(start / 4, (start + block_size - 1) / 4);
@@ -322,14 +43,13 @@
 
   for (int i = 0; i < kSize; i++) {
     SCOPED_TRACE(i);
-    EXPECT_TRUE(files.IsValid(address[i]));
+    EXPECT_TRUE(block_bitmaps.IsValid(address[i]));
   }
 
   // The first part of the allocation map should be completely filled. We used
-  // 10 bits per each four entries, so 250 bits total.
-  BlockFileHeader* header =
-      reinterpret_cast<BlockFileHeader*>(files.GetFile(address[0])->buffer());
-  uint8* buffer = reinterpret_cast<uint8*>(&header->allocation_map);
+  // 10 bits per each four entries, so 250 bits total. All entries should go to
+  // the third file.
+  uint8* buffer = reinterpret_cast<uint8*>(&headers[2].allocation_map);
   for (int i =0; i < 29; i++) {
     SCOPED_TRACE(i);
     EXPECT_EQ(0xff, buffer[i]);
@@ -337,7 +57,7 @@
 
   for (int i = 0; i < kSize; i++) {
     SCOPED_TRACE(i);
-    files.DeleteBlock(address[i], false);
+    block_bitmaps.DeleteBlock(address[i]);
   }
 
   // The allocation map should be empty.
@@ -346,5 +66,3 @@
     EXPECT_EQ(0, buffer[i]);
   }
 }
-
-}  // namespace disk_cache