Does a class for this exist: Post task to worker pool thread, then post results back to main thread

[Jói Sigurðsson]

Hi all,

Just checking if this exists in base.  I recently wrote a
special-purpose class like the one described in the subject line, and
now find myself needing the same thing for a different purpose, so I'm
planning to generalize it and move it to base.  There were some
non-trivial issues building such a class so I think it'd be useful as
a generalization, but I want to make sure such a thing doesn't already
exist before I go ahead.

Cheers,
Jói

[William Chan (陈智昌)]

I wrote something like this but never got it landed because I went on
vacation or something and never got back to it.

http://codereview.chromium.org/5710002/

ericroman and I are both very familiar with WorkerPool issues, so if
you do this, please send it to us for review.

> --
> Chromium Developers mailing list: chromi...@chromium.org
> View archives, change email options, or unsubscribe:
>    http://groups.google.com/a/chromium.org/group/chromium-dev
>

[Jói Sigurðsson]

Thanks Will. My special-purpose but probably easy to generalize
implementation is at net/proxy/dhcp_proxy_script_adapter_fetcher_win.h
and .cc (look for the WorkerThread inner class) and was already
reviewed by ericroman. The main difference to your implementation
seems to be that in mine I allow the eventual to be non-refcounted
(via weak pointers), whereas like yours mine would require inheritance
for providing the function that should run on the worker thread, and
the OnCompleted function that should run back on the originating
thread.

Do you want to revive your change and land it? Or would you prefer I
land an implementation that you could later switch the various classes
you touched in this change to? I need this generalization now so I'd
be happy to land something that is the best of both implementations
and use it immediately for my own purposes.

Cheers,
Jói

2011/6/14 William Chan (陈智昌) <will...@chromium.org>:

[William Chan (陈智昌)]

On Tue, Jun 14, 2011 at 11:58 PM, Jói Sigurðsson <j...@chromium.org> wrote:
> Thanks Will.  My special-purpose but probably easy to generalize
> implementation is at net/proxy/dhcp_proxy_script_adapter_fetcher_win.h
> and .cc (look for the WorkerThread inner class) and was already
> reviewed by ericroman.  The main difference to your implementation
> seems to be that in mine I allow the eventual to be non-refcounted
> (via weak pointers), whereas like yours mine would require inheritance
> for providing the function that should run on the worker thread, and
> the OnCompleted function that should run back on the originating
> thread.

Your implementation is buggy due to
http://code.google.com/p/chromium/issues/detail?id=82509.

>
> Do you want to revive your change and land it?  Or would you prefer I
> land an implementation that you could later switch the various classes
> you touched in this change to?  I need this generalization now so I'd
> be happy to land something that is the best of both implementations
> and use it immediately for my own purposes.

I would be happy to have you take over the task of generalizing it.
Please do so :)

[Jonathan Dixon]

On the face of it, it sounds like CancelableRequest should do exactly what you want

http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/content/browser/cancelable_request.h&exact_package=chromium&q=CancelableRequest&type=cs

The docs in that class even mention it's for sending work to a "background" thread and it would be easy to imagine the worker pool is the canonical example of background threads.

I guess it's not that simple due to the never-exit nature of the worker pool?

Would it be possible/desirable to enhance that mechanism to also work with the worker pool, rather than create a new one?

[William Chan (陈智昌)]

FWIW, we're creating a PostTaskAndReply() mechanism for
MessageLoop[Proxy] that handles posting a task to a target thread and
getting the reply on the origin thread. It's intended to replace the
multiple implementations of FileUtilProxy and maybe CancelableRequest.
It's conceivable we could extend this for WorkerPool. Albert's
recently freed up responsibilities so I hope that he will get to this
soon, amongst our other base::Callback related tasks. I don't know if
this works for Joi's timeline.

[Jói Sigurðsson]

Thanks guys, I'll take a look at those possibilities before building anything new.

(sent from phone, pardon brevity)

[Jonathan Dixon]

On 15 June 2011 15:23, Jói Sigurðsson <j...@chromium.org> wrote:

Thanks guys, I'll take a look at those possibilities before building anything new.

(sent from phone, pardon brevity)

On Jun 15, 2011 6:27 AM, "William Chan (陈智昌)" <will...@chromium.org> wrote:
> FWIW, we're creating a PostTaskAndReply() mechanism for
> MessageLoop[Proxy] that handles posting a task to a target thread and
> getting the reply on the origin thread. It's intended to replace the
> multiple implementations of FileUtilProxy and maybe CancelableRequest.
> It's conceivable we could extend this for WorkerPool. Albert's
> recently freed up responsibilities so I hope that he will get to this
> soon, amongst our other base::Callback related tasks. I don't know if
> this works for Joi's timeline.
>
> On Wed, Jun 15, 2011 at 11:35 AM, Jonathan Dixon <jo...@chromium.org> wrote:
>> On the face of it, it sounds like CancelableRequest should do exactly what
>> you want
>> http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/content/browser/cancelable_request.h&exact_package=chromium&q=CancelableRequest&type=cs
>>
>> The docs in that class even mention it's for sending work to a "background"
>> thread and it would be easy to imagine the worker pool is the canonical
>> example of background threads.
>> I guess it's not that simple due to the never-exit nature of the worker
>> pool?

Pondering this a moment longer, shouldn't CancelableRequest Just Work with the worker pool? So long as the request consumer remembers to cancel the request before it (and/or its host thread) die, then it shouldn't matter that the worker thread may continue to chug away on the request long after the consumer has packed up and gone home.

[Darin Fisher]

Yes, I think it would be great to extend PostTaskAndReply to WorkerPool.

I was thinking that we might even considering having a thread pool implement MessageLoopProxy.  (By the way, I suspect that MessageLoopProxy is actually a poor name for this interface.  It should probably be named something more like TaskQueue.)

-Darin

[Jói Sigurðsson]

Thanks all.

Seeing as CancelableRequest is currently in content/browser/, I can't
use it from net/ without moving it to base/. I'm not sure I should do
that because of the work ongoing on PostTaskAndReply, which will be in
base/ and may replace CancelableRequest. A secondary concern for
moving it to base/ is that CancelableRequest does not seem to have
unit tests (correct me if I'm wrong). OTOH it looks like it has been
around forever so it's probably very robust.

I may just implement something special-purpose for now and put a TODO
to revisit once PostTaskAndReply is available. Will, is there a crbug
I could star to stay up to date on that work?

Cheers,
Jói

[William Chan (陈智昌)]

On Thu, Jun 16, 2011 at 2:04 PM, Jói Sigurðsson <j...@chromium.org> wrote:
> Thanks all.
>
> Seeing as CancelableRequest is currently in content/browser/, I can't
> use it from net/ without moving it to base/.  I'm not sure I should do
> that because of the work ongoing on PostTaskAndReply, which will be in
> base/ and may replace CancelableRequest.  A secondary concern for
> moving it to base/ is that CancelableRequest does not seem to have
> unit tests (correct me if I'm wrong). OTOH it looks like it has been
> around forever so it's probably very robust.
>
> I may just implement something special-purpose for now and put a TODO
> to revisit once PostTaskAndReply is available.  Will, is there a crbug
> I could star to stay up to date on that work?

Filed as http://code.google.com/p/chromium/issues/detail?id=86301.

[William Chan (陈智昌)]

On Thu, Jun 16, 2011 at 2:25 AM, Darin Fisher <da...@chromium.org> wrote:
> Yes, I think it would be great to extend PostTaskAndReply to WorkerPool.
> I was thinking that we might even considering having a thread pool implement
> MessageLoopProxy.  (By the way, I suspect that MessageLoopProxy is actually

Can you clarify what you mean by having a thread pool implement
MessageLoopProxy? MessageLoopProxy seems MessageLoop specific to me.
Perhaps you meant pulling out an abstract interface, implemented by
MessageLoop[Proxy], WorkerPool, and any other thread pool?

I am tentatively in favor of something like this. My only concern is
if we start passing around this interface and use it polymorphically,
because one of the important differences between MessageLoop and
WorkerPool is that MessageLoop is ordered and WorkerPool is not. Thus,
tasks posted to MessageLoop will be completed FIFO, whereas there is
no such guarantee with WorkerPool. That said, I don't think that the
polymorphic interface will be used in many places. People typically do
MessageLoop::current()->PostTask(), WorkerPool::PostTask(),
BrowserThread::PostTask().

> a poor name for this interface.  It should probably be named something more
> like TaskQueue.)

<bikeshed color>
TaskRunner seems more appropriate to me, since it's not just a queue,
but actually invokes Run().
</bikeshed>

[Darin Fisher]

On Thu, Jun 16, 2011 at 4:29 AM, William Chan (陈智昌) <will...@chromium.org> wrote:

On Thu, Jun 16, 2011 at 2:25 AM, Darin Fisher <da...@chromium.org> wrote:
> Yes, I think it would be great to extend PostTaskAndReply to WorkerPool.
> I was thinking that we might even considering having a thread pool implement
> MessageLoopProxy.  (By the way, I suspect that MessageLoopProxy is actually

Can you clarify what you mean by having a thread pool implement
MessageLoopProxy? MessageLoopProxy seems MessageLoop specific to me.
Perhaps you meant pulling out an abstract interface, implemented by
MessageLoop[Proxy], WorkerPool, and any other thread pool?

Yes, that's what I meant.  I suspect that the new interface would have almost all of the

same methods as MessageLoopProxy (which is an interface already).  This is why I

thought we might just end up renaming MessageLoopProxy.  Not sure :-)

 

I am tentatively in favor of something like this. My only concern is
if we start passing around this interface and use it polymorphically,
because one of the important differences between MessageLoop and
WorkerPool is that MessageLoop is ordered and WorkerPool is not. Thus,
tasks posted to MessageLoop will be completed FIFO, whereas there is
no such guarantee with WorkerPool. That said, I don't think that the
polymorphic interface will be used in many places. People typically do
MessageLoop::current()->PostTask(), WorkerPool::PostTask(),
BrowserThread::PostTask().

Yes, this is the reason why I've always shied away from creating such an interface.

However, I've sort of re-convinced myself that it is worth that cost.

By the way, I once created something like this for Mozilla named EventTarget (that

EventQueue and ThreadPool both implement), and it was mostly nice.

 

> a poor name for this interface.  It should probably be named something more
> like TaskQueue.)

<bikeshed color>
TaskRunner seems more appropriate to me, since it's not just a queue,
but actually invokes Run().
</bikeshed>

Yeah, good point.  I was hoping for a name that would indicate that it is a runner

for many tasks, but TaskQueue makes one think it is FIFO.  As I mentioned above,

I used the name "Target" for Mozilla, but I'm not sure that I like TaskTarget :-P

-Darin