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

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=51312

[rvargas (doing something else), 2010-06-18 01:52:12]

As I mentioned before, this CL will be followed by a cleanup one that
removes/moves methods around

http://codereview.chromium.org/2829008/diff/16001/17007
File net/disk_cache/entry_impl.h (right):

http://codereview.chromium.org/2829008/diff/16001/17007#newcode56
net/disk_cache/entry_impl.h:56: // Background implementation of the Entry
interface.
We could switch to use XxImpl for all these methods instead of the background
thread prefix... I started with the backend (SyncXx), but most of these methods
are not actually synchronous so we cannot use that here... and we cannot really
use Impl for everything on the backend because we also need the real
implementation for some methods (due to the different signature)... although we
could use BG also for the backend.

[tonyg, 2010-06-18 17:35:13]

This is a pretty major architecture change. It would take me quite some time
to understand this code enough to give it any sort of meaningful review. I'm
also not sure I want to build up the necessary state on the guts of the disk
cache.

Is there anyone else who has more state on this code? If not, I can give it
a somewhat superficial review, but am unlikely to understand deeper issues.

On Thu, Jun 17, 2010 at 6:52 PM, <rvargas@chromium.org> wrote:

> Reviewers: tonyg,
>
> Message:
> As I mentioned before, this CL will be followed by a cleanup one that
> removes/moves methods around
>
>
> http://codereview.chromium.org/2829008/diff/16001/17007
> File net/disk_cache/entry_impl.h (right):
>
> http://codereview.chromium.org/2829008/diff/16001/17007#newcode56
> net/disk_cache/entry_impl.h:56: // Background implementation of the
> Entry interface.
> We could switch to use XxImpl for all these methods instead of the
> background thread prefix... I started with the backend (SyncXx), but
> most of these methods are not actually synchronous so we cannot use that
> here... and we cannot really use Impl for everything on the backend
> because we also need the real implementation for some methods (due to
> the different signature)... although we could use BG also for the
> backend.
>
> Description:
> Disk cache: Switch the disk cache to use the cache_thread.
>
> Add an InFlightBackendIO class that handles posting of cache
> operations back and forth between the IO thread and the cache
> thread.
>
> BUG=26730
> TEST=unit tests
>
>
>
> Please review this at http://codereview.chromium.org/2829008/show
>
> SVN Base: svn://chrome-svn/chrome/trunk/src/
>
> Affected files:
>  M     net/disk_cache/backend_impl.h
>  M     net/disk_cache/backend_impl.cc
>  M     net/disk_cache/backend_unittest.cc
>  M     net/disk_cache/disk_cache_test_base.h
>  M     net/disk_cache/disk_cache_test_base.cc
>  M     net/disk_cache/entry_impl.h
>  M     net/disk_cache/entry_impl.cc
>  M     net/disk_cache/entry_unittest.cc
>  M     net/disk_cache/eviction.cc
>  A     net/disk_cache/in_flight_backend_io.h
>  A     net/disk_cache/in_flight_backend_io.cc
>  A  +  net/disk_cache/in_flight_io.h
>  A  +  net/disk_cache/in_flight_io.cc
>  M     net/disk_cache/sparse_control.cc
>  M     net/disk_cache/stress_cache.cc
>  M     net/net.gyp
>  M     net/tools/crash_cache/crash_cache.cc
>  M     net/tools/dump_cache/upgrade.cc
>
>
>

[rvargas (doing something else), 2010-06-18 18:22:19]

Nicolas has reviewed most of the changes to the disk cache, but he has been
gradually moving away from coding for a while now, and I have the feeling
that he would prefer not being the designated reviewer any more.

Gavin has also expressed interest in the cache so maybe he could be the one
to get to know this code.

Eric is my target reviewer for the http cache, so he has reviewed most of
the related changes, but I guess he's as unfamiliar with the disk cache as
anybody else.

On Fri, Jun 18, 2010 at 10:34 AM, Tony Gentilcore <tonyg@chromium.org>wrote:

> This is a pretty major architecture change. It would take me quite some
> time to understand this code enough to give it any sort of meaningful
> review. I'm also not sure I want to build up the necessary state on the guts
> of the disk cache.
>
> Is there anyone else who has more state on this code? If not, I can give it
> a somewhat superficial review, but am unlikely to understand deeper issues.
>
> On Thu, Jun 17, 2010 at 6:52 PM, <rvargas@chromium.org> wrote:
>
>> Reviewers: tonyg,
>>
>> Message:
>> As I mentioned before, this CL will be followed by a cleanup one that
>> removes/moves methods around
>>
>>
>> http://codereview.chromium.org/2829008/diff/16001/17007
>> File net/disk_cache/entry_impl.h (right):
>>
>> http://codereview.chromium.org/2829008/diff/16001/17007#newcode56
>> net/disk_cache/entry_impl.h:56: // Background implementation of the
>> Entry interface.
>> We could switch to use XxImpl for all these methods instead of the
>> background thread prefix... I started with the backend (SyncXx), but
>> most of these methods are not actually synchronous so we cannot use that
>> here... and we cannot really use Impl for everything on the backend
>> because we also need the real implementation for some methods (due to
>> the different signature)... although we could use BG also for the
>> backend.
>>
>> Description:
>> Disk cache: Switch the disk cache to use the cache_thread.
>>
>> Add an InFlightBackendIO class that handles posting of cache
>> operations back and forth between the IO thread and the cache
>> thread.
>>
>> BUG=26730
>> TEST=unit tests
>>
>>
>>
>> Please review this at http://codereview.chromium.org/2829008/show
>>
>> SVN Base: svn://chrome-svn/chrome/trunk/src/
>>
>> Affected files:
>>  M     net/disk_cache/backend_impl.h
>>  M     net/disk_cache/backend_impl.cc
>>  M     net/disk_cache/backend_unittest.cc
>>  M     net/disk_cache/disk_cache_test_base.h
>>  M     net/disk_cache/disk_cache_test_base.cc
>>  M     net/disk_cache/entry_impl.h
>>  M     net/disk_cache/entry_impl.cc
>>  M     net/disk_cache/entry_unittest.cc
>>  M     net/disk_cache/eviction.cc
>>  A     net/disk_cache/in_flight_backend_io.h
>>  A     net/disk_cache/in_flight_backend_io.cc
>>  A  +  net/disk_cache/in_flight_io.h
>>  A  +  net/disk_cache/in_flight_io.cc
>>  M     net/disk_cache/sparse_control.cc
>>  M     net/disk_cache/stress_cache.cc
>>  M     net/net.gyp
>>  M     net/tools/crash_cache/crash_cache.cc
>>  M     net/tools/dump_cache/upgrade.cc
>>
>>
>>
>

[nsylvain, 2010-06-23 21:09:28]

a few initial comments, in addition to the ones we talked about in person.

I haven't looked at inflightio yet.

http://codereview.chromium.org/2829008/diff/44001/32002
File net/disk_cache/backend_impl.cc (right):

http://codereview.chromium.org/2829008/diff/44001/32002#newcode428
net/disk_cache/backend_impl.cc:428: background_queue_.WaitForPendingIO();
is there some sort of timeout?

http://codereview.chromium.org/2829008/diff/44001/32002#newcode436
net/disk_cache/backend_impl.cc:436: done_.Wait();
same here

http://codereview.chromium.org/2829008/diff/44001/32002#newcode611
net/disk_cache/backend_impl.cc:611: entry->Release();
can bgdoom be executed after release and then it crashes?
--
Replying to myself: No?.
It looks like "BG" is like "Sync" in the sense that it won't call the thread. 
This confused my because we already had "sync" for that, and "BG" seems to say
that it will go to the background thread (instead of 'being already in the
background thread').  Not a big deal I guess.

http://codereview.chromium.org/2829008/diff/44001/32002#newcode629
net/disk_cache/backend_impl.cc:629: bool BackendImpl::DoomAllEntries() {
this runs on the background thread right? Can we add something to know on which
thread a function is supposed to run. It could be a comment, but if you can use
the mechanism to CHECK when a function is executed on the wrong thread, that'd
be even better.

http://codereview.chromium.org/2829008/diff/44001/32002#newcode762
net/disk_cache/backend_impl.cc:762: background_queue_.EndEnumeration(*iter);
no callback here?

http://codereview.chromium.org/2829008/diff/44001/32002#newcode1168
net/disk_cache/backend_impl.cc:1168: EntryImpl*
BackendImpl::OpenPrevEntryImpl(void** iter) {
I guess I missed that, but why do we switch from Entry* to EntryImpl* ?

http://codereview.chromium.org/2829008/diff/44001/32004
File net/disk_cache/backend_unittest.cc (right):

http://codereview.chromium.org/2829008/diff/44001/32004#newcode289
net/disk_cache/backend_unittest.cc:289: EXPECT_EQ(kSize, rv);
is that dead code? Can it ever go there and not fail this EXPECT_EQ?

[rvargas (doing something else), 2010-06-23 21:49:22]

Thanks.

http://codereview.chromium.org/2829008/diff/44001/32002
File net/disk_cache/backend_impl.cc (right):

http://codereview.chromium.org/2829008/diff/44001/32002#newcode428
net/disk_cache/backend_impl.cc:428: background_queue_.WaitForPendingIO();
On 2010/06/23 21:09:28, nsylvain wrote:
> is there some sort of timeout? 

No, there isn't. This is basically the same thing we do today (old file, line
348), except that now we have to coordinate two threads instead of one.

http://codereview.chromium.org/2829008/diff/44001/32002#newcode436
net/disk_cache/backend_impl.cc:436: done_.Wait();
On 2010/06/23 21:09:28, nsylvain wrote:
> same here

The idea is that we need to wait until any pending write completes before
closing the handles because otherwise the write may be abandoned and we may end
up trusting old data (aka, garbage) as valid.

http://codereview.chromium.org/2829008/diff/44001/32002#newcode611
net/disk_cache/backend_impl.cc:611: entry->Release();
On 2010/06/23 21:09:28, nsylvain wrote:
> can bgdoom be executed after release and then it crashes?
> --
> Replying to myself: No?.
> It looks like "BG" is like "Sync" in the sense that it won't call the thread. 
> This confused my because we already had "sync" for that, and "BG" seems to say
> that it will go to the background thread (instead of 'being already in the
> background thread').  Not a big deal I guess. 

Yeah. I just realized that a downside of adding cl comments is that you don't
see them when I upload a new version. I had a comment on the first revision
(first email) about naming.... so yes, BG == Sync == Impl in terms of not
switching threads and just being executed in the context of the caller. The
implication of BG being always on the background thread is basically a +1 to use
something else :(

http://codereview.chromium.org/2829008/diff/44001/32002#newcode629
net/disk_cache/backend_impl.cc:629: bool BackendImpl::DoomAllEntries() {
On 2010/06/23 21:09:28, nsylvain wrote:
> this runs on the background thread right? Can we add something to know on
which
> thread a function is supposed to run. It could be a comment, but if you can
use
> the mechanism to CHECK when a function is executed on the wrong thread, that'd
> be even better.

This is the old public interface so it doesn't know about which thread it should
run. The idea here is that I'm changing the code so that it is easier to review
(as in see what is changing), but it is not intended to stay that way. These
methods will disappear/move somewhere else... in this particular case, the
implementation should morph into SyncDoomAllEntries (or whatever naming
convention we come up), with the new fn signature.

I don't think I can add DCHECKS for the thread because that is just the intended
use, but it is possible (for instance, for the unit tests) to call a given
function on a different thread (maybe if the whole test is not using multiple
threads). I can add comments about that... and they will be kind of generic: the
public API is intended to be called on the "main" (IO) thread, and everything
else should run on the background thread.

Do you think that will be too confusing?

http://codereview.chromium.org/2829008/diff/44001/32002#newcode762
net/disk_cache/backend_impl.cc:762: background_queue_.EndEnumeration(*iter);
On 2010/06/23 21:09:28, nsylvain wrote:
> no callback here?

This is the public interface... the caller doesn't want to know when this is
done. And that looks like a bug because how will the caller know when to release
the pointer?

http://codereview.chromium.org/2829008/diff/44001/32002#newcode1168
net/disk_cache/backend_impl.cc:1168: EntryImpl*
BackendImpl::OpenPrevEntryImpl(void** iter) {
On 2010/06/23 21:09:28, nsylvain wrote:
> I guess I missed that, but why do we switch from Entry* to EntryImpl* ?

The *Impl is sort of a private interface (public from the implementation object,
not present on the pure interface), and returning the "real" object type removes
the need of reinterpret_casts between the two classes (as long as it is returned
as the result, not another argument).

http://codereview.chromium.org/2829008/diff/44001/32004
File net/disk_cache/backend_unittest.cc (right):

http://codereview.chromium.org/2829008/diff/44001/32004#newcode289
net/disk_cache/backend_unittest.cc:289: EXPECT_EQ(kSize, rv);
On 2010/06/23 21:09:28, nsylvain wrote:
> is that dead code? Can it ever go there and not fail this EXPECT_EQ?

yes?. If it succeeds (synchronously), rv == write size == kSize.

[nsylvain, 2010-06-23 22:00:10]

On Wed, Jun 23, 2010 at 2:49 PM, <rvargas@chromium.org> wrote:

> Thanks.
>
>
>
> http://codereview.chromium.org/2829008/diff/44001/32002
> File net/disk_cache/backend_impl.cc (right):
>
> http://codereview.chromium.org/2829008/diff/44001/32002#newcode428
> net/disk_cache/backend_impl.cc:428:
> background_queue_.WaitForPendingIO();
> On 2010/06/23 21:09:28, nsylvain wrote:
>
>> is there some sort of timeout?
>>
>
> No, there isn't. This is basically the same thing we do today (old file,
> line 348), except that now we have to coordinate two threads instead of
> one.
>
>
> http://codereview.chromium.org/2829008/diff/44001/32002#newcode436
> net/disk_cache/backend_impl.cc:436: done_.Wait();
> On 2010/06/23 21:09:28, nsylvain wrote:
>
>> same here
>>
>
> The idea is that we need to wait until any pending write completes
> before closing the handles because otherwise the write may be abandoned
> and we may end up trusting old data (aka, garbage) as valid.

ok

>
>
> http://codereview.chromium.org/2829008/diff/44001/32002#newcode611
> net/disk_cache/backend_impl.cc:611: entry->Release();
> On 2010/06/23 21:09:28, nsylvain wrote:
>
>> can bgdoom be executed after release and then it crashes?
>> --
>> Replying to myself: No?.
>> It looks like "BG" is like "Sync" in the sense that it won't call the
>>
> thread.
>
>> This confused my because we already had "sync" for that, and "BG"
>>
> seems to say
>
>> that it will go to the background thread (instead of 'being already in
>>
> the
>
>> background thread').  Not a big deal I guess.
>>
>
> Yeah. I just realized that a downside of adding cl comments is that you
> don't see them when I upload a new version. I had a comment on the first
> revision (first email) about naming.... so yes, BG == Sync == Impl in
> terms of not switching threads and just being executed in the context of
> the caller. The implication of BG being always on the background thread
> is basically a +1 to use something else :(

Ok.. Not sure what's best, but I least now I think I understand better ;)

>
>
> http://codereview.chromium.org/2829008/diff/44001/32002#newcode629
> net/disk_cache/backend_impl.cc:629: bool BackendImpl::DoomAllEntries() {
> On 2010/06/23 21:09:28, nsylvain wrote:
>
>> this runs on the background thread right? Can we add something to know
>>
> on which
>
>> thread a function is supposed to run. It could be a comment, but if
>>
> you can use
>
>> the mechanism to CHECK when a function is executed on the wrong
>>
> thread, that'd
>
>> be even better.
>>
>
> This is the old public interface so it doesn't know about which thread
> it should run. The idea here is that I'm changing the code so that it is
> easier to review (as in see what is changing), but it is not intended to
> stay that way. These methods will disappear/move somewhere else... in
> this particular case, the implementation should morph into
> SyncDoomAllEntries (or whatever naming convention we come up), with the
> new fn signature.
>
> I don't think I can add DCHECKS for the thread because that is just the
> intended use, but it is possible (for instance, for the unit tests) to
> call a given function on a different thread (maybe if the whole test is
> not using multiple threads). I can add comments about that... and they
> will be kind of generic: the public API is intended to be called on the
> "main" (IO) thread, and everything else should run on the background
> thread.
>
> Do you think that will be too confusing?

Maybe not.. Or maybe your following change will make that easier to
understand

>
>
> http://codereview.chromium.org/2829008/diff/44001/32002#newcode762
> net/disk_cache/backend_impl.cc:762:
> background_queue_.EndEnumeration(*iter);
> On 2010/06/23 21:09:28, nsylvain wrote:
>
>> no callback here?
>>
>
> This is the public interface... the caller doesn't want to know when
> this is done. And that looks like a bug because how will the caller know
> when to release the pointer?

You tell me ;)

>
>
> http://codereview.chromium.org/2829008/diff/44001/32002#newcode1168
> net/disk_cache/backend_impl.cc:1168: EntryImpl*
> BackendImpl::OpenPrevEntryImpl(void** iter) {
> On 2010/06/23 21:09:28, nsylvain wrote:
>
>> I guess I missed that, but why do we switch from Entry* to EntryImpl*
>>
> ?
>

> The *Impl is sort of a private interface (public from the implementation
> object, not present on the pure interface), and returning the "real"
> object type removes the need of reinterpret_casts between the two
> classes (as long as it is returned as the result, not another argument).
>
>
> http://codereview.chromium.org/2829008/diff/44001/32004
> File net/disk_cache/backend_unittest.cc (right):
>
> http://codereview.chromium.org/2829008/diff/44001/32004#newcode289
> net/disk_cache/backend_unittest.cc:289: EXPECT_EQ(kSize, rv);
> On 2010/06/23 21:09:28, nsylvain wrote:
>
>> is that dead code? Can it ever go there and not fail this EXPECT_EQ?
>>
>
> yes?. If it succeeds (synchronously), rv == write size == kSize.

For some reasons I thought it would always return PENDING.

>
>
> http://codereview.chromium.org/2829008/show
>

[nsylvain, 2010-06-25 20:57:49]

a few more comments

http://codereview.chromium.org/2829008/diff/44001/32012
File net/disk_cache/in_flight_backend_io.cc (right):

http://codereview.chromium.org/2829008/diff/44001/32012#newcode400
net/disk_cache/in_flight_backend_io.cc:400: if (!op->IsEntryOperation() &&
!pending_ops_.empty()) {
entry operations will never be added to pending_ops?

http://codereview.chromium.org/2829008/diff/44001/32012#newcode410
net/disk_cache/in_flight_backend_io.cc:410: op->callback()->Run(op->result());
so you call the callback for cancelled entry operations? Are they not
cancellable?

http://codereview.chromium.org/2829008/diff/44001/32013
File net/disk_cache/in_flight_backend_io.h (right):

http://codereview.chromium.org/2829008/diff/44001/32013#newcode69
net/disk_cache/in_flight_backend_io.h:69: // type (for instance opening an entry
and creating the same entry). On the
same time?

[rvargas (doing something else), 2010-06-25 21:22:57]

http://codereview.chromium.org/2829008/diff/44001/32002
File net/disk_cache/backend_impl.cc (right):

http://codereview.chromium.org/2829008/diff/44001/32002#newcode762
net/disk_cache/backend_impl.cc:762: background_queue_.EndEnumeration(*iter);
On 2010/06/23 21:49:22, rvargas wrote:
> On 2010/06/23 21:09:28, nsylvain wrote:
> > no callback here?
> 
> This is the public interface... the caller doesn't want to know when this is
> done. And that looks like a bug because how will the caller know when to
release
> the pointer?

OK, not a bug because we grab what we need (here) so that the caller can release
the memory.

http://codereview.chromium.org/2829008/diff/44001/32012
File net/disk_cache/in_flight_backend_io.cc (right):

http://codereview.chromium.org/2829008/diff/44001/32012#newcode400
net/disk_cache/in_flight_backend_io.cc:400: if (!op->IsEntryOperation() &&
!pending_ops_.empty()) {
On 2010/06/25 20:57:49, nsylvain wrote:
> entry operations will never be added to pending_ops?

nope. InFlightBackendIO::QueueOperation immediately posts any entry op to the
worker thread. The reason is that those operations are already async, and we
support having any number of simultaneous operations.

http://codereview.chromium.org/2829008/diff/44001/32012#newcode410
net/disk_cache/in_flight_backend_io.cc:410: op->callback()->Run(op->result());
On 2010/06/25 20:57:49, nsylvain wrote:
> so you call the callback for cancelled entry operations? Are they not
> cancellable?

In this context, cancel means that the object that initiated the operation is
going away. The contract for an entry is that the user can call Close() and
expect any pending callbacks to still be invoked.

In practice, it means that the cache object is going away, but the entries are
"kind of" owned by a transaction, not the cache... and they may outlive the
cache... but if they do, they'll notice that the cache is gone and just fail.

http://codereview.chromium.org/2829008/diff/44001/32013
File net/disk_cache/in_flight_backend_io.h (right):

http://codereview.chromium.org/2829008/diff/44001/32013#newcode69
net/disk_cache/in_flight_backend_io.h:69: // type (for instance opening an entry
and creating the same entry). On the
On 2010/06/25 20:57:49, nsylvain wrote:
> same time?

Done.

[nsylvain, 2010-06-28 15:54:57]

i dont think you uploaded the last version yet, but, as far as I can tell: LGTM

http://codereview.chromium.org/2829008/diff/44001/32012
File net/disk_cache/in_flight_backend_io.cc (right):

http://codereview.chromium.org/2829008/diff/44001/32012#newcode20
net/disk_cache/in_flight_backend_io.cc:20: my_callback_(this,
&BackendIO::OnIOComplete)) {
here you have 4 spaces. Somewhere else you have 2 spaces. I guess 4 is good. You
should make sure it's correct everywhere

[rvargas (doing something else), 2010-06-28 19:39:00]

Uploaded, thanks.

On 2010/06/28 15:54:57, nsylvain wrote:
> i dont think you uploaded the last version yet, but, as far as I can tell:
LGTM
> 
> http://codereview.chromium.org/2829008/diff/44001/32012
> File net/disk_cache/in_flight_backend_io.cc (right):
> 
> http://codereview.chromium.org/2829008/diff/44001/32012#newcode20
> net/disk_cache/in_flight_backend_io.cc:20: my_callback_(this,
> &BackendIO::OnIOComplete)) {
> here you have 4 spaces. Somewhere else you have 2 spaces. I guess 4 is good.
You
> should make sure it's correct everywhere

[nsylvain, 2010-06-28 23:58:49]

still lgtm with the new proxy change

http://codereview.chromium.org/2829008/diff/15005/4020
File net/tools/crash_cache/crash_cache.cc (right):

http://codereview.chromium.org/2829008/diff/15005/4020#newcode174
net/tools/crash_cache/crash_cache.cc:174: int rv =
disk_cache::CreateCacheBackend(net::MEDIA_CACHE, path, 0, false,
why?

[rvargas (doing something else), 2010-06-29 01:00:45]

Thanks

http://codereview.chromium.org/2829008/diff/15005/4020
File net/tools/crash_cache/crash_cache.cc (right):

http://codereview.chromium.org/2829008/diff/15005/4020#newcode174
net/tools/crash_cache/crash_cache.cc:174: int rv =
disk_cache::CreateCacheBackend(net::MEDIA_CACHE, path, 0, false,
On 2010/06/28 23:58:50, nsylvain wrote:
> why?

Added a comment. MEDIA_CACHE uses an LRU for evictions and this makes this code
go through the proper sequence of events.