Disk cache: Introduce BlockBitmaps for V3.

net/disk_cache/block_files.h

 // 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.
 
 // See net/disk_cache/disk_cache.h for the public interface.
 
 #ifndef NET_DISK_CACHE_BLOCK_FILES_H_
 #define NET_DISK_CACHE_BLOCK_FILES_H_
 
 #include <vector>
 
 #include "base/files/file_path.h"
 #include "base/gtest_prod_util.h"
 #include "base/memory/scoped_ptr.h"
 #include "net/base/net_export.h"
 #include "net/disk_cache/addr.h"
 #include "net/disk_cache/disk_format_base.h"
 #include "net/disk_cache/mapped_file.h"
 
 namespace base {
 class ThreadChecker;
 }
 
 namespace disk_cache {
 
 // An instance of this class represents the header of a block file in memory.
 // Note that this class doesn't perform any file operation.
 class NET_EXPORT_PRIVATE BlockHeader {

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

This class continues to look like a lot of mechanism for a BlockFileHeader*; and
it introduces the potential for confusing double indirection if it's not used
carefully. I can understand why we don't want these semantic methods to be in
the file format definition, but I don't think this class is the right way to do
it.

I also understand that this is intended to be a kind of abstraction boundary,
between things on memory and things that might be on disk. That's why it
introduces these semantic methods. But I can't get my head around this class as
the right way to introduce this abstraction boundary. If you have a
BlockFIleHeader object, you can do all of these semantic operations.

I have two suggestions:

1. Have this class inherit from BlockFileHeader, and do the (sketchy)
static_cast from BlockFileHeader* to BlockHeader* when you need one of these
objects. We'd need a compile assert to make sure BlockHeader truly is POD from
the struct; I think sizeof() being equal should do it, since BlockFileHeader is
POD adding a virtual method would add a vptr.

2. Write this interface in the C-style for structure methods; i.e.

namespace disk_cache {

namespace block_header {

  bool CreateMapBlock(BlockFileHeader*, int target, int size, int* index);
  bool DeleteMapBlock(BlockFileHeader*, int index, int block_size);
...

}

}

Of these, I like (2) best, after all, the data is exactly a BlockFIleHeader;
callers can know that. But if they've brought in this header, they get the
semantic operations on that disk structure.

But, (1) is most like the current idea here. There's a data type that captures
the distinction. There will be a lot of static casts.

What do you think of these suggestions? Do you see any advantages or
disadvantages to them?

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

I'm not sure if it's a matter of naming what is bothering you.

What I want is an abstraction of the operations that I can do with the metadata
of a single logical block file (every word is important here because another CL
will be adding support for a block file that is implemented with more than one
physical file on disk).

That basically means Create and delete a block etc up to Size (as I said before,
Size is not needed on the final version). All those methods operate on the
concept of the metadata of a block file (whatever name that has). So I don't
want a namespace, I want a class because I have an entity that behaves like an
object. And I want a user of that class to know that if it needs to look for an
empty space to store a block on a given file, it invokes a method on the object
that represents that file.

So we have 8 methods that operate on that object and one method that retrieves a
pointer to the struct (the actual header) if the caller needs to access one of
the 11 fields of that struct.

I cannot simply inherit from the struct because this concept is not a struct
with methods... the underlying (in principle private) member is a pointer to a
struct: a pointer that comes from a memory mapped section, from a file (handle)
owned by another entity.

I _could_ remove the accessor to the struct but that in practice means that
users of this class will most likely need to keep two objects to interact with
two views of an entity: an instance of this class to FixAllocationCounters, and
a separate pointer to the header to get the number of the next file of the same
type. Not exactly a gain in overall clarity, simplicity or encapsulation.

My last point is that I don't want to over engineer things to accommodate code
that should go away soon (when we drop support for v2), but this particular
class moves in the direction that I want the code to go: more encapsulation and
reliance on higher level interfaces.

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

No, it's the structure. This abstraction does only two things; it hides a cast,
and it adds data methods here rather than with the disk format; and using this
class obscures that none of these methods are not actually just operating on a
BlockHeader. I tried to make two suggestions that unobscure that, and make the
casts more clear.

I can understand the desire to keep the methods that operate on the disk format
separate from the disk format. But I'm troubled by a class that is just a
pointer and a bunch of methods that really just operate on the disk format type.
Is there a final version of this class that provides something more than just a
BlockFileHeader*? Will every method on it require that data? How can we separate
methods that just operate on BlockFileHeader* from the methods that use the new
private data in a BlockHeader*?

  public:
   BlockHeader();
   explicit BlockHeader(BlockFileHeader* header);
   explicit BlockHeader(MappedFile* file);
   BlockHeader(const BlockHeader& other);
   ~BlockHeader();
 
   // Creates a new entry on the allocation map, updating the apropriate
   // counters. |target| is the type of block to use (number of empty blocks),
   // and |size| is the actual number of blocks to use.
   bool CreateMapBlock(int target, int size, int* index);
 
   // Deletes the block pointed by |index|.
   void DeleteMapBlock(int index, int block_size);
 
   // Returns true if the specified block is used.
   bool UsedMapBlock(int index, int size);
 
   // Restores the "empty counters" and allocation hints.
   void FixAllocationCounters();
 
   // Returns true if the current block file should not be used as-is to store
   // more records. |block_count| is the number of blocks to allocate.
-  bool NeedToGrowBlockFile(int block_count);
+  bool NeedToGrowBlockFile(int block_count) const;
+
+  // Returns true if this block file can be used to store an extra record of
+  // size |block_count|.
+  bool CanAllocate(int block_count) const;
 
   // Returns the number of empty blocks for this file.
   int EmptyBlocks() const;
 
   // Returns true if the counters look OK.
   bool ValidateCounters() const;
 
   // Returns the size of the wrapped structure (BlockFileHeader).
   int Size() const;
 
   BlockFileHeader* operator->() { return header_; }

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

The Google C++ style guide has a strong presumption against operator
overloading:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Operator_Overl...

Is this a "rare, special circumstance"? How?

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

Happy to remove it.

   void operator=(const BlockHeader& other) { header_ = other.header_; }
   BlockFileHeader* Get() { return header_; }
 
  private:
   BlockFileHeader* header_;
 };
 
 typedef std::vector<BlockHeader> BlockFilesBitmaps;
 
 // This class handles the set of block-files open by the disk cache.
@@ skipping 69 matching lines @@
   FRIEND_TEST_ALL_PREFIXES(DiskCacheTest, BlockFiles_TruncatedFile);
   FRIEND_TEST_ALL_PREFIXES(DiskCacheTest, BlockFiles_InvalidFile);
   FRIEND_TEST_ALL_PREFIXES(DiskCacheTest, BlockFiles_Stats);
 
   DISALLOW_COPY_AND_ASSIGN(BlockFiles);
 };
 
 }  // namespace disk_cache
 
 #endif  // NET_DISK_CACHE_BLOCK_FILES_H_