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

Issue 2827043: Disk cache: Switch the disk cache to use the cache_thread. ... (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 9 months ago by rvargas (OOO until 4_21)
Modified:
2 years, 10 months ago
CC:
chromium-reviews_chromium.org, pam+watch_chromium.org, cbentzel+watch_chromium.org, darin-cc_chromium.org, John Grabowski, PaweĊ‚ Hajdan Jr.
Visibility:
Public.

Description

Disk cache: Switch the disk cache to use the cache_thread.

Add an InFlightBackendIO class that handles posting of
cacheoperations back and forth between the IO thread and
the cachethread.

BUG=26730
TEST=unit tests


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

Patch Set 1 #

Patch Set 2 : ... and the fix #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+1358 lines, -859 lines) Lint Patch
M net/disk_cache/backend_impl.h View 10 chunks +40 lines, -9 lines 0 comments 0 errors Download
M net/disk_cache/backend_impl.cc View 1 16 chunks +234 lines, -94 lines 0 comments 0 errors Download
M net/disk_cache/backend_unittest.cc View 1 13 chunks +70 lines, -15 lines 2 comments 0 errors Download
M net/disk_cache/disk_cache_test_base.h View 4 chunks +7 lines, -1 line 0 comments 0 errors Download
M net/disk_cache/disk_cache_test_base.cc View 5 chunks +26 lines, -7 lines 0 comments 1 errors Download
M net/disk_cache/entry_impl.h View 2 chunks +15 lines, -2 lines 0 comments 1 errors Download
M net/disk_cache/entry_impl.cc View 13 chunks +117 lines, -40 lines 0 comments 0 errors Download
M net/disk_cache/entry_unittest.cc View 12 chunks +33 lines, -25 lines 0 comments 0 errors Download
M net/disk_cache/eviction.cc View 3 chunks +3 lines, -3 lines 0 comments 0 errors Download
M net/disk_cache/file_posix.cc View 4 chunks +11 lines, -2 lines 0 comments 0 errors Download
A net/disk_cache/in_flight_backend_io.h View 1 chunk +200 lines, -0 lines 0 comments 2 errors Download
A net/disk_cache/in_flight_backend_io.cc View 1 chunk +448 lines, -0 lines 0 comments 1 errors Download
A + net/disk_cache/in_flight_io.h View 4 chunks +56 lines, -301 lines 0 comments 1 errors Download
A + net/disk_cache/in_flight_io.cc View 2 chunks +27 lines, -333 lines 0 comments 0 errors Download
M net/disk_cache/sparse_control.cc View 5 chunks +9 lines, -9 lines 0 comments 0 errors Download
M net/disk_cache/stress_cache.cc View 1 chunk +0 lines, -1 line 0 comments 0 errors Download
M net/net.gyp View 1 chunk +4 lines, -0 lines 0 comments 0 errors Download
M net/tools/crash_cache/crash_cache.cc View 12 chunks +25 lines, -5 lines 0 comments 0 errors Download
M net/tools/dump_cache/upgrade.cc View 6 chunks +33 lines, -12 lines 0 comments 0 errors Download
Commit:

Messages

Total messages: 4
rvargas (OOO until 4_21)
Once again.
3 years, 9 months ago #1
gavinp
LGTM with a question http://codereview.chromium.org/2827043/diff/19001/20003 File net/disk_cache/backend_unittest.cc (right): http://codereview.chromium.org/2827043/diff/19001/20003#newcode405 net/disk_cache/backend_unittest.cc:405: // On a slow system ...
3 years, 9 months ago #2
nsylvain
lg
3 years, 9 months ago #3
rvargas (OOO until 4_21)
3 years, 9 months ago #4
Thanks.

http://codereview.chromium.org/2827043/diff/19001/20003
File net/disk_cache/backend_unittest.cc (right):

http://codereview.chromium.org/2827043/diff/19001/20003#newcode405
net/disk_cache/backend_unittest.cc:405: // On a slow system we may end up
evicting everything not currently in use.
On 2010/07/08 02:18:14, gavinp wrote:
> Does reordering the assert for OpenEntry(second) and OpenEntry(first) make a
> difference?  Why?

The problem with the old code is that it tests that only one entry (the oldest
one) is evicted. Eviction actually happens when the max_size limit is reached,
when an entry is charged by the space. In this case, it happens when "second" is
flushed from it's destructor... in other words, when the posted Close() executes
on the cache thread.

The issue with a slow system (valgrind) is that eviction of a single entry takes
more than 20 ms so we assume that we must keep working on that, and we post a
task to continue the eviction process... and given that we now have two threads,
that second task will actually run and evict the second entry so we'll fail to
open it.

By opening "second" right away, we don't give the eviction code a chance to
evict it, because when that posted task to evict runs, the entry is in use and
will not be evicted.

The sad thing is that now that we have two threads (and control for only one),
there are a lot of tests that are quite delicate in the order of events needed
to trigger the behavior under test :(
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1275:d14800f88434