Add Cache size to the Simple Index.

Add Cache size to the Simple Index.


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=193419

[gavinp, 2013-04-09 15:27:37]

This change introduces a second concept of size into the simple backend, and it
mixes them. And there's a third one to think about as well:

1) SimpleEntryImpl::GetDataSize(stream_index) is the amount of data in stream
stream_index

2) GetFilesize() returns the filesystem size of each stream, which includes the
header (variable length with key) & footer, in addition to stream_index.

3) On POSIX, we can look at the stat results in the PlatformFileInfo struct and
look at st_stblocks * st_blksize, and measure the number of bytes of disk space
the file allocates (minus directory metadata).

Should we get discipline about these and get this right? Probably worth thinking
about. There's some methods in the anon namespace at the top of
simple_synchronous_entry.cc that make these conversions between (1) and (2).

https://codereview.chromium.org/13913010/diff/14/net/disk_cache/simple/simple...
File net/disk_cache/simple/simple_entry_impl.cc (right):

https://codereview.chromium.org/13913010/diff/14/net/disk_cache/simple/simple...
net/disk_cache/simple/simple_entry_impl.cc:275: // cache size ?
sync_entry->data_size(i) still works at this point. But you're out of luck on
the entry that has now gone. We spoke earlier today about how we will actually
have to refcount the SimpleEntryImpl; perhaps a TODO(felipeg,gavinp): Once
SimpleEntryImpl is ref counted, update the index correctly in this case.

https://codereview.chromium.org/13913010/diff/14/net/disk_cache/simple/simple...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/13913010/diff/14/net/disk_cache/simple/simple...
net/disk_cache/simple/simple_index.cc:163: const base::FilePath::StringType
file_pattern = FILE_PATH_LITERAL("*_[0-2]");
This is a scary embedding of SimpleBackend::kSimpleEntryFileCount.

Maybe iterating over * is better, then just skipping non-entry files?

[gavinp, 2013-04-09 15:40:19]

https://chromiumcodereview.appspot.com/13913010/diff/14/net/disk_cache/simple...
File net/disk_cache/simple/simple_entry_impl.cc (right):

https://chromiumcodereview.appspot.com/13913010/diff/14/net/disk_cache/simple...
net/disk_cache/simple/simple_entry_impl.cc:296: total_entry_size +=
data_size_[i];
Maybe add SimpleSynchronousEntry::GetFileSize()?

[felipeg, 2013-04-09 16:24:35]

https://chromiumcodereview.appspot.com/13913010/diff/14/net/disk_cache/simple...
File net/disk_cache/simple/simple_entry_impl.cc (right):

https://chromiumcodereview.appspot.com/13913010/diff/14/net/disk_cache/simple...
net/disk_cache/simple/simple_entry_impl.cc:275: // cache size ?
On 2013/04/09 15:27:37, gavinp wrote:
> sync_entry->data_size(i) still works at this point. But you're out of luck on
> the entry that has now gone. We spoke earlier today about how we will actually
> have to refcount the SimpleEntryImpl; perhaps a TODO(felipeg,gavinp): Once
> SimpleEntryImpl is ref counted, update the index correctly in this case.

Done.

https://chromiumcodereview.appspot.com/13913010/diff/14/net/disk_cache/simple...
net/disk_cache/simple/simple_entry_impl.cc:296: total_entry_size +=
data_size_[i];
On 2013/04/09 15:40:19, gavinp wrote:
> Maybe add SimpleSynchronousEntry::GetFileSize()?

Done.

https://chromiumcodereview.appspot.com/13913010/diff/14/net/disk_cache/simple...
File net/disk_cache/simple/simple_index.cc (right):

https://chromiumcodereview.appspot.com/13913010/diff/14/net/disk_cache/simple...
net/disk_cache/simple/simple_index.cc:163: const base::FilePath::StringType
file_pattern = FILE_PATH_LITERAL("*_[0-2]");
On 2013/04/09 15:27:37, gavinp wrote:
> This is a scary embedding of SimpleBackend::kSimpleEntryFileCount.
> 
> Maybe iterating over * is better, then just skipping non-entry files?

Added a TODO

[gavinp, 2013-04-09 16:28:04]

https://chromiumcodereview.appspot.com/13913010/diff/7001/net/disk_cache/simp...
File net/disk_cache/simple/simple_entry_impl.cc (right):

https://chromiumcodereview.appspot.com/13913010/diff/7001/net/disk_cache/simp...
net/disk_cache/simple/simple_entry_impl.cc:268: DCHECK(index);
I don't think this DCHECK is right. What if the backend has been deleted while
the operation was pending?

Operations are expected to be able to outlive their backend.

https://chromiumcodereview.appspot.com/13913010/diff/7001/net/disk_cache/simp...
net/disk_cache/simple/simple_entry_impl.cc:298: }
Nit: lose braces.

[felipeg, 2013-04-09 16:36:39]

https://chromiumcodereview.appspot.com/13913010/diff/7001/net/disk_cache/simp...
File net/disk_cache/simple/simple_entry_impl.cc (right):

https://chromiumcodereview.appspot.com/13913010/diff/7001/net/disk_cache/simp...
net/disk_cache/simple/simple_entry_impl.cc:268: DCHECK(index);
On 2013/04/09 16:28:04, gavinp wrote:
> I don't think this DCHECK is right. What if the backend has been deleted while
> the operation was pending?
> 
> Operations are expected to be able to outlive their backend.

Done.

https://chromiumcodereview.appspot.com/13913010/diff/7001/net/disk_cache/simp...
net/disk_cache/simple/simple_entry_impl.cc:298: }
On 2013/04/09 16:28:04, gavinp wrote:
> Nit: lose braces.

Done.

[gavinp, 2013-04-09 16:42:57]

I'm going to look over this more, but just one comment for now; I'll hold my
LGTM back until I see those unit tests. :)

https://chromiumcodereview.appspot.com/13913010/diff/4003/net/disk_cache/simp...
File net/disk_cache/simple/simple_entry_impl.cc (right):

https://chromiumcodereview.appspot.com/13913010/diff/4003/net/disk_cache/simp...
net/disk_cache/simple/simple_entry_impl.cc:268: if (sync_entry && index)
sync_entry should always be non NULL, I think. So I think that can just be a
DCHECK.

[felipeg, 2013-04-09 16:46:57]

https://chromiumcodereview.appspot.com/13913010/diff/4003/net/disk_cache/simp...
File net/disk_cache/simple/simple_entry_impl.cc (right):

https://chromiumcodereview.appspot.com/13913010/diff/4003/net/disk_cache/simp...
net/disk_cache/simple/simple_entry_impl.cc:268: if (sync_entry && index)
On 2013/04/09 16:42:57, gavinp wrote:
> sync_entry should always be non NULL, I think. So I think that can just be a
> DCHECK.

Done.

[gavinp, 2013-04-10 10:33:23]

https://codereview.chromium.org/13913010/diff/4004/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_entry_impl.cc (right):

https://codereview.chromium.org/13913010/diff/4004/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_entry_impl.cc:269: if (sync_entry && index)
if (index)

https://codereview.chromium.org/13913010/diff/4004/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_entry_impl.h (right):

https://codereview.chromium.org/13913010/diff/4004/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_entry_impl.h:102: base::WeakPtr<SimpleIndex> index,
What's the right thing to do when an operation finishes after the simple index
has been deleted?

https://codereview.chromium.org/13913010/diff/4004/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/13913010/diff/4004/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.cc:116: // It will be updated later when the
SynchronousEntryImpl finish and the
Wording suggestion: It will be updated later when the SimpleEntryImpl finishes
opening or creating the new entry, and then UpdateEntrySize will be called.

Since the call comes from SimpleEntryImpl which is supposed to proxy for
SynchronousEntryImpl, this is conceptually easier for me. Also it avoids
mentioning threads.

https://codereview.chromium.org/13913010/diff/4004/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.cc:163: // TODO(felipeg,gavinp): Fix this
once we have a one-file per entry format.
COMPILE_ASSERT(kSimpleEntryFileCount == 3, file_pattern_must_match_file_count);

https://codereview.chromium.org/13913010/diff/4004/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.cc:164: const base::FilePath::StringType
file_pattern = FILE_PATH_LITERAL("*_[0-2]");
Would: ??????????_[0-2]

Be a good idea?

https://codereview.chromium.org/13913010/diff/4004/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.cc:190: EntrySet::iterator it =
entries_set_.find(hash_key);
Note that in the not found case (33% of enumerations) you're doing two searches
in the index. The truly cycle-counting coder writes:

std::pair<EntrySet::iterator, bool> insert_result =
entries_set_.insert(hash_key);
if (insert_result.second) {  // entry was inserted.
...
} else {  // entry was found.
...
}

Since nobody can ever read that style of code, and since InsertInternal does its
own insert, I think it's best to leave this as it is.

https://codereview.chromium.org/13913010/diff/4004/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index.h (right):

https://codereview.chromium.org/13913010/diff/4004/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.h:48: void Cleanup();
While we're here, can we comment this method? I forgot what it did while I was
reading this review. :/

https://codereview.chromium.org/13913010/diff/4004/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.h:50: bool UpdateEntrySize(const std::string&
key, uint64 entry_size);
I think this deserves a short comment describing what it does.

https://codereview.chromium.org/13913010/diff/4004/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_synchronous_entry.cc (right):

https://codereview.chromium.org/13913010/diff/4004/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_synchronous_entry.cc:220: int64
SimpleSynchronousEntry::GetFileSize() const {
Ordering: methods in a .cc file should be in the same order as a .h file.

https://codereview.chromium.org/13913010/diff/4004/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_synchronous_entry.h (right):

https://codereview.chromium.org/13913010/diff/4004/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_synchronous_entry.h:81: int64 GetFileSize() const;
Nice.

[felipeg, 2013-04-10 11:45:35]

https://chromiumcodereview.appspot.com/13913010/diff/4004/net/disk_cache/simp...
File net/disk_cache/simple/simple_entry_impl.cc (right):

https://chromiumcodereview.appspot.com/13913010/diff/4004/net/disk_cache/simp...
net/disk_cache/simple/simple_entry_impl.cc:269: if (sync_entry && index)
On 2013/04/10 10:33:23, gavinp wrote:
> if (index)

Done.

https://chromiumcodereview.appspot.com/13913010/diff/4004/net/disk_cache/simp...
File net/disk_cache/simple/simple_entry_impl.h (right):

https://chromiumcodereview.appspot.com/13913010/diff/4004/net/disk_cache/simp...
net/disk_cache/simple/simple_entry_impl.h:102: base::WeakPtr<SimpleIndex> index,
On 2013/04/10 10:33:23, gavinp wrote:
> What's the right thing to do when an operation finishes after the simple index
> has been deleted?

We do nothing, the index will be stale on disk.
What can we do ?

The next operation on that entry will update the entry data in the index or the
garbage collector will take care of refreshing the whole index.

https://chromiumcodereview.appspot.com/13913010/diff/4004/net/disk_cache/simp...
File net/disk_cache/simple/simple_index.cc (right):

https://chromiumcodereview.appspot.com/13913010/diff/4004/net/disk_cache/simp...
net/disk_cache/simple/simple_index.cc:116: // It will be updated later when the
SynchronousEntryImpl finish and the
On 2013/04/10 10:33:23, gavinp wrote:
> Wording suggestion: It will be updated later when the SimpleEntryImpl finishes
> opening or creating the new entry, and then UpdateEntrySize will be called.
> 
> Since the call comes from SimpleEntryImpl which is supposed to proxy for
> SynchronousEntryImpl, this is conceptually easier for me. Also it avoids
> mentioning threads.

Done.

https://chromiumcodereview.appspot.com/13913010/diff/4004/net/disk_cache/simp...
net/disk_cache/simple/simple_index.cc:163: // TODO(felipeg,gavinp): Fix this
once we have a one-file per entry format.
On 2013/04/10 10:33:23, gavinp wrote:
> COMPILE_ASSERT(kSimpleEntryFileCount == 3,
file_pattern_must_match_file_count);

Done.

https://chromiumcodereview.appspot.com/13913010/diff/4004/net/disk_cache/simp...
net/disk_cache/simple/simple_index.cc:164: const base::FilePath::StringType
file_pattern = FILE_PATH_LITERAL("*_[0-2]");
On 2013/04/10 10:33:23, gavinp wrote:
> Would: ??????????_[0-2]
> 
> Be a good idea?

It doesn't matter, we will change it anyway

https://chromiumcodereview.appspot.com/13913010/diff/4004/net/disk_cache/simp...
net/disk_cache/simple/simple_index.cc:190: EntrySet::iterator it =
entries_set_.find(hash_key);
On 2013/04/10 10:33:23, gavinp wrote:
> Note that in the not found case (33% of enumerations) you're doing two
searches
> in the index. The truly cycle-counting coder writes:
> 
> std::pair<EntrySet::iterator, bool> insert_result =
> entries_set_.insert(hash_key);
> if (insert_result.second) {  // entry was inserted.
> ...
> } else {  // entry was found.
> ...
> }
> 
> Since nobody can ever read that style of code, and since InsertInternal does
its
> own insert, I think it's best to leave this as it is.

it is a hash map
two searches doesn't matter compared to the increased cost in code maintenance
of having two different ways of inserting in the index

https://chromiumcodereview.appspot.com/13913010/diff/4004/net/disk_cache/simp...
File net/disk_cache/simple/simple_index.h (right):

https://chromiumcodereview.appspot.com/13913010/diff/4004/net/disk_cache/simp...
net/disk_cache/simple/simple_index.h:48: void Cleanup();
On 2013/04/10 10:33:23, gavinp wrote:
> While we're here, can we comment this method? I forgot what it did while I was
> reading this review. :/

I changed its name in the other CL

https://chromiumcodereview.appspot.com/13913010/diff/4004/net/disk_cache/simp...
net/disk_cache/simple/simple_index.h:50: bool UpdateEntrySize(const std::string&
key, uint64 entry_size);
On 2013/04/10 10:33:23, gavinp wrote:
> I think this deserves a short comment describing what it does. 

Done.

https://chromiumcodereview.appspot.com/13913010/diff/4004/net/disk_cache/simp...
File net/disk_cache/simple/simple_synchronous_entry.h (right):

https://chromiumcodereview.appspot.com/13913010/diff/4004/net/disk_cache/simp...
net/disk_cache/simple/simple_synchronous_entry.h:81: int64 GetFileSize() const;
On 2013/04/10 10:33:23, gavinp wrote:
> Nice.

Done.

[gavinp, 2013-04-10 14:49:29]

LGTM.

I suggest a CL getting the method order correct in the .cc file as soon as
in-flight changes to the index are settled.

https://chromiumcodereview.appspot.com/13913010/diff/4004/net/disk_cache/simp...
File net/disk_cache/simple/simple_index.cc (right):

https://chromiumcodereview.appspot.com/13913010/diff/4004/net/disk_cache/simp...
net/disk_cache/simple/simple_index.cc:190: EntrySet::iterator it =
entries_set_.find(hash_key);
On 2013/04/10 11:45:35, felipeg1 wrote:
> On 2013/04/10 10:33:23, gavinp wrote:
> > Note that in the not found case (33% of enumerations) you're doing two
> searches
> > in the index. The truly cycle-counting coder writes:
> > 
> > std::pair<EntrySet::iterator, bool> insert_result =
> > entries_set_.insert(hash_key);
> > if (insert_result.second) {  // entry was inserted.
> > ...
> > } else {  // entry was found.
> > ...
> > }
> > 
> > Since nobody can ever read that style of code, and since InsertInternal does
> its
> > own insert, I think it's best to leave this as it is.
> 
> it is a hash map
> two searches doesn't matter compared to the increased cost in code maintenance
> of having two different ways of inserting in the index

Good point on the hash.

Note also that every time I've written the "single-search" insert, I've had to
remove it in review. So sharing it in reviews like this is the only time I ever
get to talk about it.

https://chromiumcodereview.appspot.com/13913010/diff/4004/net/disk_cache/simp...
File net/disk_cache/simple/simple_synchronous_entry.h (right):

https://chromiumcodereview.appspot.com/13913010/diff/4004/net/disk_cache/simp...
net/disk_cache/simple/simple_synchronous_entry.h:81: int64 GetFileSize() const;
On 2013/04/10 11:45:35, felipeg1 wrote:
> On 2013/04/10 10:33:23, gavinp wrote:
> > Nice.
> 
> Done.

Done!

[pasko-google - do not use, 2013-04-10 15:26:54]

It would be nice to have a test to create a few entries with the simple backend,
write some data to them and then check the size. Index re-creation test would
also be good to test with sizes.

DataSizeFromKeyAndFileSize() is not accessible to the test, can emulate it.

[gavinp, 2013-04-10 15:33:26]

On 2013/04/10 15:26:54, pasko wrote:
> It would be nice to have a test to create a few entries with the simple
backend,
> write some data to them and then check the size. Index re-creation test would
> also be good to test with sizes.
> 
> DataSizeFromKeyAndFileSize() is not accessible to the test, can emulate it.

Or move it into simple_disk_format.h or simple_util.h

[commit-bot: I haz the power, 2013-04-10 15:58:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/felipeg@chromium.org/13913010/34002

[commit-bot: I haz the power, 2013-04-10 17:59:29]

Change committed as 193419