Chromium Code Reviews
Help | Chromium Project | Sign in
(387)

Issue 2891022: Disk cache: Read the index and data_0 files in a single... (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 10 months ago by rvargas (out of office)
Modified:
3 years, 11 months ago
CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 3

Patch Set 2 : '' #

Total comments: 10
Unified diffs Side-by-side diffs Delta from patch set Stats (+174 lines, -86 lines) Patch
M net/disk_cache/addr.h View 1 chunk +1 line, -1 line 0 comments Download
M net/disk_cache/backend_impl.cc View 4 chunks +11 lines, -5 lines 4 comments Download
M net/disk_cache/backend_unittest.cc View 1 28 chunks +51 lines, -71 lines 0 comments Download
M net/disk_cache/block_files.h View 3 chunks +7 lines, -0 lines 0 comments Download
M net/disk_cache/block_files.cc View 6 chunks +65 lines, -9 lines 4 comments Download
M net/disk_cache/block_files_unittest.cc View 1 1 chunk +22 lines, -0 lines 0 comments Download
M net/disk_cache/disk_cache_test_util.h View 1 1 chunk +3 lines, -0 lines 2 comments Download
M net/disk_cache/disk_cache_test_util.cc View 1 1 chunk +14 lines, -0 lines 0 comments Download
Commit: CQ not working?

Messages

Total messages: 7 (0 generated)
rvargas (out of office)
4 years, 10 months ago (2010-07-13 23:01:03 UTC) #1
Paweł Hajdan Jr.
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& ...
4 years, 10 months ago (2010-07-14 00:21:02 UTC) #2
rvargas (out of office)
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 ...
4 years, 10 months ago (2010-07-14 01:11:44 UTC) #3
Paweł Hajdan Jr.
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 ...
4 years, 10 months ago (2010-07-14 01:17:38 UTC) #4
nsylvain
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: ...
4 years, 10 months ago (2010-07-15 05:47:04 UTC) #5
gavinp
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 ...
4 years, 10 months ago (2010-07-15 15:03:48 UTC) #6
rvargas (out of office)
4 years, 10 months ago (2010-07-15 18:31:50 UTC) #7
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld ec887be