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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 11 months ago by rvargas (out of office)
Modified:
3 years, 11 months ago
CC:
chromium-reviews, 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) Patch
M net/disk_cache/backend_impl.h View 10 chunks +40 lines, -9 lines 0 comments Download
M net/disk_cache/backend_impl.cc View 1 16 chunks +234 lines, -94 lines 0 comments Download
M net/disk_cache/backend_unittest.cc View 1 13 chunks +70 lines, -15 lines 2 comments Download
M net/disk_cache/disk_cache_test_base.h View 4 chunks +7 lines, -1 line 0 comments Download
M net/disk_cache/disk_cache_test_base.cc View 5 chunks +26 lines, -7 lines 0 comments Download
M net/disk_cache/entry_impl.h View 2 chunks +15 lines, -2 lines 0 comments Download
M net/disk_cache/entry_impl.cc View 13 chunks +117 lines, -40 lines 0 comments Download
M net/disk_cache/entry_unittest.cc View 12 chunks +33 lines, -25 lines 0 comments Download
M net/disk_cache/eviction.cc View 3 chunks +3 lines, -3 lines 0 comments Download
M net/disk_cache/file_posix.cc View 4 chunks +11 lines, -2 lines 0 comments Download
A net/disk_cache/in_flight_backend_io.h View 1 chunk +200 lines, -0 lines 0 comments Download
A net/disk_cache/in_flight_backend_io.cc View 1 chunk +448 lines, -0 lines 0 comments Download
A + net/disk_cache/in_flight_io.h View 4 chunks +56 lines, -301 lines 0 comments Download
A + net/disk_cache/in_flight_io.cc View 2 chunks +27 lines, -333 lines 0 comments Download
M net/disk_cache/sparse_control.cc View 5 chunks +9 lines, -9 lines 0 comments Download
M net/disk_cache/stress_cache.cc View 1 chunk +0 lines, -1 line 0 comments Download
M net/net.gyp View 1 chunk +4 lines, -0 lines 0 comments Download
M net/tools/crash_cache/crash_cache.cc View 12 chunks +25 lines, -5 lines 0 comments Download
M net/tools/dump_cache/upgrade.cc View 6 chunks +33 lines, -12 lines 0 comments Download
Commit: CQ not working?

Messages

Total messages: 4 (0 generated)
rvargas (out of office)
Once again.
4 years, 10 months ago (2010-07-08 00:10:36 UTC) #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 ...
4 years, 10 months ago (2010-07-08 02:18:13 UTC) #2
nsylvain
lg
4 years, 10 months ago (2010-07-08 02:56:56 UTC) #3
rvargas (out of office)
4 years, 10 months ago (2010-07-08 17:30:49 UTC) #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 ec887be