Disk cache: Read the index and data_0 files in a single...

Disk cache: Read the index and data_0 files in a single
operation to reduce multiple IO operations generated by
on-demand paging.

Also, generate extra histograms and improve the scale of
some others.

BUG=none
TEST=net_unittests



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

[Paweł Hajdan Jr., 2010-07-14 00:21:02]

Drive-by with a minor test comment.

http://codereview.chromium.org/2891022/diff/1/9
File net/disk_cache/disk_cache_test_util.h (right):

http://codereview.chromium.org/2891022/diff/1/9#newcode25
net/disk_cache/disk_cache_test_util.h:25: bool CopyTestCache(const std::wstring&
name);
nit: Why std::wstring and not FilePath? Inside this function we're using
FromWStringHack, which would be good to avoid.

[rvargas (out of office), 2010-07-14 01:11:44]

http://codereview.chromium.org/2891022/diff/1/9
File net/disk_cache/disk_cache_test_util.h (right):

http://codereview.chromium.org/2891022/diff/1/9#newcode25
net/disk_cache/disk_cache_test_util.h:25: bool CopyTestCache(const std::wstring&
name);
On 2010/07/14 00:21:03, Paweł Hajdan Jr. wrote:
> nit: Why std::wstring and not FilePath? Inside this function we're using
> FromWStringHack, which would be good to avoid.

I'm just moving the function from one file to another... so yes, I could also
change all callers and upgrade the prototype, but being a function to simplify
the tests I much rather not use
ASSERT_TRUE(CopyTestCache(FilePath(FILE_PATH_LITERAL("the fileset that I
want")))); everywhere.

[Paweł Hajdan Jr., 2010-07-14 01:17:38]

Removing myself from reviewers.

http://codereview.chromium.org/2891022/diff/1/9
File net/disk_cache/disk_cache_test_util.h (right):

http://codereview.chromium.org/2891022/diff/1/9#newcode25
net/disk_cache/disk_cache_test_util.h:25: bool CopyTestCache(const std::wstring&
name);
On 2010/07/14 01:11:44, rvargas wrote:
> On 2010/07/14 00:21:03, Paweł Hajdan Jr. wrote:
> > nit: Why std::wstring and not FilePath? Inside this function we're using
> > FromWStringHack, which would be good to avoid.
> 
> I'm just moving the function from one file to another... so yes, I could also
> change all callers and upgrade the prototype, but being a function to simplify
> the tests I much rather not use
> ASSERT_TRUE(CopyTestCache(FilePath(FILE_PATH_LITERAL("the fileset that I
> want")))); everywhere.

I see, didn't notice earlier that it was just a move. One alternative solution
you may want to consider is using just ASCII string, so you can use
FilePath::AppendASCII. I'd call that an improvement over wstring. Up to you.

[nsylvain, 2010-07-15 05:47:04]

LGTM , but still curious about the questions below

http://codereview.chromium.org/2891022/diff/12001/10003
File net/disk_cache/backend_impl.cc (right):

http://codereview.chromium.org/2891022/diff/12001/10003#newcode1722
net/disk_cache/backend_impl.cc:1722: CACHE_UMA(COUNTS_10000, "Size2", 0,
data_->header.num_bytes / (1024 * 1024));
What is COUNTS_10000 ?

http://codereview.chromium.org/2891022/diff/12001/10003#newcode1853
net/disk_cache/backend_impl.cc:1853: return index_->Read(buf.get(),
current_size, 0);
How much does it help? Or is this just an experiment?

http://codereview.chromium.org/2891022/diff/12001/10005
File net/disk_cache/block_files.cc (right):

http://codereview.chromium.org/2891022/diff/12001/10005#newcode524
net/disk_cache/block_files.cc:524: used -= header->empty[i] * (i + 1);
What does header->empty[i] contain? This looks a little bit weird that we
multiple by i+1, but i'm sure it's right ;)

[gavinp, 2010-07-15 15:03:48]

LGTM!

http://codereview.chromium.org/2891022/diff/12001/10005
File net/disk_cache/block_files.cc (right):

http://codereview.chromium.org/2891022/diff/12001/10005#newcode254
net/disk_cache/block_files.cc:254: size_t file_len = file->GetLength();
nit: const

http://codereview.chromium.org/2891022/diff/12001/10009
File net/disk_cache/disk_cache_test_util.h (right):

http://codereview.chromium.org/2891022/diff/12001/10009#newcode25
net/disk_cache/disk_cache_test_util.h:25: bool CopyTestCache(const std::string&
name);
Just a question: why are the const char*/std::string preferable to the wstring&
?  Just the initialization syntax?

[rvargas (out of office), 2010-07-15 18:31:50]

Thanks.

http://codereview.chromium.org/2891022/diff/12001/10003
File net/disk_cache/backend_impl.cc (right):

http://codereview.chromium.org/2891022/diff/12001/10003#newcode1722
net/disk_cache/backend_impl.cc:1722: CACHE_UMA(COUNTS_10000, "Size2", 0,
data_->header.num_bytes / (1024 * 1024));
On 2010/07/15 05:47:04, nsylvain wrote:
> What is COUNTS_10000 ?

Counts adjust the scale from 0 to 1 million. counts_10000 goes from 0 to 10000.
Given that the value is already MB, we have better resolution around interesting
values with the latter. It was an oversight / I thought COUNT was already 10k...
the downside is that now we need another name for the histogram because they are
different.

http://codereview.chromium.org/2891022/diff/12001/10003#newcode1853
net/disk_cache/backend_impl.cc:1853: return index_->Read(buf.get(),
current_size, 0);
On 2010/07/15 05:47:04, nsylvain wrote:
> How much does it help? Or is this just an experiment?

Hard to tell, because it only affects people with slow / overloaded systems (IO
wise). It reduces the number of IO reads from 4/8 to 1, but makes cache
initialization a little slower.

Another side benefit is that it should reduce a few crashes due to unreliable
hardware when we actually try to access the table (it *should* fail at this
point).

http://codereview.chromium.org/2891022/diff/12001/10005
File net/disk_cache/block_files.cc (right):

http://codereview.chromium.org/2891022/diff/12001/10005#newcode254
net/disk_cache/block_files.cc:254: size_t file_len = file->GetLength();
On 2010/07/15 15:03:48, gavinp wrote:
> nit: const

What do you mean?

if you mean const size_t file_len = x, we don't usually do that unless we are
declaring a constant

http://codereview.chromium.org/2891022/diff/12001/10005#newcode524
net/disk_cache/block_files.cc:524: used -= header->empty[i] * (i + 1);
On 2010/07/15 05:47:04, nsylvain wrote:
> What does header->empty[i] contain? This looks a little bit weird that we
> multiple by i+1, but i'm sure it's right ;)

empty is an array of 4 slots that keeps the number of empty blocks that can hold
an entry of the desired size:
entry[0] == x -> can store x entries of size 1
entry[3] == x -> can store x entries of size 4 (or 3, 2x of size 2 and 4x of
size 1)

http://codereview.chromium.org/2891022/diff/12001/10009
File net/disk_cache/disk_cache_test_util.h (right):

http://codereview.chromium.org/2891022/diff/12001/10009#newcode25
net/disk_cache/disk_cache_test_util.h:25: bool CopyTestCache(const std::string&
name);
On 2010/07/15 15:03:48, gavinp wrote:
> Just a question: why are the const char*/std::string preferable to the
wstring&
> ?  Just the initialization syntax?

They are basically the same (to me)... I just did it because Pawel cares about
it... it saves a few bytes on the executable (3x for mac/linux) so for non-test
code there is a real advantage, and we should try to have a FilePath as soon as
possible.