Disk cache: Introduce BlockBitmaps for V3.

net/disk_cache/v3/block_bitmaps.h

Index: net/disk_cache/v3/block_bitmaps.h
===================================================================
--- net/disk_cache/v3/block_bitmaps.h	(revision 210990)
+++ net/disk_cache/v3/block_bitmaps.h	(working copy)
@@ -4,43 +4,34 @@
 
 // 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_
+#ifndef NET_DISK_CACHE_V3_BLOCK_BITMAPS_H_
+#define NET_DISK_CACHE_V3_BLOCK_BITMAPS_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/mapped_file.h"
+#include "net/disk_cache/block_files.h"
 
 namespace disk_cache {
 
 // This class handles the set of block-files open by the disk cache.

[gavinp, 2013/08/05 17:06:15]

Can we expand on this comment a bit? I'm still a bit confused about what a
bitmap is? Also "handles" is a really general verb. More specifics will put this
in context.

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

Expended the comment. I can think of handles/represents/encapsulates but the
first one is the most accurate imo.

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

None of "handles/represents/encapsulates" are great words to use when explaining
a new object to a naive audience, though they sometimes work. The trouble is
they're really general. I want to be able to read a class description in a
header and put the class in context of what it's used for; knowing about what it
operates on, what operations it does, and what consumers it's aimed at are
great.

The expansion helps me, but I'm still a bit befuddled. What does it mean to not
use dedicated files? This interface is used by what consumers? I really am at a
loss here.

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

"dedicated files" is a basic concept of this cache: a resource can be stored
inside a block file, or on a dedicated file. I was afraid you were going to read
a statement without an explicit mention to the exception (storage needed by the
cache which is _not_ handled by this class) and come back saying that the
comment was misleading.

While it is possible to list all known consumers of a class on its declaration,
I generally consider that practice to be incorrect because a class should not
care about who uses it, and it breaks the logical structure of dependencies.
Granted, the final version of this class has basically one implied user because
the constructor takes a BackendImplV3 so I might as well document at this point
who is the caller, but that is certainly not my first impulse when documenting a
class.

As for what it does (what's the meaning of "handling"), a quick glance at the 6
public methods convey a much clear idea of what it does than adding text
repeating (although in advance) how to use this class. In fact it mainly creates
and deletes blocks, so I honestly considered saying that when you asked for an
expanded comment. My issue is that I see it basically as repetition, and it is
really all that the backend needs to do with blockfiles as an entity (which is
the actual point of this class and it is expressed in the first line of the
text). Ha, I just realized that I actually wrote that it creates and deletes
blocks even though I really don't like that as a "description".

Keep in mind that this is not a general purpose class, and it is not intended to
be used by anybody outside of the implementation of this cache. It even has a
comment pointing stray readers to the proper public interface.

At the end of the day, it is much faster if you just write the text that you
want to see here and I copy it.

[gavinp, 2013/08/08 03:04:46]

If I understood what this class did, I would be happy to do so.

However, I do not.

[Randy Smith (Not in Mondays), 2013/08/09 19:11:49]

At risk of saying something completely wrong (which seems to be my modus
operandi in this review :-}), may I give it a try?

"This class is the interface in the v3 disk cache to the set of files holding
cached data that is small enough to not be efficiently stored in a dedicated
file (i.e. < kMaxBlockSize).  It is primarily used to allocate and free regions
in those files used to store data."

If that doesn't address your concerns, Gavin, could you give some specific
questions you imagine someone reading this code for the first time would have on
reading it?

(Ricardo: If I'm saying something incorrect or that you object to, please
correct me--I'm focussing on Gavin since I think this is just an issue of
addressing his concerns about documentation.)

[rvargas (doing something else), 2013/08/09 19:53:41]

Thanks. I'm more than happy happy to use that description.

-class NET_EXPORT_PRIVATE BlockFiles {
+class NET_EXPORT_PRIVATE BlockBitmaps {
  public:
-  explicit BlockFiles(const base::FilePath& path);
-  ~BlockFiles();
+  BlockBitmaps();
+  ~BlockBitmaps();
 
-  // Performs the object initialization. create_files indicates if the backing
-  // files should be created or just open.
-  bool Init(bool create_files);
+  void Init(const BlockFilesBitmaps& bitmaps);
 
   // Creates a new entry on a block file. block_type indicates the size of block
   // to be used (as defined on cache_addr.h), block_count is the number of
   // blocks to allocate, and block_address is the address of the new entry.
   bool CreateBlock(FileType block_type, int block_count, Addr* block_address);
 
-  // Removes an entry from the block files. If deep is true, the storage is zero
-  // filled; otherwise the entry is removed but the data is not altered (must be
-  // already zeroed).
-  void DeleteBlock(Addr address, bool deep);
+  // Removes an entry from the block files.
+  void DeleteBlock(Addr address);
 
-  // Close all the files and set the internal state to be initializad again. The
-  // cache is being purged.
-  void CloseFiles();
+  // Releases the internal bitmaps. The cache is being purged.
+  void Clear();
 
   // Sends UMA stats.
   void ReportStats();
@@ -50,26 +41,20 @@
   bool IsValid(Addr address);
 
  private:
-  // Returns the file that stores a given address.
-  MappedFile* GetFile(Addr address);
+  // Returns the header number that stores a given address.
+  int GetHeaderNumber(Addr address);
 
-  // Attemp to grow this file. Fails if the file cannot be extended anymore.
-  bool GrowBlockFile(MappedFile* file, BlockFileHeader* header);
+  // Returns the appropriate header to use for a new block.
+  int HeaderNumberForNewBlock(FileType block_type, int block_count);
 
-  // Returns the appropriate file to use for a new block.
-  MappedFile* FileForNewBlock(FileType block_type, int block_count);
-
-  // Restores the header of a potentially inconsistent file.
-  bool FixBlockFileHeader(MappedFile* file);
-
   // Retrieves stats for the given file index.
   void GetFileStats(int index, int* used_count, int* load);
 
-  bool init_;
+  BlockFilesBitmaps bitmaps_;
 
-  DISALLOW_COPY_AND_ASSIGN(BlockFiles);
+  DISALLOW_COPY_AND_ASSIGN(BlockBitmaps);
 };
 
 }  // namespace disk_cache
 
-#endif  // NET_DISK_CACHE_BLOCK_FILES_H_
+#endif  // NET_DISK_CACHE_V3_BLOCK_BITMAPS_H_