Http cache: Add support for a dedicated cache thread.

Http cache: Add support for a dedicated cache thread.

This is an interface-only change, nothing is really moving to another thread yet.

BUG=26730
TEST=none



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

[michaeln, 2010-03-18 06:20:59]

i'll look closer tomorrow

http://codereview.chromium.org/983007/diff/1/13
File webkit/appcache/appcache_disk_cache.cc (right):

http://codereview.chromium.org/983007/diff/1/13#newcode108
webkit/appcache/appcache_disk_cache.cc:108: cache_type, cache_directory,
cache_size, force, NULL,
can you put a...
// TODO(michaeln): pass in a valid cache_thread ptr
... here

[michaeln, 2010-03-18 19:57:34]

LGTM 

Maybe update the description to indicate that your just putting in the
programming interface at this point, the additional parameter, and that actual
support for offloading the IO operations to a separate thread will come later.

(thnx for adding the TODO for me)

[michaeln, 2010-03-18 19:58:21]

oh... i also had this question about the API that could be cleared up in the doc
comment

http://codereview.chromium.org/983007/diff/20001/21005
File net/disk_cache/disk_cache.h (right):

http://codereview.chromium.org/983007/diff/20001/21005#newcode61
net/disk_cache/disk_cache.h:61: // operations if a dedicated thread is required.
The returned pointer can be
Is it ok to pass NULL for thread, and if so, what does that mean... is the
current thread used in that case? From the doc comments, it looks like that's
the case, but it may be good to clarify things.

[rvargas (doing something else), 2010-03-18 20:47:57]

Michael, thanks for the review.

Eric, your turn :)

http://codereview.chromium.org/983007/diff/20001/21005
File net/disk_cache/disk_cache.h (right):

http://codereview.chromium.org/983007/diff/20001/21005#newcode61
net/disk_cache/disk_cache.h:61: // operations if a dedicated thread is required.
The returned pointer can be
On 2010/03/18 19:58:27, michaeln wrote:
> Is it ok to pass NULL for thread, and if so, what does that mean... is the
> current thread used in that case? From the doc comments, it looks like that's
> the case, but it may be good to clarify things.

I updated the comment to show that passing NULL is not really supported (except
for a memory-only backend). I don't want to leave a thread hanging around in the
product without being used while I put everything together so that's why
everybody passes NULL right now.

[michaeln, 2010-03-18 21:15:21]

> I updated the comment to show that passing NULL is not really supported
(except
> for a memory-only backend). I don't want to leave a thread hanging around in
the
> product without being used while I put everything together so that's why
> everybody passes NULL right now.

Ok, thanx for the clarification.

[michaeln, 2010-03-18 21:38:17]

> Ok, thanx for the clarification.

QQ: What if you were to pass in the IO thread's message loop (so the msgloop of
the thread the backend API is called on) will it work or not?

[rvargas (doing something else), 2010-03-18 21:50:26]

On 2010/03/18 21:38:17, michaeln wrote:
> > Ok, thanx for the clarification.
> 
> QQ: What if you were to pass in the IO thread's message loop (so the msgloop
of
> the thread the backend API is called on) will it work or not?

I though about that when figuring out what would be the best API (passing a
message loop or creating another thread from within the disk cache code,
allowing the use of the same thread either by null or  MessageLoop::current()
etc).

The actual implementation of the cache construction/destruction has some issues
if the cache_thread is the IO loop, although to be fair, there is a line of code
to handle that case for some unit tests. (details: the disk cache destructor
blocks on an event until work is done on the background thread, and there are
some PostTasks in between)

That said, it will mostly work (aka, maybe everything works).

[eroman, 2010-03-19 20:07:30]

I wander if it wouldn't be better to simplify the constructors of HttpCache, and
always require passing in the Backend* ?

We already cache several temporary values solely for the purpose of creating the
backend later (disk_cache_dir_, cache_size_, type_), and now the cache_thread. I
think it would be cleaner if the caller created the Backend with the appropriate
parameters (the disk-based ones could take the message loop argument), and
passed it into the constructor.

http://codereview.chromium.org/983007/diff/29001/30003
File net/http/http_cache.h (right):

http://codereview.chromium.org/983007/diff/29001/30003#newcode79
net/http/http_cache.h:79: MessageLoop* cache_thread,
minor nit: I wander if it would be more accurate to call this cache_loop or
cache_message_loop ? I am OK with leaving it as cache_thread if you think that
is clearer.

[rvargas (doing something else), 2010-03-19 21:31:48]

Interesting idea.

I think we can collapse the constructors so that they always pass the same
arguments (regardless of the cache type), if that helps.

I believe the original reasoning was to hide the backend behind the http
cache... in fact, behind the "network" interface, hoping that nobody outside of
net would ever need to know details about it (not really the case today, but
almost).

A more serious concern is that the interface for creating a backend is more
complex than the one of the http cache: it requires a callback to be notified
when the object is created because it will create/open a few files (or even
open/read/close/delete/create/write). That functionality will be provided by the
http cache, leaving ChromeUrlRequestContext with a much simpler job... and there
is also the issue that creation happens lazily with the first request so
requiring that work before calling the http constructor will slow down start up.

http://codereview.chromium.org/983007/diff/29001/30003
File net/http/http_cache.h (right):

http://codereview.chromium.org/983007/diff/29001/30003#newcode79
net/http/http_cache.h:79: MessageLoop* cache_thread,
On 2010/03/19 20:07:30, eroman wrote:
> minor nit: I wander if it would be more accurate to call this cache_loop or
> cache_message_loop ? I am OK with leaving it as cache_thread if you think that
> is clearer.

Yeah... I thought about that. I think it is clearer to refer to that as a
thread, even though it is a message loop (conceptually we want the dedicated
thread to use, but the simplest thing to pass is the message loop; it would have
been nice if PostTask was a member of the thread... but nobody passes threads
around).

[eroman, 2010-03-19 23:48:32]

As far as the issue initializing backend, won't the interface for backend need
to be async, so that initialization could happen lazily during one of the async
calls?

Anyhow, I won't block this change from happening, just voicing my ideas.

LGTM.

[rvargas (doing something else), 2010-04-29 19:42:44]

I finally checked in this cl.

To answer your last question, yes, the backend interface for creating the cache
is async, and all methods that could block are also async.

The flow goes like this:

1. ChromeURLRequestContext creates the http synchronously at startup.
2. With the first request, the http cache starts the creation of the backend
(when creating the HttpTransaction). It uses an internal completion callback.
3. All transactions that reach Start before the backend is created will be
queued by the http cache (seems safer than just bypassing the cache).
4. When the callback is notified, resume processing of queued TXs. Note that a
possible outcome of this step is that there is no backend at all (and the
session will proceed without a cache).

The downside is that anybody accessing directly the backend must implement a
callback to deal with the async creation. That's the case of the browser data
cleaner and the view-cache implementation.

Of course, this is yet to be implemented.

Thanks again.

On 2010/03/19 23:48:32, eroman wrote:
> As far as the issue initializing backend, won't the interface for backend need
> to be async, so that initialization could happen lazily during one of the
async
> calls?
> 
> Anyhow, I won't block this change from happening, just voicing my ideas.
> 
> LGTM.