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.

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

Is "file operation" well defined?

This class definitely does a lot of IO. Can we expand on this comment a bit,
both in explaining what a BlockHeader is, and what it means to not do file
operations?

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

I meant the usual meaning of file operations (file IO). This class doesn't do
file IO by itself any more than any class that touches memory (almost any
class?) does file IO.

[Randy Smith (Not in Mondays), 2013/08/05 20:23:49]

For what it's worth, this confused me a bit too when I read it.  Touching memory
that's mapped from a file means you can't make any guarantees about when file
data changes on disk, so I'm not sure what use the comment has.  Is there a way
in which consumer code relies on this class not performing explicit file
operations, while it performing implicit file changes is ok?  (Or am I confused
about it touching data mapped from a file somehow?)

[rvargas (doing something else), 2013/08/05 21:03:02]

Now I'm confused too. I can certainly remove the comment... the only reason for
the comment was to point out the main design difference between working with
this object (or a collection of these objects, like BlockFilesBitmaps), and
working instead with BlockFiles. As in BackendImplV2 deals with BlockFiles and
it should expect all kind of synchronous IO operations to take place, as opposed
to BackendImplV3 which, by using this class (or specifically BlockBitmaps which
relies on BlockFilesBitmaps which is a vector of BlockHeader(s)), can expect all
calls to be non-blocking.

Another way to put it is that this class is IO thread friendly, as opposed to
BlockFiles... but saying that violates the abstraction of threads of net.

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

Is this class constructed on something other than a memory mapped file in
anything other than tests? Can you tell me more about that situation?

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

Dereferencing a pointer into a memory map is no different than calling read(2)
on some offset into the file. If the latter is prohibited, so is the former. It
is NOT OK to memory map a file and then use that memory map on the IO thread. Do
not be confused by the fact that a thread guard doesn't prevent this.

Further, reading and writing to memory that is, in all cases, expected to be
backed by a memory map is precisely doing IO using read(2) and write(2). If this
class was designed with the expectation that it dereferences pointers into
memory maps, it was designed to do IO.

[Randy Smith (Not in Mondays), 2013/08/06 15:16:28]

Just to make explicit what may be obvious to both of you (based on a
conversation Gavin and I had last night): This equivalence is because reading or
writing to a memory mapped to a file may block as the file is paged in.  In some
ways it's worse than using read/write because you don't know ahead of time if
you get blocked, and if you do the code doesn't let you know.

 class NET_EXPORT_PRIVATE BlockHeader {
  public:
   BlockHeader();
   explicit BlockHeader(BlockFileHeader* header);
   explicit BlockHeader(MappedFile* file);

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

Is it appropriate to add a TODO(rvargas): Remove this constructor after v3 cache
finishes landing?

[gavinp, 2013/08/26 14:30:56]

Do these TODO comments make sense? I was asking as much to make sure I
understand what's happening as to improve the code?

[rvargas (doing something else), 2013/09/11 02:49:53]

I added a TODO at line 81 of PS9... I thought that made it clear that the class
has more than needed while V2 and V3 coexist.

   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_; }
-  void operator=(const BlockHeader& other) { header_ = other.header_; }
-  BlockFileHeader* Get() { return header_; }
+  // Returns a pointer to the underlying BlockFileHeader.

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

Is it appropriate to add a TODO(rvargas): Remove this accessor after v3 cache
finishes landing?

[gavinp, 2013/08/26 14:30:56]

Do these TODO comments make sense? I was asking as much to make sure I
understand what's happening as to improve the code?

+  BlockFileHeader* Header();
 
  private:
   BlockFileHeader* header_;
 };
 
 typedef std::vector<BlockHeader> BlockFilesBitmaps;

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

In reviewing this code, I think this typedef is causing me some pain. Can we
just use std::vector<BlockHeader> everywhere, it's only seven characters longer?
I think it's referenced only twice?

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

It is used more that twice on the V3 code. This is the type that is transferred
between BlockFiles and BlockBitmaps whenever the mappings change.

BlockFilesMappings ?

[gavinp, 2013/08/26 14:30:56]

I'll go look at the V3 code. But I'm still scared; I worry that STL typedefs
hide subtleties that deserve attention.

Can we just not use the typedef here, and then later introduce it in one of the
downstream CLs in which we use it more?

[rvargas (doing something else), 2013/09/11 02:49:53]

I could move the define to block_bitmaps.h in this CL, but it will be moving
back to this file later on (because it is used by BlockFiles) so I don't see
much value in doing that.

How about leaving it here for the time being and talking about its need when
there are more users?. I don't share your concern about stl defines but we
should talk about that later.

 
 // This class handles the set of block-files open by the disk cache.
 class NET_EXPORT_PRIVATE BlockFiles {
  public:
   explicit BlockFiles(const base::FilePath& path);
   ~BlockFiles();
 
   // Performs the object initialization. create_files indicates if the backing
   // files should be created or just open.
   bool Init(bool create_files);
@@ skipping 61 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_