Disk cache: First pass to add support for sparse entries....

Disk cache: First pass to add support for sparse entries.

Adding Read/Write support.

BUG=12258
TEST=unittests.



Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=18723

[rvargas (doing something else), 2009-06-16 00:48:33]

Carlos,

Please review Bitmap.* and IOBuffer.*

Nicolas,

Please review everything else. You may want to look at disk_cache.h to see
what's this whole thing about.

[Nicolas Sylvain, 2009-06-16 15:06:06]

Haven't looked at the 2 new sparse file yet. Will do that later.

Some initial comments below

http://codereview.chromium.org/126179/diff/1040/42
File net/disk_cache/entry_impl.cc (right):

http://codereview.chromium.org/126179/diff/1040/42#newcode97
Line 97: if (sparse_.get())
Do you really need to do a .get here? .reset should do the right thing if it's
null?

http://codereview.chromium.org/126179/diff/1040/42#newcode391
Line 391: if (!sparse_.get()) {
This piece of code is there 3 times. maybe it should be moved to a common
function? 

If you prefer to keep it like this, at least make it really identical (i.e. you
have some comments in some that you don't have in others). It helps the eyes
realize that it's the same code.

http://codereview.chromium.org/126179/diff/1040/43
File net/disk_cache/entry_impl.h (right):

http://codereview.chromium.org/126179/diff/1040/43#newcode154
Line 154: scoped_ptr<SparseControl> sparse_;
you seem to have a comment for all the other members.

http://codereview.chromium.org/126179/diff/1040/44
File net/disk_cache/entry_unittest.cc (right):

http://codereview.chromium.org/126179/diff/1040/44#newcode901
Line 901: int ret = entry->ReadSparseData(offset, buf_1, size, cb);
would ReadSparseData stop reading when it reaches size? Well, i guess so, but
would it still be a success? 

I'm asking because you might want to read from a buffer that is bigger than your
source, to catch the case where the size of your sparse data is bigger than the
size passed in parameter.

Or maybe GetResult would return something like "buffer too small" ?

Or actually the sparse data is always kind of bigger anyway, and we always need
to ask for a range.. so passing a bigger buffer might make it fail if it tries
to read from a place where we never set sparse data before?

http://codereview.chromium.org/126179/diff/1040/44#newcode985
Line 985: TEST_F(DiskCacheEntryTest, DISABLED_MemoryOnlyHugeSparseSyncIO) {
what's up with that one?

[rvargas (doing something else), 2009-06-16 17:38:55]

thanks.

http://codereview.chromium.org/126179/diff/1040/44
File net/disk_cache/entry_unittest.cc (right):

http://codereview.chromium.org/126179/diff/1040/44#newcode901
Line 901: int ret = entry->ReadSparseData(offset, buf_1, size, cb);
IOBuffer does not include size information so it is impossible for the
implemenattion of ReadData to find out what's the real size of the buffer (or at
least that's the idea), so the only number that it can use is the explicit
argument. If the buffer is is not big enough, an access violation is likely to
occur (or heap corruption etc).

That said, I could allocate a bigger buffer here, and have two kSizes around,
but that may cause more confusion to the casual reader.

http://codereview.chromium.org/126179/diff/1040/44#newcode985
Line 985: TEST_F(DiskCacheEntryTest, DISABLED_MemoryOnlyHugeSparseSyncIO) {
The memory-only version is not implemented yet. I'm just being explicit about
tests that should pass at some point.

[cpu_(ooo_6.6-7.5), 2009-06-16 20:00:06]

LGTM on my part of the review, some comments below.

http://codereview.chromium.org/126179/diff/1040/35
File net/base/io_buffer.h (right):

http://codereview.chromium.org/126179/diff/1040/35#newcode33
Line 33: class IOBufferImpl : public IOBuffer {
IOBufferWithSize ? IOBufferImpl sounds like the implementation of some abstract
class.

http://codereview.chromium.org/126179/diff/1040/41
File net/disk_cache/bitmap.h (right):

http://codereview.chromium.org/126179/diff/1040/41#newcode22
Line 22: : max_size_(num_bits), array_size_(RequiredArraySize(num_bits)),
after looking over the logic below, it would be easier to read if max_size_ was
named num_bits_ or  something with 'bit' in the name like bit_count_

http://codereview.chromium.org/126179/diff/1040/41#newcode50
Line 50: void Resize(int num_bits, bool clear_bits);
imho it is surprising behavior that Resize() changes it from managed to
unmanaged memory (leak prone?) but if you really want to, you need to mention
it.

I would instead expect that resize would fail.

http://codereview.chromium.org/126179/diff/1040/41#newcode60
Line 60: memset(map_, (value ? 0xFF : 0x00), array_size_ * sizeof(*map_));
sizeof(map[0]) to keep consistency with the .cc

http://codereview.chromium.org/126179/diff/1040/41#newcode79
Line 79: // Sirectly sets the whole internal map. |size| is the number of 32-bit
words
typo

http://codereview.chromium.org/126179/diff/1040/41#newcode137
Line 137: static const int kIntBits = 32;    // 32 bits in a uint32
this should be a compile assert

[Nicolas Sylvain, 2009-06-17 04:07:00]

I could not find any error in space_control.cc, but this is a lot of code, that
needs a lot of visualization on how the pieces fall in together. (which I kind
of lack). If you want to have more sanity checking, we can meet tomorrow and
walk through the code together. (up to you).

LGTM

http://codereview.chromium.org/126179/diff/1048/60
File net/disk_cache/sparse_control.h (right):

http://codereview.chromium.org/126179/diff/1048/60#newcode66
Line 66: 
extra space here