Make new TaskRunner, SequencedTaskRunner, and SingleThreadTaskRunner interfaces

Make new TaskRunner, SequencedTaskRunner, and SingleThreadTaskRunner interfaces

TaskRunner just has Post{,Delayed}Task(), SequencedTaskRunner
extends Executor to have ordering guarantees and PostNonNestable{,Delayed}Task(), and SingleThreadTaskRunner extends SequencedTaskRunner and guarantees execution on a single thread.

Move a bunch of methods from MessageLoopProxy into the TaskRunner classes and make it inherit from SingleThreadTaskRunner.

BUG=110973
TEST=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=121999

[akalin, 2012-01-25 02:40:30]

+darin for review, +willchan, ajwong in case they have any comments.

This is my initial pass at the interfaces.  Let me know what you think.

[darin (slow to review), 2012-01-25 07:25:56]

I still need to let the name Executor and SerialExecutor sink in.  I like that
it is one word instead of two like TaskRunner, but TaskRunner is nice because it
so very clearly evokes task->Run(), which makes its purpose very obvious.

https://chromiumcodereview.appspot.com/9169037/diff/1/base/executor_helpers.h
File base/executor_helpers.h (right):

https://chromiumcodereview.appspot.com/9169037/diff/1/base/executor_helpers.h...
base/executor_helpers.h:70: static ReturnType DeleteOnExecutor(
I wonder if "From" or "Via" wouldn't be a better that "On" here.

https://chromiumcodereview.appspot.com/9169037/diff/1/base/serial_executor.h
File base/serial_executor.h (right):

https://chromiumcodereview.appspot.com/9169037/diff/1/base/serial_executor.h#...
base/serial_executor.h:16: // will be executed on a single specific thread. 
Furthermore, it
It wasn't clear to me that we needed this interface to guarantee same-thread
execution.  The main requirement was FIFO execution, I thought, for tasks posted
from the same thread.  That way, the user can know that their tasks won't ever
overlap or run non-FIFO.

[akalin, 2012-01-25 08:57:01]

On 2012/01/25 07:25:56, darin wrote:
> I still need to let the name Executor and SerialExecutor sink in.  I like that
> it is one word instead of two like TaskRunner, but TaskRunner is nice because
it
> so very clearly evokes task->Run(), which makes its purpose very obvious.

I don't mind either way.  However, note that libjingle already uses the class
TaskRunner in the talk_base namespace, and it's substantially different in
implementation.

> https://chromiumcodereview.appspot.com/9169037/diff/1/base/executor_helpers.h
> File base/executor_helpers.h (right):
> 
>
https://chromiumcodereview.appspot.com/9169037/diff/1/base/executor_helpers.h...
> base/executor_helpers.h:70: static ReturnType DeleteOnExecutor(
> I wonder if "From" or "Via" wouldn't be a better that "On" here.

What about "With"?

> https://chromiumcodereview.appspot.com/9169037/diff/1/base/serial_executor.h
> File base/serial_executor.h (right):
> 
>
https://chromiumcodereview.appspot.com/9169037/diff/1/base/serial_executor.h#...
> base/serial_executor.h:16: // will be executed on a single specific thread. 
> Furthermore, it
> It wasn't clear to me that we needed this interface to guarantee same-thread
> execution.  The main requirement was FIFO execution, I thought, for tasks
posted
> from the same thread.  That way, the user can know that their tasks won't ever
> overlap or run non-FIFO.

I've thought about it and I think we do need to guarantee same-thread execution.
 Here's why: if we guarantee only FIFO execution, then a possible implementation
would have multiple underlying threads and some other mechanism to guarantee
FIFO-ness (plus appropriate memory barriers when the thread is switched).  But
then one wouldn't be able to use a SerialExecutor with objects that live on a
single thread (e.g., WeakPtr).  You'd post a task to the SerialExecutor to
create such an object, but if you post another task to use it, or to destroy it,
then you're not guaranteed that it'll be run on the same thread.

One possibility would be to make SerialExecutor guarantee FIFO-ness, and some
other subinterface guarantee single-threadedness (ThreadExecutor?).  But I'm not
sure if that's very useful.  WDYT?

[darin (slow to review), 2012-01-25 16:09:55]

On Wed, Jan 25, 2012 at 12:57 AM, <akalin@chromium.org> wrote:

> On 2012/01/25 07:25:56, darin wrote:
>
>> I still need to let the name Executor and SerialExecutor sink in.  I like
>> that
>> it is one word instead of two like TaskRunner, but TaskRunner is nice
>> because
>>
> it
>
>> so very clearly evokes task->Run(), which makes its purpose very obvious.
>>
>
> I don't mind either way.  However, note that libjingle already uses the
> class
> TaskRunner in the talk_base namespace, and it's substantially different in
> implementation.


such name conflicts with third party libraries are unfortunate but probably
not deal breakers.  we should pick the best names for chromium, just taking
chromium's needs into account.  (e.g., webkit also has a class named
RenderView!)


>
>
>  https://chromiumcodereview.**appspot.com/9169037/diff/1/**
>>
base/executor_helpers.h<https://chromiumcodereview.appspot.com/9169037/diff/1/base/executor_helpers.h>
>> File base/executor_helpers.h (right):
>>
>
>
> https://chromiumcodereview.**appspot.com/9169037/diff/1/**
>
base/executor_helpers.h#**newcode70<https://chromiumcodereview.appspot.com/9169037/diff/1/base/executor_helpers.h#newcode70>
>
>> base/executor_helpers.h:70: static ReturnType DeleteOnExecutor(
>> I wonder if "From" or "Via" wouldn't be a better that "On" here.
>>
>
> What about "With"?


maybe... it sounds like you could use an executor to delete something,
which doesn't seem quite right.  it doesn't delete things.  it runs things.
 hmm...



>
>
>  https://chromiumcodereview.**appspot.com/9169037/diff/1/**
>>
base/serial_executor.h<https://chromiumcodereview.appspot.com/9169037/diff/1/base/serial_executor.h>
>> File base/serial_executor.h (right):
>>
>
>
> https://chromiumcodereview.**appspot.com/9169037/diff/1/**
>
base/serial_executor.h#**newcode16<https://chromiumcodereview.appspot.com/9169037/diff/1/base/serial_executor.h#newcode16>
>
>> base/serial_executor.h:16: // will be executed on a single specific
>> thread.
>> Furthermore, it
>> It wasn't clear to me that we needed this interface to guarantee
>> same-thread
>> execution.  The main requirement was FIFO execution, I thought, for tasks
>>
> posted
>
>> from the same thread.  That way, the user can know that their tasks won't
>> ever
>> overlap or run non-FIFO.
>>
>
> I've thought about it and I think we do need to guarantee same-thread
> execution.
>  Here's why: if we guarantee only FIFO execution, then a possible
> implementation
> would have multiple underlying threads and some other mechanism to
> guarantee
> FIFO-ness (plus appropriate memory barriers when the thread is switched).
>  But
> then one wouldn't be able to use a SerialExecutor with objects that live
> on a
> single thread (e.g., WeakPtr).  You'd post a task to the SerialExecutor to
> create such an object, but if you post another task to use it, or to
> destroy it,
> then you're not guaranteed that it'll be run on the same thread.
>
>
makes sense.  that said, the NonThreadSafe class asserts used on same
thread when in reality it only needs to assert serial usage!  hmm...
probably is simpler to just think of these as analogous as in practice.
 serial executor implies all tasks run on the same thread.  ok!



> One possibility would be to make SerialExecutor guarantee FIFO-ness, and
> some
> other subinterface guarantee single-threadedness (ThreadExecutor?).  But
> I'm not
> sure if that's very useful.  WDYT?
>
>
http://codereview.chromium.**org/9169037/<http://codereview.chromium.org/9169...
>

[akalin, 2012-01-25 23:55:34]

On 2012/01/25 16:09:55, darin wrote:
> such name conflicts with third party libraries are unfortunate but probably
> not deal breakers.  we should pick the best names for chromium, just taking
> chromium's needs into account.  (e.g., webkit also has a class named
> RenderView!)

Okay, fair enough.  But sticking with Executor until we decide otherwise.

> makes sense.  that said, the NonThreadSafe class asserts used on same
> thread when in reality it only needs to assert serial usage!  hmm...
> probably is simpler to just think of these as analogous as in practice.
>  serial executor implies all tasks run on the same thread.  ok!

Yeah.  Added comment justifying the same-thread requirement.

[akalin, 2012-01-25 23:56:07]

Expanded class comments to explain in detail the various guarantees that
Executor and SerialExecutor make.

http://codereview.chromium.org/9169037/diff/1/base/executor_helpers.h
File base/executor_helpers.h (right):

http://codereview.chromium.org/9169037/diff/1/base/executor_helpers.h#newcode70
base/executor_helpers.h:70: static ReturnType DeleteOnExecutor(
On 2012/01/25 07:25:56, darin wrote:
> I wonder if "From" or "Via" wouldn't be a better that "On" here.

Went with "Via"

[willchan no longer on Chromium, 2012-01-26 02:45:49]

Sent from my iNexus from Thailand

On Jan 25, 2012 11:09 PM, "Darin Fisher" <darin@chromium.org> wrote:
>
> On Wed, Jan 25, 2012 at 12:57 AM, <akalin@chromium.org> wrote:
>>
>> On 2012/01/25 07:25:56, darin wrote:
>>>
>>> I still need to let the name Executor and SerialExecutor sink in.  I
like that
>>> it is one word instead of two like TaskRunner, but TaskRunner is nice
because
>>
>> it
>>>
>>> so very clearly evokes task->Run(), which makes its purpose very
obvious.

I minorly prefer Executor since it is commonly used in other concurrency
libraries. I don't care enough to push it though.

>>
>>
>> I don't mind either way.  However, note that libjingle already uses the
class
>> TaskRunner in the talk_base namespace, and it's substantially different
in
>> implementation.
>
>
> such name conflicts with third party libraries are unfortunate but
probably not deal breakers.  we should pick the best names for chromium,
just taking chromium's needs into account.  (e.g., webkit also has a class
named RenderView!)
>
>>
>>
>>
>>>
https://chromiumcodereview.appspot.com/9169037/diff/1/base/executor_helpers.h
>>> File base/executor_helpers.h (right):
>>
>>
>>
>>
https://chromiumcodereview.appspot.com/9169037/diff/1/base/executor_helpers.h...
>>>
>>> base/executor_helpers.h:70: static ReturnType DeleteOnExecutor(
>>> I wonder if "From" or "Via" wouldn't be a better that "On" here.
>>
>>
>> What about "With"?
>
>
> maybe... it sounds like you could use an executor to delete something,
which doesn't seem quite right.  it doesn't delete things.  it runs things.
 hmm...
>
>
>>
>>
>>
>>>
https://chromiumcodereview.appspot.com/9169037/diff/1/base/serial_executor.h
>>> File base/serial_executor.h (right):
>>
>>
>>
>>
https://chromiumcodereview.appspot.com/9169037/diff/1/base/serial_executor.h#...
>>>
>>> base/serial_executor.h:16: // will be executed on a single specific
thread.
>>> Furthermore, it
>>> It wasn't clear to me that we needed this interface to guarantee
same-thread
>>> execution.  The main requirement was FIFO execution, I thought, for
tasks
>>
>> posted
>>>
>>> from the same thread.  That way, the user can know that their tasks
won't ever
>>> overlap or run non-FIFO.
>>
>>
>> I've thought about it and I think we do need to guarantee same-thread
execution.
>>  Here's why: if we guarantee only FIFO execution, then a possible
implementation
>> would have multiple underlying threads and some other mechanism to
guarantee
>> FIFO-ness (plus appropriate memory barriers when the thread is
switched).  But
>> then one wouldn't be able to use a SerialExecutor with objects that live
on a
>> single thread (e.g., WeakPtr).  You'd post a task to the SerialExecutor
to
>> create such an object, but if you post another task to use it, or to
destroy it,
>> then you're not guaranteed that it'll be run on the same thread.
>>
>
> makes sense.  that said, the NonThreadSafe class asserts used on same
thread when in reality it only needs to assert serial usage!  hmm...
probably is simpler to just think of these as analogous as in practice.
 serial executor implies all tasks run on the same thread.  ok!

Using multiple threads is silly, since only one task can run on any thread,
while all others must sit idle since otherwise you get races. That said, we
should consider testing with mock Executors, where NonThreadSafe assertions
may be annoying. It's probably fine.

>
>
>>
>> One possibility would be to make SerialExecutor guarantee FIFO-ness, and
some
>> other subinterface guarantee single-threadedness (ThreadExecutor?).  But
I'm not
>> sure if that's very useful.  WDYT?
>>
>> http://codereview.chromium.org/9169037/
>
>

[akalin, 2012-01-26 03:21:08]

On 2012/01/26 02:45:49, willchan wrote:
> Using multiple threads is silly, since only one task can run on any thread,
> while all others must sit idle since otherwise you get races. That said, we
> should consider testing with mock Executors, where NonThreadSafe assertions
> may be annoying. It's probably fine.

I can think of a couple of use cases for a SerialExecutor that wasn't
necessarily on one thread:

1) Let's say that you wanted to wrap a MessageLoop in a SerialExecutor class,
and also detect when that MessageLoop is destroyed and stash any future posted
tasks onto a list to run later.  Then, those tasks will be executed on a
different thread than normal. (This would be used if it were somehow really
important to run all the tasks that a particular object posts, say)

2) Let's say that the FILE thread was replaced with some thread pool, but you
wanted to serialize a set of file operations.  Then you could wrap the
ThreadPoolExecutor with a SerialExecutor wrapper that waited for each task to
finish before sending another one to the thread pool.  With the current
SerialExecutor, you'd also have to bind to a thread out of the thread-pool,
which may not be possible/desirable.

I don't know if these use cases are common enough to make a distinction between
SerialExecutor and ThreadExecutor.

[darin (slow to review), 2012-01-26 03:55:52]

On Wed, Jan 25, 2012 at 7:21 PM, <akalin@chromium.org> wrote:

> On 2012/01/26 02:45:49, willchan wrote:
>
>> Using multiple threads is silly, since only one task can run on any
>> thread,
>> while all others must sit idle since otherwise you get races. That said,
>> we
>> should consider testing with mock Executors, where NonThreadSafe
>> assertions
>> may be annoying. It's probably fine.
>>
>
> I can think of a couple of use cases for a SerialExecutor that wasn't
> necessarily on one thread:
>
> 1) Let's say that you wanted to wrap a MessageLoop in a SerialExecutor
> class,
> and also detect when that MessageLoop is destroyed and stash any future
> posted
> tasks onto a list to run later.  Then, those tasks will be executed on a
> different thread than normal. (This would be used if it were somehow really
> important to run all the tasks that a particular object posts, say)
>
> 2) Let's say that the FILE thread was replaced with some thread pool, but
> you
> wanted to serialize a set of file operations.  Then you could wrap the
> ThreadPoolExecutor with a SerialExecutor wrapper that waited for each task
> to
> finish before sending another one to the thread pool.  With the current
> SerialExecutor, you'd also have to bind to a thread out of the thread-pool,
> which may not be possible/desirable.
>

^^^ This seems like a scenario that could come up in our world, given the
work
Brett did recently.

-Darin



>
> I don't know if these use cases are common enough to make a distinction
> between
> SerialExecutor and ThreadExecutor.
>
>
https://chromiumcodereview.**appspot.com/9169037/<https://chromiumcodereview....
>

[akalin, 2012-01-26 04:19:10]

On 2012/01/26 03:55:52, darin wrote:
> ^^^ This seems like a scenario that could come up in our world, given the
> work
> Brett did recently.

Should we then have Executor, SerialExecutor (guarantees FIFO behavior), and
ThreadExecutor (guarantees FIFO behavior on a single thread)?

[akalin, 2012-01-26 05:22:45]

On 2012/01/26 04:19:10, akalin wrote:
> On 2012/01/26 03:55:52, darin wrote:
> > ^^^ This seems like a scenario that could come up in our world, given the
> > work
> > Brett did recently.
> 
> Should we then have Executor, SerialExecutor (guarantees FIFO behavior), and
> ThreadExecutor (guarantees FIFO behavior on a single thread)?

Alternative: Just have a single Executor class, but with the following
additional methods:

bool IsSerial()
bool isSingleThreaded()

or

enum { PARALLEL, SERIAL, SINGLE_THREADED } GetType();

then presumably if a class cares about one or more of these properties, it can
check these flags.

[willchan no longer on Chromium, 2012-01-26 14:41:23]

On Thursday, January 26, 2012,  <akalin@chromium.org> wrote:
> On 2012/01/26 02:45:49, willchan wrote:
>>
>> Using multiple threads is silly, since only one task can run on any
thread,
>> while all others must sit idle since otherwise you get races. That said,
we
>> should consider testing with mock Executors, where NonThreadSafe
assertions
>> may be annoying. It's probably fine.
>
> I can think of a couple of use cases for a SerialExecutor that wasn't
> necessarily on one thread:
>
> 1) Let's say that you wanted to wrap a MessageLoop in a SerialExecutor
class,
> and also detect when that MessageLoop is destroyed and stash any future
posted
> tasks onto a list to run later.  Then, those tasks will be executed on a
> different thread than normal. (This would be used if it were somehow
really
> important to run all the tasks that a particular object posts, say)

Sounds weird and possibly wrong. Doesn't this end up possibly never halting?

>
> 2) Let's say that the FILE thread was replaced with some thread pool, but
you
> wanted to serialize a set of file operations.  Then you could wrap the
> ThreadPoolExecutor with a SerialExecutor wrapper that waited for each
task to
> finish before sending another one to the thread pool.  With the current
> SerialExecutor, you'd also have to bind to a thread out of the
thread-pool,
> which may not be possible/desirable.

This is a very good point. It may arguably be desirable, since otherwise
our FILE thread DCHECKs are all going to be worthless. Actually they'll
probably break no matter what. We could use a scoped thread local to assert
that we're on the appropriate SerialExecutor perhaps.

>
> I don't know if these use cases are common enough to make a distinction
between
> SerialExecutor and ThreadExecutor.
>
> https://chromiumcodereview.appspot.com/9169037/
>

[akalin, 2012-01-27 01:16:08]

On 2012/01/26 14:41:23, willchan wrote:
> Sounds weird and possibly wrong. Doesn't this end up possibly never halting?

In general, yes.  But if restricted to a single class I can see it being okay. 
Anyway, it was just a contrived use case.

[akalin, 2012-01-27 01:18:11]

I uploaded a new patch.  In this one, I have three classes: Executor <-
SerialExecutor <- SingleThreadExecutor.  I also added a virtual function
IsCompatibleWithCurrentThread() to Executor, which tasks can use to sanity-check
that they're being run from a particular {,Serial,SingleThread}Executor.  Of
course, SingleThreadExecutor::IsCompatibleWithCurrentThread() is equivalent
checking if you're on the right thread.

What do you guys think?  Does everyone agree with this implementation?  If so, I
can go ahead and polish it up.

[akalin, 2012-01-31 01:48:34]

Pinging Darin!

On 2012/01/27 01:18:11, akalin wrote:
> I uploaded a new patch.  In this one, I have three classes: Executor <-
> SerialExecutor <- SingleThreadExecutor.  I also added a virtual function
> IsCompatibleWithCurrentThread() to Executor, which tasks can use to
sanity-check
> that they're being run from a particular {,Serial,SingleThread}Executor.  Of
> course, SingleThreadExecutor::IsCompatibleWithCurrentThread() is equivalent
> checking if you're on the right thread.
> 
> What do you guys think?  Does everyone agree with this implementation?  If so,
I
> can go ahead and polish it up.

[darin (slow to review), 2012-01-31 07:11:48]

I think this change will impact a lot of people.  You should probably
pre-announce on chromium-dev and provide rationale for the design / naming /
etc.

http://codereview.chromium.org/9169037/diff/6002/base/executor.h
File base/executor.h (right):

http://codereview.chromium.org/9169037/diff/6002/base/executor.h#newcode37
base/executor.h:37: //   - Increasing the delay can only delay execution of the
task.
what does this mean?

http://codereview.chromium.org/9169037/diff/6002/base/executor.h#newcode51
base/executor.h:51: //
you don't want to mention the simplest form of executor?  the message loop?

http://codereview.chromium.org/9169037/diff/6002/base/executor.h#newcode54
base/executor.h:54: //   - An Executor that, for each task, spawns a
non-joinable thread to
Hmm... such an implementation carries with it the risk that the task may run
_after_ AtExitManager has already destroyed stuff.  We don't generally use
non-joinable threads in Chromium (except for calling getaddrinfo because it can
hang).

http://codereview.chromium.org/9169037/diff/6002/base/executor.h#newcode98
base/executor.h:98: virtual bool IsCompatibleWithCurrentThread() const = 0;
Compatible is such a vague term.  It somehow keeps bugging me.  Maybe a more
explicit "MayRunTasksOnCurrentThread" would work better?

[darin (slow to review), 2012-01-31 07:20:17]

On Thu, Jan 26, 2012 at 5:18 PM, <akalin@chromium.org> wrote:

> I uploaded a new patch.  In this one, I have three classes: Executor <-
> SerialExecutor <- SingleThreadExecutor.  I also added a virtual function
> IsCompatibleWithCurrentThread(**) to Executor, which tasks can use to
> sanity-check
> that they're being run from a particular {,Serial,SingleThread}**Executor.
>  Of
> course, SingleThreadExecutor::**IsCompatibleWithCurrentThread(**) is
> equivalent
> checking if you're on the right thread.
>

For SingleThreadExecutor, it'd probably be nice to have a more explicit
BelongsToCurrentThread check.  That will result in more readable code, I
think.



>
> What do you guys think?  Does everyone agree with this implementation?  If
> so, I
> can go ahead and polish it up.
>
>
http://codereview.chromium.**org/9169037/<http://codereview.chromium.org/9169...
>

[akalin, 2012-01-31 23:57:29]

http://codereview.chromium.org/9169037/diff/6002/base/executor.h
File base/executor.h (right):

http://codereview.chromium.org/9169037/diff/6002/base/executor.h#newcode37
base/executor.h:37: //   - Increasing the delay can only delay execution of the
task.
On 2012/01/31 07:11:48, darin wrote:
> what does this mean?

If you increase the delay, it can either do nothing, or make the task run later
than it normally would, but not make it run earlier.  Added these details into
the comments.

http://codereview.chromium.org/9169037/diff/6002/base/executor.h#newcode51
base/executor.h:51: //
On 2012/01/31 07:11:48, darin wrote:
> you don't want to mention the simplest form of executor?  the message loop?

I'm a bit hesitant to do that, because people may assume that Executor ==
MessageLoop, when it's not (since ML is single threaded, etc.)  However, added
the Message Loop example to SingleThreadedExecutor.

http://codereview.chromium.org/9169037/diff/6002/base/executor.h#newcode54
base/executor.h:54: //   - An Executor that, for each task, spawns a
non-joinable thread to
On 2012/01/31 07:11:48, darin wrote:
> Hmm... such an implementation carries with it the risk that the task may run
> _after_ AtExitManager has already destroyed stuff.  We don't generally use
> non-joinable threads in Chromium (except for calling getaddrinfo because it
can
> hang).

These are just theoretical implementations for clients to get an idea of what
can be a 'valid' Executor.  Changed 'possible' to 'theoretical'.

http://codereview.chromium.org/9169037/diff/6002/base/executor.h#newcode98
base/executor.h:98: virtual bool IsCompatibleWithCurrentThread() const = 0;
On 2012/01/31 07:11:48, darin wrote:
> Compatible is such a vague term.  It somehow keeps bugging me.  Maybe a more
> explicit "MayRunTasksOnCurrentThread" would work better?

Done.

[akalin, 2012-01-31 23:57:57]

> For SingleThreadExecutor, it'd probably be nice to have a more explicit
> BelongsToCurrentThread check.  That will result in more readable code, I
> think.

Done.

[akalin, 2012-01-31 23:58:43]

On 2012/01/31 07:11:48, darin wrote:
> I think this change will impact a lot of people.  You should probably
> pre-announce on chromium-dev and provide rationale for the design / naming /
> etc.

Okay.  But I think that should happen after we get consensus, i.e. you LGTM this
patch, just so that I don't send something out that will change drastically.

[akalin, 2012-01-31 23:59:21]

Addressed all comments, please take another look.

[darin (slow to review), 2012-02-01 05:45:22]

Looks good.  Now that Will is back from vacation, it'd be good to also get
his review.

On Tue, Jan 31, 2012 at 3:59 PM, <akalin@chromium.org> wrote:

> Addressed all comments, please take another look.
>
>
http://codereview.chromium.**org/9169037/<http://codereview.chromium.org/9169...
>

[akalin, 2012-02-01 05:57:18]

+willchan for review

Also +brettw to cc (explicitly).  Brett, are these interfaces in line with what
you had in mind?

[willchan no longer on Chromium, 2012-02-01 10:34:10]

Some initial comments. I will be off to Tahoe soon so I most likely won't reply
until Friday.

http://codereview.chromium.org/9169037/diff/22001/base/executor.cc
File base/executor.cc (right):

http://codereview.chromium.org/9169037/diff/22001/base/executor.cc#newcode15
base/executor.cc:15: class PostTaskAndReplyExecutor : public
internal::PostTaskAndReplyImpl {
The plan is to get rid of internal::PostTaskAndReplyImpl eventually, right?
Since we should only have one implementation and it should live here. If that
sounds right to you, can you leave a TODO?

http://codereview.chromium.org/9169037/diff/22001/base/executor.h
File base/executor.h (right):

http://codereview.chromium.org/9169037/diff/22001/base/executor.h#newcode85
base/executor.h:85: virtual bool PostDelayedTask(const
tracked_objects::Location& from_here,
Why are all these other PostTask() variants in here? WorkerPool and
SequencedWorkerPool do not use them. Do they not belong in SerialExecutor
instead?

Now that I say that though, it's interesting to imagine what an Executor shim in
front of SequencedWorkerPool would look like. My initial thoughts were that, if
you grabbed a SequenceToken from SequencedWorkerPool, then you could hold that
and the SequencedWorkerPool pointer within a Executor shim, that probably should
be a SerialExecutor actually to imply the FIFO ordering. Yet, it's not clear to
me that we would want these other PostTask() variants implemented in that
SerialExecutor shim class. I need to think a bit more about this, but perhaps
others have thoughts here.

http://codereview.chromium.org/9169037/diff/22001/base/executor.h#newcode107
base/executor.h:107: virtual bool MayRunTasksOnCurrentThread() const = 0;
I need to think this member over some more. I think this is the replacement for
BelongsToCurrentThread(), yes? "May" sounds very vague here. Anyway, I'll come
back to this later.

http://codereview.chromium.org/9169037/diff/22001/base/executor.h#newcode109
base/executor.h:109: // Posts |task| on the current Executor.  On completion,
|reply| is
Is this comment accurate? "current" is misleading since it may be read as the
current thread. It's really the target Executor, more accurately called |this|.

Also a lot of the terminology here assumes individual threads. What if the
executor you're currently running on is the WorkerPool? It's wrong to say that
|reply| will run on the same thread. Of course, it may be weird to call
PostTaskAndReply() from WorkerPool, since the current implementation relies on
MessageLoop::current() to return a valid MessageLoop. I need to think a bit
later on about whether or not we need a Executor::current() thing.

http://codereview.chromium.org/9169037/diff/22001/base/executor_helpers.h
File base/executor_helpers.h (right):

http://codereview.chromium.org/9169037/diff/22001/base/executor_helpers.h#new...
base/executor_helpers.h:5: #ifndef BASE_EXECUTOR_HELPERS_H_
Does this file need to exist? My understanding is it exists because it is shared
in message_loop.h, message_loop_proxy.h and what not. I sort of think that the
implementations should just move into executor.h, since that file is needed
anyway for calling DeleteSoonInternal and friends.

Let me know if you disagree. It's also acceptable to agree, but leave it as a
TODO to keep the changelist small.

[akalin, 2012-02-01 11:13:33]

On 2012/02/01 10:34:10, willchan wrote:
> Some initial comments. I will be off to Tahoe soon so I most likely won't
reply
> until Friday.
> 
> http://codereview.chromium.org/9169037/diff/22001/base/executor.cc
> File base/executor.cc (right):
> 
> http://codereview.chromium.org/9169037/diff/22001/base/executor.cc#newcode15
> base/executor.cc:15: class PostTaskAndReplyExecutor : public
> internal::PostTaskAndReplyImpl {
> The plan is to get rid of internal::PostTaskAndReplyImpl eventually, right?
> Since we should only have one implementation and it should live here. If that
> sounds right to you, can you leave a TODO?

Hmm really?  I was under the impression that some stuff (like MessageLoop)
wouldn't inherit from *Executor, and instead you'd have a wrapper around it
(i.e., MessageLoopProxy).  In that case, I think we still need
PostTaskAndReplyImpl.

> http://codereview.chromium.org/9169037/diff/22001/base/executor.h
> File base/executor.h (right):
> 
> http://codereview.chromium.org/9169037/diff/22001/base/executor.h#newcode85
> base/executor.h:85: virtual bool PostDelayedTask(const
> tracked_objects::Location& from_here,
> Why are all these other PostTask() variants in here? WorkerPool and
> SequencedWorkerPool do not use them. Do they not belong in SerialExecutor
> instead?

I don't think there's anything SerialExecutor-specific about posting a delayed
or non-nestable task, is there?

> Now that I say that though, it's interesting to imagine what an Executor shim
in
> front of SequencedWorkerPool would look like. My initial thoughts were that,
if
> you grabbed a SequenceToken from SequencedWorkerPool, then you could hold that
> and the SequencedWorkerPool pointer within a Executor shim, that probably
should
> be a SerialExecutor actually to imply the FIFO ordering. Yet, it's not clear
to
> me that we would want these other PostTask() variants implemented in that
> SerialExecutor shim class. I need to think a bit more about this, but perhaps
> others have thoughts here.

I think it must be that WorkerPool would implement Executor and
SequencedWorkerPool would implement SerialExecutor.

> http://codereview.chromium.org/9169037/diff/22001/base/executor.h#newcode107
> base/executor.h:107: virtual bool MayRunTasksOnCurrentThread() const = 0;
> I need to think this member over some more. I think this is the replacement
for
> BelongsToCurrentThread(), yes? "May" sounds very vague here. Anyway, I'll come
> back to this later.

Yes.  But 'May' is accurate.  Just because a thread is in a worker pool doesn't
mean that it will (as in definitely will) run tasks on it.

> 
> http://codereview.chromium.org/9169037/diff/22001/base/executor.h#newcode109
> base/executor.h:109: // Posts |task| on the current Executor.  On completion,
> |reply| is
> Is this comment accurate? "current" is misleading since it may be read as the
> current thread. It's really the target Executor, more accurately called
|this|.
> 
> Also a lot of the terminology here assumes individual threads. What if the
> executor you're currently running on is the WorkerPool? It's wrong to say that
> |reply| will run on the same thread. Of course, it may be weird to call
> PostTaskAndReply() from WorkerPool, since the current implementation relies on
> MessageLoop::current() to return a valid MessageLoop. I need to think a bit
> later on about whether or not we need a Executor::current() thing.

You're right in that 'current Executor' is misleading.  But the other
terminology assuming individual threads is correct; |reply| *will* run on the
same thread, assuming that there exists a MessageLoop on it, i.e. the
implementation calls MessageLoopProxy::current().  I guess worker threads may
not have message loops?  If so we may have to modify it to do something sane on
NULL MessageLoops.

> 
> http://codereview.chromium.org/9169037/diff/22001/base/executor_helpers.h
> File base/executor_helpers.h (right):
> 
>
http://codereview.chromium.org/9169037/diff/22001/base/executor_helpers.h#new...
> base/executor_helpers.h:5: #ifndef BASE_EXECUTOR_HELPERS_H_
> Does this file need to exist? My understanding is it exists because it is
shared
> in message_loop.h, message_loop_proxy.h and what not. I sort of think that the
> implementations should just move into executor.h, since that file is needed
> anyway for calling DeleteSoonInternal and friends.

As I said above, I don't think we'll be unifying MessageLoop/MLP/Executor
anytime soon, so we may have to live with this for a while.

[willchan no longer on Chromium, 2012-02-01 11:30:29]

On Wed, Feb 1, 2012 at 3:13 AM, <akalin@chromium.org> wrote:

> On 2012/02/01 10:34:10, willchan wrote:
>
>> Some initial comments. I will be off to Tahoe soon so I most likely won't
>>
> reply
>
>> until Friday.
>>
>
> 
http://codereview.chromium.**org/9169037/diff/22001/base/**executor.cc<http:/...
>> File base/executor.cc (right):
>>
>
>  http://codereview.chromium.**org/9169037/diff/22001/base/**
>>
executor.cc#newcode15<http://codereview.chromium.org/9169037/diff/22001/base/executor.cc#newcode15>
>> base/executor.cc:15: class PostTaskAndReplyExecutor : public
>> internal::PostTaskAndReplyImpl {
>> The plan is to get rid of internal::PostTaskAndReplyImpl eventually,
>> right?
>> Since we should only have one implementation and it should live here. If
>> that
>> sounds right to you, can you leave a TODO?
>>
>
> Hmm really?  I was under the impression that some stuff (like MessageLoop)
> wouldn't inherit from *Executor, and instead you'd have a wrapper around it
> (i.e., MessageLoopProxy).  In that case, I think we still need
> PostTaskAndReplyImpl.


I'm not sure I understand. Doesn't PostTaskAndReply() exist in
MessageLoopProxy, not MessageLoop? And shouldn't we always be using the
wrappers? I know that's currently not the case, but I think it makes sense
long-term to almost always avoid using MessageLoop directly, and instead
use Executor/MessageLoopProxy.


>
>
> 
http://codereview.chromium.**org/9169037/diff/22001/base/**executor.h<http://...
>> File base/executor.h (right):
>>
>
>  http://codereview.chromium.**org/9169037/diff/22001/base/**
>>
executor.h#newcode85<http://codereview.chromium.org/9169037/diff/22001/base/executor.h#newcode85>
>> base/executor.h:85: virtual bool PostDelayedTask(const
>> tracked_objects::Location& from_here,
>> Why are all these other PostTask() variants in here? WorkerPool and
>> SequencedWorkerPool do not use them. Do they not belong in SerialExecutor
>> instead?
>>
>
> I don't think there's anything SerialExecutor-specific about posting a
> delayed
> or non-nestable task, is there?


I don't understand what a nestable task means outside of a MessageLoop
context (ie a worker pool). While I can see how we can implement a delayed
task for WorkerPool, it's less clear how to do so for SequencedWorkerPool
if we're using a single SequenceToken. What is the desired ordering of
multiple tasks posted to a SequencedWorkerPool where they share the same
SequenceToken yet some are delayed?


>
>
>  Now that I say that though, it's interesting to imagine what an Executor
>> shim
>>
> in
>
>> front of SequencedWorkerPool would look like. My initial thoughts were
>> that,
>>
> if
>
>> you grabbed a SequenceToken from SequencedWorkerPool, then you could hold
>> that
>> and the SequencedWorkerPool pointer within a Executor shim, that probably
>>
> should
>
>> be a SerialExecutor actually to imply the FIFO ordering. Yet, it's not
>> clear
>>
> to
>
>> me that we would want these other PostTask() variants implemented in that
>> SerialExecutor shim class. I need to think a bit more about this, but
>> perhaps
>> others have thoughts here.
>>
>
> I think it must be that WorkerPool would implement Executor and
> SequencedWorkerPool would implement SerialExecutor.
>
>
>  http://codereview.chromium.**org/9169037/diff/22001/base/**
>>
executor.h#newcode107<http://codereview.chromium.org/9169037/diff/22001/base/executor.h#newcode107>
>> base/executor.h:107: virtual bool MayRunTasksOnCurrentThread() const = 0;
>> I need to think this member over some more. I think this is the
>> replacement
>>
> for
>
>> BelongsToCurrentThread(), yes? "May" sounds very vague here. Anyway, I'll
>> come
>> back to this later.
>>
>
> Yes.  But 'May' is accurate.  Just because a thread is in a worker pool
> doesn't
> mean that it will (as in definitely will) run tasks on it.


Yes, I agree it's accurate, but am just concerned it will confuse readers.
It's non-obvious what the meaning is. Perhaps it's better to invert the
subject. Threads are not shared among executors, right? Perhaps something
like OwnsCurrentThread()?


>
>
>
>  http://codereview.chromium.**org/9169037/diff/22001/base/**
>>
executor.h#newcode109<http://codereview.chromium.org/9169037/diff/22001/base/executor.h#newcode109>
>> base/executor.h:109: // Posts |task| on the current Executor.  On
>> completion,
>> |reply| is
>> Is this comment accurate? "current" is misleading since it may be read as
>> the
>> current thread. It's really the target Executor, more accurately called
>>
> |this|.
>
>  Also a lot of the terminology here assumes individual threads. What if the
>> executor you're currently running on is the WorkerPool? It's wrong to say
>> that
>> |reply| will run on the same thread. Of course, it may be weird to call
>> PostTaskAndReply() from WorkerPool, since the current implementation
>> relies on
>> MessageLoop::current() to return a valid MessageLoop. I need to think a
>> bit
>> later on about whether or not we need a Executor::current() thing.
>>
>
> You're right in that 'current Executor' is misleading.  But the other
> terminology assuming individual threads is correct; |reply| *will* run on
> the
> same thread, assuming that there exists a MessageLoop on it, i.e. the
> implementation calls MessageLoopProxy::current().  I guess worker threads
> may
> not have message loops?  If so we may have to modify it to do something
> sane on
> NULL MessageLoops.


Yeah, my concern was that, if we completely generalize PostTaskAndReply
across Executors, then the assumption that there's a MessageLoop on the
current thread is invalid.


>
>
>
>  http://codereview.chromium.**org/9169037/diff/22001/base/**
>>
executor_helpers.h<http://codereview.chromium.org/9169037/diff/22001/base/executor_helpers.h>
>> File base/executor_helpers.h (right):
>>
>
>
> http://codereview.chromium.**org/9169037/diff/22001/base/**
>
executor_helpers.h#newcode5<http://codereview.chromium.org/9169037/diff/22001/base/executor_helpers.h#newcode5>
>
>> base/executor_helpers.h:5: #ifndef BASE_EXECUTOR_HELPERS_H_
>> Does this file need to exist? My understanding is it exists because it is
>>
> shared
>
>> in message_loop.h, message_loop_proxy.h and what not. I sort of think
>> that the
>> implementations should just move into executor.h, since that file is
>> needed
>> anyway for calling DeleteSoonInternal and friends.
>>
>
> As I said above, I don't think we'll be unifying MessageLoop/MLP/Executor
> anytime soon, so we may have to live with this for a while.
>
>
http://codereview.chromium.**org/9169037/<http://codereview.chromium.org/9169...
>

[akalin, 2012-02-01 12:11:24]

On Wed, Feb 1, 2012 at 3:30 AM, William Chan (陈智昌) <willchan@chromium.org>wrote:

> I'm not sure I understand. Doesn't PostTaskAndReply() exist in
> MessageLoopProxy, not MessageLoop? And shouldn't we always be using the
> wrappers? I know that's currently not the case, but I think it makes sense
> long-term to almost always avoid using MessageLoop directly, and instead
> use Executor/MessageLoopProxy.
>

My mistake, you're right, PostTaskAndReply() is in MLP but not MessageLoop.
 However, it looks like WorkerPool also implements PostTaskAndReply, and
that is the (only) other implementation of internal::PostTaskAndReplyImpl.
 I agree that if possible, there should be only one implementation.  I'll
add a TODO.


> 
http://codereview.chromium.**org/9169037/diff/22001/base/**executor.h<http://...
>>> File base/executor.h (right):
>>>
>>
>>  http://codereview.chromium.**org/9169037/diff/22001/base/**
>>>
executor.h#newcode85<http://codereview.chromium.org/9169037/diff/22001/base/executor.h#newcode85>
>>> base/executor.h:85: virtual bool PostDelayedTask(const
>>> tracked_objects::Location& from_here,
>>> Why are all these other PostTask() variants in here? WorkerPool and
>>> SequencedWorkerPool do not use them. Do they not belong in SerialExecutor
>>> instead?
>>>
>>
>> I don't think there's anything SerialExecutor-specific about posting a
>> delayed
>> or non-nestable task, is there?
>
>
> I don't understand what a nestable task means outside of a MessageLoop
> context (ie a worker pool). While I can see how we can implement a delayed
> task for WorkerPool, it's less clear how to do so for SequencedWorkerPool
> if we're using a single SequenceToken. What is the desired ordering of
> multiple tasks posted to a SequencedWorkerPool where they share the same
> SequenceToken yet some are delayed?
>

It's plausible that even in a worker pool, you can end up with nested
tasks.  For example, if you have a simple WorkerPool implementation that
spawns n threads, each with their own MessageLoop, it's plausible that a
posted task can simple call 'MessageLoop::run()' directly.
 PostNonNestableTask guarantees that the posted task won't be run inside
such a loop.  I think the comment above 'Executor::PostNonNestableTask()'
says it in a general, non-MessageLoop specific way (I got the wording from
Darin): "[PostNonNestableTask guarantees] that the submitted task will not
execute nested within an already-executing task."

If nested tasks are impossible for a given Executor implementation (e.g.,
WorkerPool implementation that doesn't use MessageLoops or expose a ::run()
function), a valid implementation would be make PostNonNestableTask ==
PostTask.

The more practical concern is that DeleteSoon() and ReleaseSoon() both use
PostNonNestableTask, so if we want Executor to have those methods, we need
PostNonNestableTask.

As for delayed tasks, I don't think we can guarantee anything about the
ordering except in simple cases, e.g. if task B is posted after task A, and
task B's delay is > task A's delay, then task B will run after task A.
 Also, tasks with the same delay are FIFO.  I believe the comments in
serial_executor.h (which I assume SequencedWorkerPool would implement)
specify the ordering guarantees to be flexible enough for any reasonable
implementation, e.g. one that uses something like TaskQueue, or even one
that ignores the delay completely.  If SequencedWorkerPool is just a
wrapper around some WorkerPool that implements Executor, and Executor has
the whole range of Post*Task methods, then I think it would be easy to
satisfy the conditions of SerialExecutor just from the guarantees of
Executor plus some queue data structure.  I think even without delays you
still need some sort of queue, right?  What do you think?


> Yes, I agree it's accurate, but am just concerned it will confuse readers.
> It's non-obvious what the meaning is. Perhaps it's better to invert the
> subject. Threads are not shared among executors, right? Perhaps something
> like OwnsCurrentThread()?
>

It's plausible that Executors share threads, e.g. one SerialExecutor wraps
around another Executor, so the concept of an Executor "owning" a thread is
problematic.  The "May" is there because something like
"RunsTasksOnCurrentThread" seems to connote that the current thread is the
only thread that tasks are run on.  But maybe it still works?  WDYT?

http://codereview.chromium.**org/9169037/diff/22001/base/**
>>>
executor.h#newcode109<http://codereview.chromium.org/9169037/diff/22001/base/executor.h#newcode109>
>>> base/executor.h:109: // Posts |task| on the current Executor.  On
>>> completion,
>>> |reply| is
>>> Is this comment accurate? "current" is misleading since it may be read
>>> as the
>>> current thread. It's really the target Executor, more accurately called
>>>
>> |this|.
>>
>>  Also a lot of the terminology here assumes individual threads. What if
>>> the
>>> executor you're currently running on is the WorkerPool? It's wrong to
>>> say that
>>> |reply| will run on the same thread. Of course, it may be weird to call
>>> PostTaskAndReply() from WorkerPool, since the current implementation
>>> relies on
>>> MessageLoop::current() to return a valid MessageLoop. I need to think a
>>> bit
>>> later on about whether or not we need a Executor::current() thing.
>>>
>>
>> You're right in that 'current Executor' is misleading.  But the other
>> terminology assuming individual threads is correct; |reply| *will* run on
>> the
>> same thread, assuming that there exists a MessageLoop on it, i.e. the
>> implementation calls MessageLoopProxy::current().  I guess worker threads
>> may
>> not have message loops?  If so we may have to modify it to do something
>> sane on
>> NULL MessageLoops.
>
>
> Yeah, my concern was that, if we completely generalize PostTaskAndReply
> across Executors, then the assumption that there's a MessageLoop on the
> current thread is invalid.
>

Sure, but I think that can be left for future work.  I'll add a TODO.

The right solution may be to have PostTaskAndReply take an
Executor/SerialExecutor to reply on.

[willchan no longer on Chromium, 2012-02-01 12:27:06]

On Wed, Feb 1, 2012 at 4:11 AM, Fred Akalin <akalin@chromium.org> wrote:

> On Wed, Feb 1, 2012 at 3:30 AM, William Chan (陈智昌)
<willchan@chromium.org>wrote:
>
>> I'm not sure I understand. Doesn't PostTaskAndReply() exist in
>> MessageLoopProxy, not MessageLoop? And shouldn't we always be using the
>> wrappers? I know that's currently not the case, but I think it makes sense
>> long-term to almost always avoid using MessageLoop directly, and instead
>> use Executor/MessageLoopProxy.
>>
>
> My mistake, you're right, PostTaskAndReply() is in MLP but not
> MessageLoop.  However, it looks like WorkerPool also implements
> PostTaskAndReply, and that is the (only) other implementation of
> internal::PostTaskAndReplyImpl.  I agree that if possible, there should be
> only one implementation.  I'll add a TODO.
>
>
>> 
http://codereview.chromium.**org/9169037/diff/22001/base/**executor.h<http://...
>>>> File base/executor.h (right):
>>>>
>>>
>>>  http://codereview.chromium.**org/9169037/diff/22001/base/**
>>>>
executor.h#newcode85<http://codereview.chromium.org/9169037/diff/22001/base/executor.h#newcode85>
>>>> base/executor.h:85: virtual bool PostDelayedTask(const
>>>> tracked_objects::Location& from_here,
>>>> Why are all these other PostTask() variants in here? WorkerPool and
>>>> SequencedWorkerPool do not use them. Do they not belong in
>>>> SerialExecutor
>>>> instead?
>>>>
>>>
>>> I don't think there's anything SerialExecutor-specific about posting a
>>> delayed
>>> or non-nestable task, is there?
>>
>>
>> I don't understand what a nestable task means outside of a MessageLoop
>> context (ie a worker pool). While I can see how we can implement a delayed
>> task for WorkerPool, it's less clear how to do so for SequencedWorkerPool
>> if we're using a single SequenceToken. What is the desired ordering of
>> multiple tasks posted to a SequencedWorkerPool where they share the same
>> SequenceToken yet some are delayed?
>>
>
> It's plausible that even in a worker pool, you can end up with nested
> tasks.  For example, if you have a simple WorkerPool implementation that
> spawns n threads, each with their own MessageLoop, it's plausible that a
> posted task can simple call 'MessageLoop::run()' directly.
>  PostNonNestableTask guarantees that the posted task won't be run inside
> such a loop.  I think the comment above 'Executor::PostNonNestableTask()'
> says it in a general, non-MessageLoop specific way (I got the wording from
> Darin): "[PostNonNestableTask guarantees] that the submitted task will not
> execute nested within an already-executing task."
>

I may need Darin to assist here since he has more experience with the
problems with nested MessageLoops. My understanding is the problem that
your example of a WorkerPool that spawns n threads, each with their own
MessageLoop, introduces no new problems with nesting. The tasks could
already run concurrently on different threads, why is it worse that a task
is run within a task callstack on a single worker thread? It's only when
you have the expectation that it's a SerialExecutor that nesting (and it's
associated re-entrancies and out of order processing of tasks) causes
problems.


> If nested tasks are impossible for a given Executor implementation (e.g.,
> WorkerPool implementation that doesn't use MessageLoops or expose a ::run()
> function), a valid implementation would be make PostNonNestableTask ==
> PostTask.
>
> The more practical concern is that DeleteSoon() and ReleaseSoon() both use
> PostNonNestableTask, so if we want Executor to have those methods, we need
> PostNonNestableTask.
>
> As for delayed tasks, I don't think we can guarantee anything about the
> ordering except in simple cases, e.g. if task B is posted after task A, and
> task B's delay is > task A's delay, then task B will run after task A.
>  Also, tasks with the same delay are FIFO.  I believe the comments in
> serial_executor.h (which I assume SequencedWorkerPool would implement)
> specify the ordering guarantees to be flexible enough for any reasonable
> implementation, e.g. one that uses something like TaskQueue, or even one
> that ignores the delay completely.  If SequencedWorkerPool is just a
> wrapper around some WorkerPool that implements Executor, and Executor has
> the whole range of Post*Task methods, then I think it would be easy to
> satisfy the conditions of SerialExecutor just from the guarantees of
> Executor plus some queue data structure.  I think even without delays you
> still need some sort of queue, right?  What do you think?
>

I think that's reasonable. It's not currently supported by
SequencedWorkerPool, but I think we can argue that it should be. Probably
there has been no need yet and that's why it's not already supported. I
defer to Brett.


>
>
>> Yes, I agree it's accurate, but am just concerned it will confuse
>> readers. It's non-obvious what the meaning is. Perhaps it's better to
>> invert the subject. Threads are not shared among executors, right? Perhaps
>> something like OwnsCurrentThread()?
>>
>
> It's plausible that Executors share threads, e.g. one SerialExecutor wraps
> around another Executor, so the concept of an Executor "owning" a thread is
> problematic.  The "May" is there because something like
> "RunsTasksOnCurrentThread" seems to connote that the current thread is the
> only thread that tasks are run on.  But maybe it still works?  WDYT?
>

I think I prefer RunsTasksOnCurrentThread(), despite its flaws that you
have noted.


>
> http://codereview.chromium.**org/9169037/diff/22001/base/**
>>>>
executor.h#newcode109<http://codereview.chromium.org/9169037/diff/22001/base/executor.h#newcode109>
>>>> base/executor.h:109: // Posts |task| on the current Executor.  On
>>>> completion,
>>>> |reply| is
>>>> Is this comment accurate? "current" is misleading since it may be read
>>>> as the
>>>> current thread. It's really the target Executor, more accurately called
>>>>
>>> |this|.
>>>
>>>  Also a lot of the terminology here assumes individual threads. What if
>>>> the
>>>> executor you're currently running on is the WorkerPool? It's wrong to
>>>> say that
>>>> |reply| will run on the same thread. Of course, it may be weird to call
>>>> PostTaskAndReply() from WorkerPool, since the current implementation
>>>> relies on
>>>> MessageLoop::current() to return a valid MessageLoop. I need to think a
>>>> bit
>>>> later on about whether or not we need a Executor::current() thing.
>>>>
>>>
>>> You're right in that 'current Executor' is misleading.  But the other
>>> terminology assuming individual threads is correct; |reply| *will* run
>>> on the
>>> same thread, assuming that there exists a MessageLoop on it, i.e. the
>>> implementation calls MessageLoopProxy::current().  I guess worker
>>> threads may
>>> not have message loops?  If so we may have to modify it to do something
>>> sane on
>>> NULL MessageLoops.
>>
>>
>> Yeah, my concern was that, if we completely generalize PostTaskAndReply
>> across Executors, then the assumption that there's a MessageLoop on the
>> current thread is invalid.
>>
>
> Sure, but I think that can be left for future work.  I'll add a TODO.
>
> The right solution may be to have PostTaskAndReply take an
> Executor/SerialExecutor to reply on.
>

[akalin, 2012-02-02 06:55:10]

> I may need Darin to assist here since he has more experience with the
> problems with nested MessageLoops. My understanding is the problem that
> your example of a WorkerPool that spawns n threads, each with their own
> MessageLoop, introduces no new problems with nesting. The tasks could
> already run concurrently on different threads, why is it worse that a task
> is run within a task callstack on a single worker thread? It's only when
> you have the expectation that it's a SerialExecutor that nesting (and it's
> associated re-entrancies and out of order processing of tasks) causes
> problems.

Yeah, I see your point.  I'll defer to Darin, also.

> I think that's reasonable. It's not currently supported by
> SequencedWorkerPool, but I think we can argue that it should be. Probably
> there has been no need yet and that's why it's not already supported. I
> defer to Brett.

Okay, me too.  Keep in mind that ignoring the delay is a valid implementation,
and one we could do if we have no need for delays yet.

> I think I prefer RunsTasksOnCurrentThread(), despite its flaws that you
> have noted.

Okay.  What about ExecutesOnCurrentThread() to be more inline with the name?
(TaskRunner : RunTasksOnCurrentThread :: Executor : ExecutesOnCurrentThread())

I'll wait until everyone gets back from the trip and then we can hash out the
points above.

[darin (slow to review), 2012-02-03 18:18:19]

On Wed, Feb 1, 2012 at 10:55 PM, <akalin@chromium.org> wrote:

> I may need Darin to assist here since he has more experience with the
>> problems with nested MessageLoops. My understanding is the problem that
>> your example of a WorkerPool that spawns n threads, each with their own
>> MessageLoop, introduces no new problems with nesting. The tasks could
>> already run concurrently on different threads, why is it worse that a task
>> is run within a task callstack on a single worker thread? It's only when
>> you have the expectation that it's a SerialExecutor that nesting (and it's
>> associated re-entrancies and out of order processing of tasks) causes
>> problems.
>>
>
> Yeah, I see your point.  I'll defer to Darin, also.
>
>
>  I think that's reasonable. It's not currently supported by
>> SequencedWorkerPool, but I think we can argue that it should be. Probably
>> there has been no need yet and that's why it's not already supported. I
>> defer to Brett.
>>
>
> Okay, me too.  Keep in mind that ignoring the delay is a valid
> implementation,
> and one we could do if we have no need for delays yet.
>
>
>  I think I prefer RunsTasksOnCurrentThread(), despite its flaws that you
>> have noted.
>>
>
> Okay.  What about ExecutesOnCurrentThread() to be more inline with the
> name?
> (TaskRunner : RunTasksOnCurrentThread :: Executor :
> ExecutesOnCurrentThread())
>
> I'll wait until everyone gets back from the trip and then we can hash out
> the
> points above.
>


Guys, I'm starting to really sour on the Executor name.  I much prefer
TaskRunner because it is so much more obvious what it means.  I realize
that Executor is one word instead of two, but I think we should prefer
terms that are self evident over terms that are brief.  Anyways, 10
characters instead of 8 doesn't seem like a big difference to me.  Closures
have a Run method.  Closures have a Run method, not an Execute method.

On the topic of SerialExecutor and PostDelayedTask, it gets complicated,
but we have learned some lessons from MessageLoop that are important.
 PostDelayedTask does potentially take the task out of the FIFO and push it
further down in the execution order.  However, PostDelayedTask can never
result in the task being moved ahead in the execution order.

Internally, a PostDelayedTask should alway start out as an "internal"
PostTask.  When the internal PostTask runs, it looks at how much delay
still exists before it should have run, and then it gets put on a delayed
queue to be run once that time expires if the time has not already expired.
 Otherwise, it gets run right away.  This means that if the delay is
processing the queue is longer than the delay passed to PostDelayedTask
that a PostDelayedTask will effectively be treated as a PostTask.  This
preserves FIFO'ness for the consumer.  If they call PostTask followed by
PostDelayedTask, they can be ensured thatthe delayed task will not run
before the non-delayed task.

Now, as far as PostNonNestableTask is concerned, I agree it seems a bit
weird to say that a serial executor could run tasks in a nested fashion
since it is supposed to serialize their execution.  Maybe that suggests
that "serial executor" is maybe the wrong name?  How do we capture the
possibility of someone running a nested task loop?  As soon as we allow
nested task loops, then we need a way to post tasks that cannot run in a
nested fashion.  This does imply a secondary task queue, but much like
PostDelayedTask, a PostNonNestedTask starts out its life as a normal
PostTask.  At execution time, it looks to see if it is OK to run the task.
 If not, then it gets put on a delayed-until-unnested queue.

-Darin



>
>
http://codereview.chromium.**org/9169037/<http://codereview.chromium.org/9169...
>

[willchan no longer on Chromium, 2012-02-03 18:37:59]

On Fri, Feb 3, 2012 at 10:18 AM, Darin Fisher <darin@chromium.org> wrote:

>
>
> On Wed, Feb 1, 2012 at 10:55 PM, <akalin@chromium.org> wrote:
>
>>  I may need Darin to assist here since he has more experience with the
>>> problems with nested MessageLoops. My understanding is the problem that
>>> your example of a WorkerPool that spawns n threads, each with their own
>>> MessageLoop, introduces no new problems with nesting. The tasks could
>>> already run concurrently on different threads, why is it worse that a
>>> task
>>> is run within a task callstack on a single worker thread? It's only when
>>> you have the expectation that it's a SerialExecutor that nesting (and
>>> it's
>>> associated re-entrancies and out of order processing of tasks) causes
>>> problems.
>>>
>>
>> Yeah, I see your point.  I'll defer to Darin, also.
>>
>>
>>  I think that's reasonable. It's not currently supported by
>>> SequencedWorkerPool, but I think we can argue that it should be. Probably
>>> there has been no need yet and that's why it's not already supported. I
>>> defer to Brett.
>>>
>>
>> Okay, me too.  Keep in mind that ignoring the delay is a valid
>> implementation,
>> and one we could do if we have no need for delays yet.
>>
>>
>>  I think I prefer RunsTasksOnCurrentThread(), despite its flaws that you
>>> have noted.
>>>
>>
>> Okay.  What about ExecutesOnCurrentThread() to be more inline with the
>> name?
>> (TaskRunner : RunTasksOnCurrentThread :: Executor :
>> ExecutesOnCurrentThread())
>>
>> I'll wait until everyone gets back from the trip and then we can hash out
>> the
>> points above.
>>
>
>
> Guys, I'm starting to really sour on the Executor name.  I much prefer
> TaskRunner because it is so much more obvious what it means.  I realize
> that Executor is one word instead of two, but I think we should prefer
> terms that are self evident over terms that are brief.  Anyways, 10
> characters instead of 8 doesn't seem like a big difference to me.  Closures
> have a Run method.  Closures have a Run method, not an Execute method.
>

Fine by me.


>
> On the topic of SerialExecutor and PostDelayedTask, it gets complicated,
> but we have learned some lessons from MessageLoop that are important.
>  PostDelayedTask does potentially take the task out of the FIFO and push it
> further down in the execution order.  However, PostDelayedTask can never
> result in the task being moved ahead in the execution order.
>

> Internally, a PostDelayedTask should alway start out as an "internal"
> PostTask.  When the internal PostTask runs, it looks at how much delay
> still exists before it should have run, and then it gets put on a delayed
> queue to be run once that time expires if the time has not already expired.
>  Otherwise, it gets run right away.  This means that if the delay is
> processing the queue is longer than the delay passed to PostDelayedTask
> that a PostDelayedTask will effectively be treated as a PostTask.  This
> preserves FIFO'ness for the consumer.  If they call PostTask followed by
> PostDelayedTask, they can be ensured thatthe delayed task will not run
> before the non-delayed task.
>

That's an interesting point that I hadn't considered. It's not relevant to
Executor, but I guess for SerialExecutor, it does make sense. I wonder if
this will complicate the implementation of SequencedWorkerPool (I haven't
looked at the implementation details of that class). But yes, I think we
should enforce the FIFO invariant.


>
> Now, as far as PostNonNestableTask is concerned, I agree it seems a bit
> weird to say that a serial executor could run tasks in a nested fashion
> since it is supposed to serialize their execution.  Maybe that suggests
> that "serial executor" is maybe the wrong name?  How do we capture the
> possibility of someone running a nested task loop?  As soon as we allow
> nested task loops, then we need a way to post tasks that cannot run in a
> nested fashion.  This does imply a secondary task queue, but much like
> PostDelayedTask, a PostNonNestedTask starts out its life as a normal
> PostTask.  At execution time, it looks to see if it is OK to run the task.
>  If not, then it gets put on a delayed-until-unnested queue.
>

I think SerialExecutor is fine. Tasks always at least *start* in FIFO
order. Nesting just makes it possible that, before one task is finished,
that others may start and finish too. My main point is that I don't think
the concept of a nestable task belongs in the Executor interface, only in
the SerialExecutor interface. The base Executor interface makes no
guarantees on anything about how the tasks are executed, and indeed they
may be executed concurrently. So disallowing nestable tasks seems weird for
Executor, and thus I think that virtual method belongs in the
SerialExecutor interface instead.


>
> -Darin
>
>
>
>>
>>
http://codereview.chromium.**org/9169037/<http://codereview.chromium.org/9169...
>>
>
>

[akalin, 2012-02-03 18:38:49]

[On Fri, Feb 3, 2012 at 10:18 AM, Darin Fisher <darin@chromium.org> wrote:

> Guys, I'm starting to really sour on the Executor name.  I much prefer
> TaskRunner because it is so much more obvious what it means.  I realize
> that Executor is one word instead of two, but I think we should prefer
> terms that are self evident over terms that are brief.  Anyways, 10
> characters instead of 8 doesn't seem like a big difference to me.  Closures
> have a Run method.  Closures have a Run method, not an Execute method.
>

Okay, I'll change it.


> Now, as far as PostNonNestableTask is concerned, I agree it seems a bit
> weird to say that a serial executor could run tasks in a nested fashion
> since it is supposed to serialize their execution.  Maybe that suggests
> that "serial executor" is maybe the wrong name?  How do we capture the
> possibility of someone running a nested task loop?  As soon as we allow
> nested task loops, then we need a way to post tasks that cannot run in a
> nested fashion.  This does imply a secondary task queue, but much like
> PostDelayedTask, a PostNonNestedTask starts out its life as a normal
> PostTask.  At execution time, it looks to see if it is OK to run the task.
>  If not, then it gets put on a delayed-until-unnested queue.
>
>
I think willchan was more concerned with the concept of
nestable/non-nestable tasks with regular Executors/TaskRunners, and not
SerialExecutors.  That is, a regular task runner already runs tasks
overlapping, out of order, etc., so what's the difference between
nestability and overlapping/out-of-order?

But I should clarify the comments re. SerialTaskRunner about nestedness.
 What about OrderedTaskRunner?

[darin (slow to review), 2012-02-03 18:43:22]

On Fri, Feb 3, 2012 at 10:38 AM, Fred Akalin <akalin@chromium.org> wrote:

> [On Fri, Feb 3, 2012 at 10:18 AM, Darin Fisher <darin@chromium.org> wrote:
>
>> Guys, I'm starting to really sour on the Executor name.  I much prefer
>> TaskRunner because it is so much more obvious what it means.  I realize
>> that Executor is one word instead of two, but I think we should prefer
>> terms that are self evident over terms that are brief.  Anyways, 10
>> characters instead of 8 doesn't seem like a big difference to me.  Closures
>> have a Run method.  Closures have a Run method, not an Execute method.
>>
>
> Okay, I'll change it.
>
>
>> Now, as far as PostNonNestableTask is concerned, I agree it seems a bit
>> weird to say that a serial executor could run tasks in a nested fashion
>> since it is supposed to serialize their execution.  Maybe that suggests
>> that "serial executor" is maybe the wrong name?  How do we capture the
>> possibility of someone running a nested task loop?  As soon as we allow
>> nested task loops, then we need a way to post tasks that cannot run in a
>> nested fashion.  This does imply a secondary task queue, but much like
>> PostDelayedTask, a PostNonNestedTask starts out its life as a normal
>> PostTask.  At execution time, it looks to see if it is OK to run the task.
>>  If not, then it gets put on a delayed-until-unnested queue.
>>
>>
> I think willchan was more concerned with the concept of
> nestable/non-nestable tasks with regular Executors/TaskRunners, and not
> SerialExecutors.  That is, a regular task runner already runs tasks
> overlapping, out of order, etc., so what's the difference between
> nestability and overlapping/out-of-order?
>

I see.. Yeah, moving the PostNonNestable*Task methods out of Executor /
TaskRunner makes sense.  I am not as convinced about PostDelayedTask.  It
seems like even the simplest TaskRunner could support delaying a task.



>
> But I should clarify the comments re. SerialTaskRunner about nestedness.
>  What about OrderedTaskRunner?
>

OrderedTaskRunner and SerialTaskRunner seems like synonyms to me.  What is
the subtle difference you are going for?  Is it that serial sounds like it
cannot support overlapping, but ordered could?  Makes sense.

-Darin

[darin (slow to review), 2012-02-03 18:46:59]

On Fri, Feb 3, 2012 at 10:37 AM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> On Fri, Feb 3, 2012 at 10:18 AM, Darin Fisher <darin@chromium.org> wrote:
>
>>
>>
>> On Wed, Feb 1, 2012 at 10:55 PM, <akalin@chromium.org> wrote:
>>
>>>  I may need Darin to assist here since he has more experience with the
>>>> problems with nested MessageLoops. My understanding is the problem that
>>>> your example of a WorkerPool that spawns n threads, each with their own
>>>> MessageLoop, introduces no new problems with nesting. The tasks could
>>>> already run concurrently on different threads, why is it worse that a
>>>> task
>>>> is run within a task callstack on a single worker thread? It's only when
>>>> you have the expectation that it's a SerialExecutor that nesting (and
>>>> it's
>>>> associated re-entrancies and out of order processing of tasks) causes
>>>> problems.
>>>>
>>>
>>> Yeah, I see your point.  I'll defer to Darin, also.
>>>
>>>
>>>  I think that's reasonable. It's not currently supported by
>>>> SequencedWorkerPool, but I think we can argue that it should be.
>>>> Probably
>>>> there has been no need yet and that's why it's not already supported. I
>>>> defer to Brett.
>>>>
>>>
>>> Okay, me too.  Keep in mind that ignoring the delay is a valid
>>> implementation,
>>> and one we could do if we have no need for delays yet.
>>>
>>>
>>>  I think I prefer RunsTasksOnCurrentThread(), despite its flaws that you
>>>> have noted.
>>>>
>>>
>>> Okay.  What about ExecutesOnCurrentThread() to be more inline with the
>>> name?
>>> (TaskRunner : RunTasksOnCurrentThread :: Executor :
>>> ExecutesOnCurrentThread())
>>>
>>> I'll wait until everyone gets back from the trip and then we can hash
>>> out the
>>> points above.
>>>
>>
>>
>> Guys, I'm starting to really sour on the Executor name.  I much prefer
>> TaskRunner because it is so much more obvious what it means.  I realize
>> that Executor is one word instead of two, but I think we should prefer
>> terms that are self evident over terms that are brief.  Anyways, 10
>> characters instead of 8 doesn't seem like a big difference to me.  Closures
>> have a Run method.  Closures have a Run method, not an Execute method.
>>
>
> Fine by me.
>
>
>>
>> On the topic of SerialExecutor and PostDelayedTask, it gets complicated,
>> but we have learned some lessons from MessageLoop that are important.
>>  PostDelayedTask does potentially take the task out of the FIFO and push it
>> further down in the execution order.  However, PostDelayedTask can never
>> result in the task being moved ahead in the execution order.
>>
>
>> Internally, a PostDelayedTask should alway start out as an "internal"
>> PostTask.  When the internal PostTask runs, it looks at how much delay
>> still exists before it should have run, and then it gets put on a delayed
>> queue to be run once that time expires if the time has not already expired.
>>  Otherwise, it gets run right away.  This means that if the delay is
>> processing the queue is longer than the delay passed to PostDelayedTask
>> that a PostDelayedTask will effectively be treated as a PostTask.  This
>> preserves FIFO'ness for the consumer.  If they call PostTask followed by
>> PostDelayedTask, they can be ensured thatthe delayed task will not run
>> before the non-delayed task.
>>
>
> That's an interesting point that I hadn't considered. It's not relevant to
> Executor, but I guess for SerialExecutor, it does make sense. I wonder if
> this will complicate the implementation of SequencedWorkerPool (I haven't
> looked at the implementation details of that class). But yes, I think we
> should enforce the FIFO invariant.
>

You'd probably implement PostDelayedTask using PostTask.  Then, when it
runs, if there is still time before the task should run, then perhaps
schedule a timer on the current thread, or post to some other thread which
will schedule a timer, and then upon expiry of the timer, re-queue to the
SequencedWorkerPool?  I haven't thought about this enough to be sure that
it will do what we want in all cases.  I agree it seems a bit tricky...



>
>
>>
>> Now, as far as PostNonNestableTask is concerned, I agree it seems a bit
>> weird to say that a serial executor could run tasks in a nested fashion
>> since it is supposed to serialize their execution.  Maybe that suggests
>> that "serial executor" is maybe the wrong name?  How do we capture the
>> possibility of someone running a nested task loop?  As soon as we allow
>> nested task loops, then we need a way to post tasks that cannot run in a
>> nested fashion.  This does imply a secondary task queue, but much like
>> PostDelayedTask, a PostNonNestedTask starts out its life as a normal
>> PostTask.  At execution time, it looks to see if it is OK to run the task.
>>  If not, then it gets put on a delayed-until-unnested queue.
>>
>
> I think SerialExecutor is fine. Tasks always at least *start* in FIFO
> order. Nesting just makes it possible that, before one task is finished,
> that others may start and finish too. My main point is that I don't think
> the concept of a nestable task belongs in the Executor interface, only in
> the SerialExecutor interface. The base Executor interface makes no
> guarantees on anything about how the tasks are executed, and indeed they
> may be executed concurrently. So disallowing nestable tasks seems weird for
> Executor, and thus I think that virtual method belongs in the
> SerialExecutor interface instead.
>
>
>>
>> -Darin
>>
>>
>>
>>>
>>>
http://codereview.chromium.**org/9169037/<http://codereview.chromium.org/9169...
>>>
>>
>>
>

[akalin, 2012-02-03 18:51:46]

On Fri, Feb 3, 2012 at 10:43 AM, Darin Fisher <darin@chromium.org> wrote:
>
> I see.. Yeah, moving the PostNonNestable*Task methods out of Executor /
> TaskRunner makes sense.  I am not as convinced about PostDelayedTask.  It
> seems like even the simplest TaskRunner could support delaying a task.
>

Yeah, I think we all agree that TaskRunner can have PostDelayedTask.


> But I should clarify the comments re. SerialTaskRunner about nestedness.
>>  What about OrderedTaskRunner?
>>
>
> OrderedTaskRunner and SerialTaskRunner seems like synonyms to me.  What is
> the subtle difference you are going for?  Is it that serial sounds like it
> cannot support overlapping, but ordered could?  Makes sense.
>

Yeah, exactly.  "ordered" sounds more like it can allow overlapping, since
there are many orders, in particular partial orders.  Although willchan's
argument that even if the tasks aren't serialized, the task starts are, is
compelling, and would require only a few changes to the language.  I'll
think about it some more.

[willchan no longer on Chromium, 2012-02-03 18:53:37]

On Fri, Feb 3, 2012 at 10:46 AM, Darin Fisher <darin@chromium.org> wrote:

>
>
> On Fri, Feb 3, 2012 at 10:37 AM, William Chan (陈智昌) <willchan@chromium.org
> > wrote:
>
>> On Fri, Feb 3, 2012 at 10:18 AM, Darin Fisher <darin@chromium.org> wrote:
>>
>>>
>>>
>>> On Wed, Feb 1, 2012 at 10:55 PM, <akalin@chromium.org> wrote:
>>>
>>>>  I may need Darin to assist here since he has more experience with the
>>>>> problems with nested MessageLoops. My understanding is the problem that
>>>>> your example of a WorkerPool that spawns n threads, each with their own
>>>>> MessageLoop, introduces no new problems with nesting. The tasks could
>>>>> already run concurrently on different threads, why is it worse that a
>>>>> task
>>>>> is run within a task callstack on a single worker thread? It's only
>>>>> when
>>>>> you have the expectation that it's a SerialExecutor that nesting (and
>>>>> it's
>>>>> associated re-entrancies and out of order processing of tasks) causes
>>>>> problems.
>>>>>
>>>>
>>>> Yeah, I see your point.  I'll defer to Darin, also.
>>>>
>>>>
>>>>  I think that's reasonable. It's not currently supported by
>>>>> SequencedWorkerPool, but I think we can argue that it should be.
>>>>> Probably
>>>>> there has been no need yet and that's why it's not already supported. I
>>>>> defer to Brett.
>>>>>
>>>>
>>>> Okay, me too.  Keep in mind that ignoring the delay is a valid
>>>> implementation,
>>>> and one we could do if we have no need for delays yet.
>>>>
>>>>
>>>>  I think I prefer RunsTasksOnCurrentThread(), despite its flaws that you
>>>>> have noted.
>>>>>
>>>>
>>>> Okay.  What about ExecutesOnCurrentThread() to be more inline with the
>>>> name?
>>>> (TaskRunner : RunTasksOnCurrentThread :: Executor :
>>>> ExecutesOnCurrentThread())
>>>>
>>>> I'll wait until everyone gets back from the trip and then we can hash
>>>> out the
>>>> points above.
>>>>
>>>
>>>
>>> Guys, I'm starting to really sour on the Executor name.  I much prefer
>>> TaskRunner because it is so much more obvious what it means.  I realize
>>> that Executor is one word instead of two, but I think we should prefer
>>> terms that are self evident over terms that are brief.  Anyways, 10
>>> characters instead of 8 doesn't seem like a big difference to me.  Closures
>>> have a Run method.  Closures have a Run method, not an Execute method.
>>>
>>
>> Fine by me.
>>
>>
>>>
>>> On the topic of SerialExecutor and PostDelayedTask, it gets complicated,
>>> but we have learned some lessons from MessageLoop that are important.
>>>  PostDelayedTask does potentially take the task out of the FIFO and push it
>>> further down in the execution order.  However, PostDelayedTask can never
>>> result in the task being moved ahead in the execution order.
>>>
>>
>>> Internally, a PostDelayedTask should alway start out as an "internal"
>>> PostTask.  When the internal PostTask runs, it looks at how much delay
>>> still exists before it should have run, and then it gets put on a delayed
>>> queue to be run once that time expires if the time has not already expired.
>>>  Otherwise, it gets run right away.  This means that if the delay is
>>> processing the queue is longer than the delay passed to PostDelayedTask
>>> that a PostDelayedTask will effectively be treated as a PostTask.  This
>>> preserves FIFO'ness for the consumer.  If they call PostTask followed by
>>> PostDelayedTask, they can be ensured thatthe delayed task will not run
>>> before the non-delayed task.
>>>
>>
>> That's an interesting point that I hadn't considered. It's not relevant
>> to Executor, but I guess for SerialExecutor, it does make sense. I wonder
>> if this will complicate the implementation of SequencedWorkerPool (I
>> haven't looked at the implementation details of that class). But yes, I
>> think we should enforce the FIFO invariant.
>>
>
> You'd probably implement PostDelayedTask using PostTask.  Then, when it
> runs, if there is still time before the task should run, then perhaps
> schedule a timer on the current thread, or post to some other thread which
> will schedule a timer, and then upon expiry of the timer, re-queue to the
> SequencedWorkerPool?  I haven't thought about this enough to be sure that
> it will do what we want in all cases.  I agree it seems a bit tricky...
>

I think it's doable. Yes, let's make all TaskRunners support
PostDelayedTask. I think supporting it is tractable.


>
>
>
>>
>>
>>>
>>> Now, as far as PostNonNestableTask is concerned, I agree it seems a bit
>>> weird to say that a serial executor could run tasks in a nested fashion
>>> since it is supposed to serialize their execution.  Maybe that suggests
>>> that "serial executor" is maybe the wrong name?  How do we capture the
>>> possibility of someone running a nested task loop?  As soon as we allow
>>> nested task loops, then we need a way to post tasks that cannot run in a
>>> nested fashion.  This does imply a secondary task queue, but much like
>>> PostDelayedTask, a PostNonNestedTask starts out its life as a normal
>>> PostTask.  At execution time, it looks to see if it is OK to run the task.
>>>  If not, then it gets put on a delayed-until-unnested queue.
>>>
>>
>> I think SerialExecutor is fine. Tasks always at least *start* in FIFO
>> order. Nesting just makes it possible that, before one task is finished,
>> that others may start and finish too. My main point is that I don't think
>> the concept of a nestable task belongs in the Executor interface, only in
>> the SerialExecutor interface. The base Executor interface makes no
>> guarantees on anything about how the tasks are executed, and indeed they
>> may be executed concurrently. So disallowing nestable tasks seems weird for
>> Executor, and thus I think that virtual method belongs in the
>> SerialExecutor interface instead.
>>
>>
>>>
>>> -Darin
>>>
>>>
>>>
>>>>
>>>>
http://codereview.chromium.**org/9169037/<http://codereview.chromium.org/9169...
>>>>
>>>
>>>
>>
>

[willchan no longer on Chromium, 2012-02-03 18:55:59]

On Fri, Feb 3, 2012 at 10:51 AM, Fred Akalin <akalin@chromium.org> wrote:

> On Fri, Feb 3, 2012 at 10:43 AM, Darin Fisher <darin@chromium.org> wrote:
>>
>> I see.. Yeah, moving the PostNonNestable*Task methods out of Executor /
>> TaskRunner makes sense.  I am not as convinced about PostDelayedTask.  It
>> seems like even the simplest TaskRunner could support delaying a task.
>>
>
> Yeah, I think we all agree that TaskRunner can have PostDelayedTask.
>
>
>> But I should clarify the comments re. SerialTaskRunner about nestedness.
>>>  What about OrderedTaskRunner?
>>>
>>
>> OrderedTaskRunner and SerialTaskRunner seems like synonyms to me.  What
>> is the subtle difference you are going for?  Is it that serial sounds like
>> it cannot support overlapping, but ordered could?  Makes sense.
>>
>
> Yeah, exactly.  "ordered" sounds more like it can allow overlapping, since
> there are many orders, in particular partial orders.  Although willchan's
> argument that even if the tasks aren't serialized, the task starts are, is
> compelling, and would require only a few changes to the language.  I'll
> think about it some more.
>

I don't feel strongly about either name. I think it's confusing no matter
what :P Good comments are necessary here. Hopefully most people won't deal
with nested TaskRunners in the first place, and the people who do will read
the comments.

[brettw, 2012-02-03 22:42:06]

Sorry I didn't read all of the comments...

I also like TaskRunner better than Executor.

I think the ordered one should be named Sequenced since that's what the
SequencedWorkerPool uses. I think they're all just as ambiguous. If we think
another name is better, we should rename the SequencedWorkerPool to match.

One comment said that SequencedWorkerPool would implement SequencedExecutor.
This isn't the case. It would implement the regular non-sequenced one, which
would mean "run on any thread." You would have to write wrappers around a
sequence token to ensure ordering between the tasks you want to be sequenced.
Therefore, you would have many SequencedExecutors for the worker pool.

Note that SequencedWorkerPool does not support delay and adding such support
properly would be nontrivial.

[brettw, 2012-02-03 22:42:14]

http://codereview.chromium.org/9169037/diff/22001/base/serial_executor.h
File base/serial_executor.h (right):

http://codereview.chromium.org/9169037/diff/22001/base/serial_executor.h#newc...
base/serial_executor.h:18: //     T2 will be executed after T1 iff:
Just write "if". "iff" is a non-word jumps out and makes reading more difficult
and it's just as precise in this context (nobody means "if and not only if" in
code).

http://codereview.chromium.org/9169037/diff/22001/base/serial_executor.h#newc...
base/serial_executor.h:30: // Some corollaries to the above guarantees, in order
of increasing
Does "in order of increasing generality" help anything here? I'd just remove it.

[willchan no longer on Chromium, 2012-02-03 22:50:46]

On Fri, Feb 3, 2012 at 2:42 PM, <brettw@chromium.org> wrote:

> Sorry I didn't read all of the comments...
>
> I also like TaskRunner better than Executor.
>
> I think the ordered one should be named Sequenced since that's what the
> SequencedWorkerPool uses. I think they're all just as ambiguous. If we
> think
> another name is better, we should rename the SequencedWorkerPool to match.
>
> One comment said that SequencedWorkerPool would implement
> SequencedExecutor.
> This isn't the case. It would implement the regular non-sequenced one,
> which
> would mean "run on any thread." You would have to write wrappers around a
> sequence token to ensure ordering between the tasks you want to be
> sequenced.
> Therefore, you would have many SequencedExecutors for the worker pool.
>

I think you may have misread the comments. Yes, I agree that
SequencedWorkerPool should inherit from the base interface, not the
serial/ordered/sequenced one. BUT, we could do something where we
instantiate a serial/ordered/sequenced one that contains a pointer to the
SequencedWorkerPool and contains a SequenceToken. So anything posted to
that SequencedExecutor would post to the SequencedWorkerPool using the same
SequenceToken. For testing purposes, we could swap in a mock
SequencedExecutor, instead of using the underlying
SequencedWorkerPool+SequenceToken.


>
> Note that SequencedWorkerPool does not support delay and adding such
> support
> properly would be nontrivial.
>

I suspect that is true, but do you believe we should not add support for
delayed tasks? Or just want to warn that it'll be difficult? Without
looking at the code in detail, just basing off what is "desirable" IMO from
an interface perspective, I think we should support delay in
SequencedWorkerPool, and I do suspect it might be tricky.


>
>
http://codereview.chromium.**org/9169037/<http://codereview.chromium.org/9169...
>

[akalin, 2012-02-03 23:48:14]

> I see.. Yeah, moving the PostNonNestable*Task methods out of Executor /
> TaskRunner makes sense.  I am not as convinced about PostDelayedTask.  It
> seems like even the simplest TaskRunner could support delaying a task.

What shall we do about DeleteSoon/ReleaseSoon?  Those use PostNonNestable*Task. 
So either we move those out of TaskRunner, too, or define some sort of virtual
method that it uses that calls PostTask for Executor and PostNonNestable*Task
for SerialTaskRunner.

I'll go with the latter for now.

[brettw, 2012-02-04 03:55:51]

> > Note that SequencedWorkerPool does not support delay and adding such
> > support
> > properly would be nontrivial.
> >
> 
> I suspect that is true, but do you believe we should not add support for
> delayed tasks? Or just want to warn that it'll be difficult? Without
> looking at the code in detail, just basing off what is "desirable" IMO from
> an interface perspective, I think we should support delay in
> SequencedWorkerPool, and I do suspect it might be tricky.

I agree that it would be desirable from a completeness perspective. I'm mostly
warning that it will be a bunch of work, and also of limited value. I can invent
some cases where we'd want to run delayed tasks on the pool (need to check that
a file hasn't changed after 10 seconds) but the use cases are somewhat
contrived.

[darin (slow to review), 2012-02-04 05:29:37]

On Fri, Feb 3, 2012 at 3:48 PM, <akalin@chromium.org> wrote:

> I see.. Yeah, moving the PostNonNestable*Task methods out of Executor /
>> TaskRunner makes sense.  I am not as convinced about PostDelayedTask.  It
>> seems like even the simplest TaskRunner could support delaying a task.
>>
>
> What shall we do about DeleteSoon/ReleaseSoon?  Those use
> PostNonNestable*Task.
> So either we move those out of TaskRunner, too, or define some sort of
> virtual
> method that it uses that calls PostTask for Executor and
> PostNonNestable*Task
> for SerialTaskRunner.
>
> I'll go with the latter for now.
>

OK

... the concern with using PostTask is that it can cause problems if
someone spins a nested message loop.

Suppose a TaskRunner is backed by a ThreadPool, but suppose the threads in
the ThreadPool have a MessageLoop.  Maybe they don't in our world, and
therefore maybe we shouldn't worry about if they do?

-Darin




>
>
http://codereview.chromium.**org/9169037/<http://codereview.chromium.org/9169...
>

[akalin, 2012-02-04 06:49:47]

>
> ... the concern with using PostTask is that it can cause problems if
> someone spins a nested message loop.
>
>
What exactly is the concern with DeleteSoon, PostTask, and nested message
loops?

[darin (slow to review), 2012-02-04 06:57:40]

On Fri, Feb 3, 2012 at 10:49 PM, Fred Akalin <akalin@chromium.org> wrote:

> ... the concern with using PostTask is that it can cause problems if
>> someone spins a nested message loop.
>>
>>
> What exactly is the concern with DeleteSoon, PostTask, and nested message
> loops?
>

Oh, sorry... it goes like this:  people use DeleteSoon to postpone
destruction of an object until they are done using it.  Often, deep within
a call stack you might find that you need to delete an object, but you
might actually return to a method of the object, and it might attempt to
dereference its members.  That would crash unless destruction of the object
was postponed.  Hence, DeleteSoon.

If DeleteSoon is not implemented using PostNonNestedTask, then all it takes
is someone pumping nested events to cause the above countermeasure to fail.

-Darin

[willchan no longer on Chromium, 2012-02-04 07:59:56]

On Fri, Feb 3, 2012 at 7:55 PM, <brettw@chromium.org> wrote:

> > Note that SequencedWorkerPool does not support delay and adding such
>> > support
>> > properly would be nontrivial.
>> >
>>
>
>  I suspect that is true, but do you believe we should not add support for
>> delayed tasks? Or just want to warn that it'll be difficult? Without
>> looking at the code in detail, just basing off what is "desirable" IMO
>> from
>> an interface perspective, I think we should support delay in
>> SequencedWorkerPool, and I do suspect it might be tricky.
>>
>
> I agree that it would be desirable from a completeness perspective. I'm
> mostly
> warning that it will be a bunch of work, and also of limited value. I can
> invent
> some cases where we'd want to run delayed tasks on the pool (need to check
> that
> a file hasn't changed after 10 seconds) but the use cases are somewhat
> contrived.
>

That sounds accurate to me. How about we just use a CHECK(false) or a
NOTREACHED() and when a use case comes along, we can either say it's silly
or at that point implement it?


>
http://codereview.chromium.**org/9169037/<http://codereview.chromium.org/9169...
>

[willchan no longer on Chromium, 2012-02-04 08:18:11]

On Fri, Feb 3, 2012 at 9:29 PM, Darin Fisher <darin@chromium.org> wrote:

> On Fri, Feb 3, 2012 at 3:48 PM, <akalin@chromium.org> wrote:
>
>>  I see.. Yeah, moving the PostNonNestable*Task methods out of Executor /
>>> TaskRunner makes sense.  I am not as convinced about PostDelayedTask.  It
>>> seems like even the simplest TaskRunner could support delaying a task.
>>>
>>
>> What shall we do about DeleteSoon/ReleaseSoon?  Those use
>> PostNonNestable*Task.
>> So either we move those out of TaskRunner, too, or define some sort of
>> virtual
>> method that it uses that calls PostTask for Executor and
>> PostNonNestable*Task
>> for SerialTaskRunner.
>>
>> I'll go with the latter for now.
>>
>
> OK
>
> ... the concern with using PostTask is that it can cause problems if
> someone spins a nested message loop.
>
> Suppose a TaskRunner is backed by a ThreadPool, but suppose the threads in
> the ThreadPool have a MessageLoop.  Maybe they don't in our world, and
> therefore maybe we shouldn't worry about if they do?
>

If it's a generic TaskRunner and not a Serial/Ordered/SequencedTaskRunner,
then I think there's no guarantee about the order in which the DeleteSoon()
happens. If the DeleteSoon() needs to happen after a task that is currently
running on the TaskRunner completes, then it relies on semantics which are
not guaranteed by the generic TaskRunner interface. Currently, only
SerialTaskRunner provides such guarantees. I think DeleteSoon() should be
removed from TaskRunner, since I think (as Darin notes below), that the
main reason to use DeleteSoon() is to make sure that the deletion happens
after the current task finishes using the object.


> -Darin
>
>
>
>
>>
>>
http://codereview.chromium.**org/9169037/<http://codereview.chromium.org/9169...
>>
>
>

[darin (slow to review), 2012-02-04 18:09:13]

On Sat, Feb 4, 2012 at 12:18 AM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> On Fri, Feb 3, 2012 at 9:29 PM, Darin Fisher <darin@chromium.org> wrote:
>
>> On Fri, Feb 3, 2012 at 3:48 PM, <akalin@chromium.org> wrote:
>>
>>>  I see.. Yeah, moving the PostNonNestable*Task methods out of Executor /
>>>> TaskRunner makes sense.  I am not as convinced about PostDelayedTask.
>>>>  It
>>>> seems like even the simplest TaskRunner could support delaying a task.
>>>>
>>>
>>> What shall we do about DeleteSoon/ReleaseSoon?  Those use
>>> PostNonNestable*Task.
>>> So either we move those out of TaskRunner, too, or define some sort of
>>> virtual
>>> method that it uses that calls PostTask for Executor and
>>> PostNonNestable*Task
>>> for SerialTaskRunner.
>>>
>>> I'll go with the latter for now.
>>>
>>
>> OK
>>
>> ... the concern with using PostTask is that it can cause problems if
>> someone spins a nested message loop.
>>
>> Suppose a TaskRunner is backed by a ThreadPool, but suppose the threads
>> in the ThreadPool have a MessageLoop.  Maybe they don't in our world, and
>> therefore maybe we shouldn't worry about if they do?
>>
>
> If it's a generic TaskRunner and not a Serial/Ordered/SequencedTaskRunner,
> then I think there's no guarantee about the order in which the DeleteSoon()
> happens. If the DeleteSoon() needs to happen after a task that is currently
> running on the TaskRunner completes, then it relies on semantics which are
> not guaranteed by the generic TaskRunner interface. Currently, only
> SerialTaskRunner provides such guarantees. I think DeleteSoon() should be
> removed from TaskRunner, since I think (as Darin notes below), that the
> main reason to use DeleteSoon() is to make sure that the deletion happens
> after the current task finishes using the object.
>
>
Agreed!


>
>> -Darin
>>
>>
>>
>>
>>>
>>>
http://codereview.chromium.**org/9169037/<http://codereview.chromium.org/9169...
>>>
>>
>>
>

[akalin, 2012-02-07 03:01:59]

Hey everyone,

I uploaded a new patch, which I think addresses everyone's concerns. 
Highlights:

- Renamed back to *TaskRunner.
- Renamed SerialExecutor to SequencedTaskRunner.
- Move PostNonNestable*Task to SerialExecutor.
- Renamed MayRunTasksOnCurrentThread() to RunsTasksOnCurrentThread().
- Addressed brett's and willchan's code review comments.
- Elaborated on nestable vs. non-nestable task behavior in SequencedTaskRunner
comments.

Please take another look!

[akalin, 2012-02-08 00:37:04]

pinging darin + willchan + brett!

On 2012/02/07 03:01:59, akalin wrote:
> Hey everyone,
> 
> I uploaded a new patch, which I think addresses everyone's concerns. 
> Highlights:
> 
> - Renamed back to *TaskRunner.
> - Renamed SerialExecutor to SequencedTaskRunner.
> - Move PostNonNestable*Task to SerialExecutor.
> - Renamed MayRunTasksOnCurrentThread() to RunsTasksOnCurrentThread().
> - Addressed brett's and willchan's code review comments.
> - Elaborated on nestable vs. non-nestable task behavior in SequencedTaskRunner
> comments.
> 
> Please take another look!

[willchan no longer on Chromium, 2012-02-08 00:46:37]

I skimmed it and think I am fine with everything now. I will re-read to make
sure the comments look good.

[brettw, 2012-02-09 00:26:04]

I did a high-level look through. Looks fine to me, I'll go along with Will's
final low-level review, so don't wait for me to LGTM again.

http://codereview.chromium.org/9169037/diff/29002/base/task_runner.h
File base/task_runner.h (right):

http://codereview.chromium.org/9169037/diff/29002/base/task_runner.h#newcode51
base/task_runner.h:51: //   - An TaskRunner that uses a worker pool to run
submitted tasks.
Grammar: An -> A (same in the next paras).

[akalin, 2012-02-09 00:36:05]

Will, any more comments?

http://codereview.chromium.org/9169037/diff/29002/base/task_runner.h
File base/task_runner.h (right):

http://codereview.chromium.org/9169037/diff/29002/base/task_runner.h#newcode51
base/task_runner.h:51: //   - An TaskRunner that uses a worker pool to run
submitted tasks.
On 2012/02/09 00:26:04, brettw wrote:
> Grammar: An -> A (same in the next paras).

Done.

[willchan no longer on Chromium, 2012-02-09 20:50:50]

http://codereview.chromium.org/9169037/diff/42001/base/message_loop.h
File base/message_loop.h (right):

http://codereview.chromium.org/9169037/diff/42001/base/message_loop.h#newcode15
base/message_loop.h:15: #include "base/sequenced_task_runner_helpers.h"
sort

http://codereview.chromium.org/9169037/diff/42001/base/sequenced_task_runner.h
File base/sequenced_task_runner.h (right):

http://codereview.chromium.org/9169037/diff/42001/base/sequenced_task_runner....
base/sequenced_task_runner.h:15: // A SequencedTaskRunner is an extension of
TaskRunner that provides
Sorry to be pedantic, but let's use subclass / derived class instead of
extension.

http://codereview.chromium.org/9169037/diff/42001/base/sequenced_task_runner....
base/sequenced_task_runner.h:26: // already-running task.
Please add a rough oversimplified explanation somewhere early on for people who
don't want/need to read the rules below. Something like,

Barring delayed/non-nestable-tasks, tasks will execute one by one in FIFO order.

http://codereview.chromium.org/9169037/diff/42001/base/sequenced_task_runner....
base/sequenced_task_runner.h:30: //   - Given two tasks T2 and T1 that are
posted from the same thread,
They don't have to be posted from the same thread, right?

http://codereview.chromium.org/9169037/diff/42001/base/sequenced_task_runner....
base/sequenced_task_runner.h:34: //       * T2 has equal or higher delay than
T1; and
This is imprecise, but I don't have a better concise wording. I'm fine as is. It
is imprecise because "delay" is imprecise, it's actually the post time + delay
time that matters, not just the absolute value of the delay. I'm being pedantic
again by nitting here. Feel free to ignore me.

http://codereview.chromium.org/9169037/diff/42001/base/sequenced_task_runner....
base/sequenced_task_runner.h:41: //       * T1 doesn't call any task-running
methods of S.
S has not been defined

http://codereview.chromium.org/9169037/diff/42001/base/sequenced_task_runner....
base/sequenced_task_runner.h:90: public:
Shift 1 column, ditch the extra newline as per style guide.

http://codereview.chromium.org/9169037/diff/42001/base/sequenced_task_runner_...
File base/sequenced_task_runner_helpers.h (right):

http://codereview.chromium.org/9169037/diff/42001/base/sequenced_task_runner_...
base/sequenced_task_runner_helpers.h:26: // Template helpers which use a
function indirection to erase T from
s/a //

http://codereview.chromium.org/9169037/diff/42001/base/sequenced_task_runner_...
base/sequenced_task_runner_helpers.h:36: private:
I think you're being too draconian here with all the privates and friends. One
level of draconianness is enough. I think putting this in base::internal should
be just fine (it's not used outside of base/). I think internal is more
appropriate than subtle.

http://codereview.chromium.org/9169037/diff/42001/base/task_runner.cc
File base/task_runner.cc (right):

http://codereview.chromium.org/9169037/diff/42001/base/task_runner.cc#newcode25
base/task_runner.cc:25: // Non-owning.
Sorry, I'm tired. Why is it OK for this to be non-owning? Shouldn't it be a
scoped_refptr?

http://codereview.chromium.org/9169037/diff/42001/base/task_runner.cc#newcode30
base/task_runner.cc:30: TaskRunner* destination) : destination_(destination) {}
DCHECK early on that it's non-NULL.

http://codereview.chromium.org/9169037/diff/42001/base/task_runner.h
File base/task_runner.h (right):

http://codereview.chromium.org/9169037/diff/42001/base/task_runner.h#newcode22
base/task_runner.h:22: // A TaskRunner is an object that runs submitted tasks
(in the form of
s/submitted/posted/

http://codereview.chromium.org/9169037/diff/42001/base/task_runner.h#newcode29
base/task_runner.h:29: //   - Submitting a task will not run it synchronously. 
That is, no
Replace all the submit with post.

http://codereview.chromium.org/9169037/diff/42001/base/task_runner.h#newcode32
base/task_runner.h:32: //   - Increasing the delay can only delay when the task
gets run.
It seems like this section is unnecessary (too obvious). Is there a subtlety I'm
not noticing?

"it won't make it run earlier than it normally would"?

http://codereview.chromium.org/9169037/diff/42001/base/task_runner.h#newcode51
base/task_runner.h:51: //   - A TaskRunner that uses a worker pool to run
submitted tasks.
s/worker pool/thread pool/ to make it more obvious it can be multithreaded.

http://codereview.chromium.org/9169037/diff/42001/base/task_runner.h#newcode68
base/task_runner.h:68: // TODO(akalin): Consider forwarding this to
PostDelayedTask(_, _,
What's your thought here? Do it or not? We should delete the TODO and just do
one of these. I kinda like eliminating the virtual.

[akalin, 2012-02-10 22:48:59]

addressed all comments, PTAL

http://codereview.chromium.org/9169037/diff/42001/base/message_loop.h
File base/message_loop.h (right):

http://codereview.chromium.org/9169037/diff/42001/base/message_loop.h#newcode15
base/message_loop.h:15: #include "base/sequenced_task_runner_helpers.h"
On 2012/02/09 20:50:50, willchan wrote:
> sort

Done.

http://codereview.chromium.org/9169037/diff/42001/base/sequenced_task_runner.h
File base/sequenced_task_runner.h (right):

http://codereview.chromium.org/9169037/diff/42001/base/sequenced_task_runner....
base/sequenced_task_runner.h:15: // A SequencedTaskRunner is an extension of
TaskRunner that provides
On 2012/02/09 20:50:50, willchan wrote:
> Sorry to be pedantic, but let's use subclass / derived class instead of
> extension.

Done.

http://codereview.chromium.org/9169037/diff/42001/base/sequenced_task_runner....
base/sequenced_task_runner.h:26: // already-running task.
On 2012/02/09 20:50:50, willchan wrote:
> Please add a rough oversimplified explanation somewhere early on for people
who
> don't want/need to read the rules below. Something like,
> 
> Barring delayed/non-nestable-tasks, tasks will execute one by one in FIFO
order.

Done.

http://codereview.chromium.org/9169037/diff/42001/base/sequenced_task_runner....
base/sequenced_task_runner.h:30: //   - Given two tasks T2 and T1 that are
posted from the same thread,
On 2012/02/09 20:50:50, willchan wrote:
> They don't have to be posted from the same thread, right?

Well, are there cases where we care about the ordering of tasks posted from
different threads?  I can't think of one, and talking about it would mandate
implementations to essentially have a shared queue, which I don't think is
really necessary.

http://codereview.chromium.org/9169037/diff/42001/base/sequenced_task_runner....
base/sequenced_task_runner.h:34: //       * T2 has equal or higher delay than
T1; and
On 2012/02/09 20:50:50, willchan wrote:
> This is imprecise, but I don't have a better concise wording. I'm fine as is.
It
> is imprecise because "delay" is imprecise, it's actually the post time + delay
> time that matters, not just the absolute value of the delay. I'm being
pedantic
> again by nitting here. Feel free to ignore me.

It's not imprecise, if you assume they're posted from the same thread and T2 is
posted after T1 (which implies that T1's post time < T2's post time).

http://codereview.chromium.org/9169037/diff/42001/base/sequenced_task_runner....
base/sequenced_task_runner.h:41: //       * T1 doesn't call any task-running
methods of S.
On 2012/02/09 20:50:50, willchan wrote:
> S has not been defined

Fixed.

http://codereview.chromium.org/9169037/diff/42001/base/sequenced_task_runner....
base/sequenced_task_runner.h:90: public:
On 2012/02/09 20:50:50, willchan wrote:
> Shift 1 column, ditch the extra newline as per style guide.

Done.

http://codereview.chromium.org/9169037/diff/42001/base/sequenced_task_runner_...
File base/sequenced_task_runner_helpers.h (right):

http://codereview.chromium.org/9169037/diff/42001/base/sequenced_task_runner_...
base/sequenced_task_runner_helpers.h:26: // Template helpers which use a
function indirection to erase T from
On 2012/02/09 20:50:50, willchan wrote:
> s/a //

Done.

http://codereview.chromium.org/9169037/diff/42001/base/sequenced_task_runner_...
base/sequenced_task_runner_helpers.h:36: private:
On 2012/02/09 20:50:50, willchan wrote:
> I think you're being too draconian here with all the privates and friends. One
> level of draconianness is enough. I think putting this in base::internal
should
> be just fine (it's not used outside of base/). I think internal is more
> appropriate than subtle.

Moving DeleteHelper into internal won't work, because tons of classes (mostly
ref-counted ones) have to friend base::DeleteHelper in order for stuff to work.

FYI, this is just a straight move of message_loop_helpers.h

http://codereview.chromium.org/9169037/diff/42001/base/task_runner.cc
File base/task_runner.cc (right):

http://codereview.chromium.org/9169037/diff/42001/base/task_runner.cc#newcode25
base/task_runner.cc:25: // Non-owning.
On 2012/02/09 20:50:50, willchan wrote:
> Sorry, I'm tired. Why is it OK for this to be non-owning? Shouldn't it be a
> scoped_refptr?

This class is essentially just to define a PostTask.  In PostTaskAndReply, this
class is created, PostTaskAndReply is called, then the object is immediately
destroyed.

http://codereview.chromium.org/9169037/diff/42001/base/task_runner.cc#newcode30
base/task_runner.cc:30: TaskRunner* destination) : destination_(destination) {}
On 2012/02/09 20:50:50, willchan wrote:
> DCHECK early on that it's non-NULL.

Done.

http://codereview.chromium.org/9169037/diff/42001/base/task_runner.h
File base/task_runner.h (right):

http://codereview.chromium.org/9169037/diff/42001/base/task_runner.h#newcode22
base/task_runner.h:22: // A TaskRunner is an object that runs submitted tasks
(in the form of
On 2012/02/09 20:50:50, willchan wrote:
> s/submitted/posted/

Done.

http://codereview.chromium.org/9169037/diff/42001/base/task_runner.h#newcode29
base/task_runner.h:29: //   - Submitting a task will not run it synchronously. 
That is, no
On 2012/02/09 20:50:50, willchan wrote:
> Replace all the submit with post.

Done.

http://codereview.chromium.org/9169037/diff/42001/base/task_runner.h#newcode32
base/task_runner.h:32: //   - Increasing the delay can only delay when the task
gets run.
On 2012/02/09 20:50:50, willchan wrote:
> It seems like this section is unnecessary (too obvious). Is there a subtlety
I'm
> not noticing?
> 
> "it won't make it run earlier than it normally would"?

It is pretty obvious, but I think it's worthwhile to state.  It implies that
implementations may either ignore the parameter or use it, but only in such a
way that it actually works.

http://codereview.chromium.org/9169037/diff/42001/base/task_runner.h#newcode51
base/task_runner.h:51: //   - A TaskRunner that uses a worker pool to run
submitted tasks.
On 2012/02/09 20:50:50, willchan wrote:
> s/worker pool/thread pool/ to make it more obvious it can be multithreaded.

Done.

http://codereview.chromium.org/9169037/diff/42001/base/task_runner.h#newcode68
base/task_runner.h:68: // TODO(akalin): Consider forwarding this to
PostDelayedTask(_, _,
On 2012/02/09 20:50:50, willchan wrote:
> What's your thought here? Do it or not? We should delete the TODO and just do
> one of these. I kinda like eliminating the virtual.

Okay, I just implemented this TODO.

[akalin, 2012-02-13 18:24:20]

Ping, darin and willchan!

On 2012/02/10 22:48:59, akalin wrote:
> addressed all comments, PTAL
> 
> http://codereview.chromium.org/9169037/diff/42001/base/message_loop.h
> File base/message_loop.h (right):
> 
>
http://codereview.chromium.org/9169037/diff/42001/base/message_loop.h#newcode15
> base/message_loop.h:15: #include "base/sequenced_task_runner_helpers.h"
> On 2012/02/09 20:50:50, willchan wrote:
> > sort
> 
> Done.
> 
> http://codereview.chromium.org/9169037/diff/42001/base/sequenced_task_runner.h
> File base/sequenced_task_runner.h (right):
> 
>
http://codereview.chromium.org/9169037/diff/42001/base/sequenced_task_runner....
> base/sequenced_task_runner.h:15: // A SequencedTaskRunner is an extension of
> TaskRunner that provides
> On 2012/02/09 20:50:50, willchan wrote:
> > Sorry to be pedantic, but let's use subclass / derived class instead of
> > extension.
> 
> Done.
> 
>
http://codereview.chromium.org/9169037/diff/42001/base/sequenced_task_runner....
> base/sequenced_task_runner.h:26: // already-running task.
> On 2012/02/09 20:50:50, willchan wrote:
> > Please add a rough oversimplified explanation somewhere early on for people
> who
> > don't want/need to read the rules below. Something like,
> > 
> > Barring delayed/non-nestable-tasks, tasks will execute one by one in FIFO
> order.
> 
> Done.
> 
>
http://codereview.chromium.org/9169037/diff/42001/base/sequenced_task_runner....
> base/sequenced_task_runner.h:30: //   - Given two tasks T2 and T1 that are
> posted from the same thread,
> On 2012/02/09 20:50:50, willchan wrote:
> > They don't have to be posted from the same thread, right?
> 
> Well, are there cases where we care about the ordering of tasks posted from
> different threads?  I can't think of one, and talking about it would mandate
> implementations to essentially have a shared queue, which I don't think is
> really necessary.
> 
>
http://codereview.chromium.org/9169037/diff/42001/base/sequenced_task_runner....
> base/sequenced_task_runner.h:34: //       * T2 has equal or higher delay than
> T1; and
> On 2012/02/09 20:50:50, willchan wrote:
> > This is imprecise, but I don't have a better concise wording. I'm fine as
is.
> It
> > is imprecise because "delay" is imprecise, it's actually the post time +
delay
> > time that matters, not just the absolute value of the delay. I'm being
> pedantic
> > again by nitting here. Feel free to ignore me.
> 
> It's not imprecise, if you assume they're posted from the same thread and T2
is
> posted after T1 (which implies that T1's post time < T2's post time).
> 
>
http://codereview.chromium.org/9169037/diff/42001/base/sequenced_task_runner....
> base/sequenced_task_runner.h:41: //       * T1 doesn't call any task-running
> methods of S.
> On 2012/02/09 20:50:50, willchan wrote:
> > S has not been defined
> 
> Fixed.
> 
>
http://codereview.chromium.org/9169037/diff/42001/base/sequenced_task_runner....
> base/sequenced_task_runner.h:90: public:
> On 2012/02/09 20:50:50, willchan wrote:
> > Shift 1 column, ditch the extra newline as per style guide.
> 
> Done.
> 
>
http://codereview.chromium.org/9169037/diff/42001/base/sequenced_task_runner_...
> File base/sequenced_task_runner_helpers.h (right):
> 
>
http://codereview.chromium.org/9169037/diff/42001/base/sequenced_task_runner_...
> base/sequenced_task_runner_helpers.h:26: // Template helpers which use a
> function indirection to erase T from
> On 2012/02/09 20:50:50, willchan wrote:
> > s/a //
> 
> Done.
> 
>
http://codereview.chromium.org/9169037/diff/42001/base/sequenced_task_runner_...
> base/sequenced_task_runner_helpers.h:36: private:
> On 2012/02/09 20:50:50, willchan wrote:
> > I think you're being too draconian here with all the privates and friends.
One
> > level of draconianness is enough. I think putting this in base::internal
> should
> > be just fine (it's not used outside of base/). I think internal is more
> > appropriate than subtle.
> 
> Moving DeleteHelper into internal won't work, because tons of classes (mostly
> ref-counted ones) have to friend base::DeleteHelper in order for stuff to
work.
> 
> FYI, this is just a straight move of message_loop_helpers.h
> 
> http://codereview.chromium.org/9169037/diff/42001/base/task_runner.cc
> File base/task_runner.cc (right):
> 
>
http://codereview.chromium.org/9169037/diff/42001/base/task_runner.cc#newcode25
> base/task_runner.cc:25: // Non-owning.
> On 2012/02/09 20:50:50, willchan wrote:
> > Sorry, I'm tired. Why is it OK for this to be non-owning? Shouldn't it be a
> > scoped_refptr?
> 
> This class is essentially just to define a PostTask.  In PostTaskAndReply,
this
> class is created, PostTaskAndReply is called, then the object is immediately
> destroyed.
> 
>
http://codereview.chromium.org/9169037/diff/42001/base/task_runner.cc#newcode30
> base/task_runner.cc:30: TaskRunner* destination) : destination_(destination)
{}
> On 2012/02/09 20:50:50, willchan wrote:
> > DCHECK early on that it's non-NULL.
> 
> Done.
> 
> http://codereview.chromium.org/9169037/diff/42001/base/task_runner.h
> File base/task_runner.h (right):
> 
> http://codereview.chromium.org/9169037/diff/42001/base/task_runner.h#newcode22
> base/task_runner.h:22: // A TaskRunner is an object that runs submitted tasks
> (in the form of
> On 2012/02/09 20:50:50, willchan wrote:
> > s/submitted/posted/
> 
> Done.
> 
> http://codereview.chromium.org/9169037/diff/42001/base/task_runner.h#newcode29
> base/task_runner.h:29: //   - Submitting a task will not run it synchronously.

> That is, no
> On 2012/02/09 20:50:50, willchan wrote:
> > Replace all the submit with post.
> 
> Done.
> 
> http://codereview.chromium.org/9169037/diff/42001/base/task_runner.h#newcode32
> base/task_runner.h:32: //   - Increasing the delay can only delay when the
task
> gets run.
> On 2012/02/09 20:50:50, willchan wrote:
> > It seems like this section is unnecessary (too obvious). Is there a subtlety
> I'm
> > not noticing?
> > 
> > "it won't make it run earlier than it normally would"?
> 
> It is pretty obvious, but I think it's worthwhile to state.  It implies that
> implementations may either ignore the parameter or use it, but only in such a
> way that it actually works.
> 
> http://codereview.chromium.org/9169037/diff/42001/base/task_runner.h#newcode51
> base/task_runner.h:51: //   - A TaskRunner that uses a worker pool to run
> submitted tasks.
> On 2012/02/09 20:50:50, willchan wrote:
> > s/worker pool/thread pool/ to make it more obvious it can be multithreaded.
> 
> Done.
> 
> http://codereview.chromium.org/9169037/diff/42001/base/task_runner.h#newcode68
> base/task_runner.h:68: // TODO(akalin): Consider forwarding this to
> PostDelayedTask(_, _,
> On 2012/02/09 20:50:50, willchan wrote:
> > What's your thought here? Do it or not? We should delete the TODO and just
do
> > one of these. I kinda like eliminating the virtual.
> 
> Okay, I just implemented this TODO.

[willchan no longer on Chromium, 2012-02-14 05:59:07]

http://codereview.chromium.org/9169037/diff/42001/base/sequenced_task_runner.h
File base/sequenced_task_runner.h (right):

http://codereview.chromium.org/9169037/diff/42001/base/sequenced_task_runner....
base/sequenced_task_runner.h:30: //   - Given two tasks T2 and T1 that are
posted from the same thread,
On 2012/02/10 22:48:59, akalin wrote:
> On 2012/02/09 20:50:50, willchan wrote:
> > They don't have to be posted from the same thread, right?
> 
> Well, are there cases where we care about the ordering of tasks posted from
> different threads?  I can't think of one, and talking about it would mandate
> implementations to essentially have a shared queue, which I don't think is
> really necessary.

I need to reflect on this further. What happens in the case where you have
thread 1 where the SequenecedTaskRunner is actually running (in the single
threaded task runner case). You have threads 2 and 3. Thread 2 posts task A to
the SequencedTaskRunner (thread 1), then posts task B to thread 3, which posts
task C to the SequencedTaskRunner.

We need to guarantee that task A will execute on SequencedTaskRunner before task
C. This is important.

[akalin, 2012-02-14 10:47:42]

>
> I need to reflect on this further. What happens in the case where you
> have thread 1 where the SequenecedTaskRunner is actually running (in the
> single threaded task runner case). You have threads 2 and 3. Thread 2
> posts task A to the SequencedTaskRunner (thread 1), then posts task B to
> thread 3, which posts task C to the SequencedTaskRunner.
>
> We need to guarantee that task A will execute on SequencedTaskRunner
> before task C. This is important.
>

Okay, after thinking about it, I think you're right.  Allowing C to run
before A is way too subtle behavior.  I'll change the wording.

[willchan no longer on Chromium, 2012-02-14 19:09:01]

On 2012/02/14 10:47:42, akalin wrote:
> >
> > I need to reflect on this further. What happens in the case where you
> > have thread 1 where the SequenecedTaskRunner is actually running (in the
> > single threaded task runner case). You have threads 2 and 3. Thread 2
> > posts task A to the SequencedTaskRunner (thread 1), then posts task B to
> > thread 3, which posts task C to the SequencedTaskRunner.
> >
> > We need to guarantee that task A will execute on SequencedTaskRunner
> > before task C. This is important.
> >
> 
> Okay, after thinking about it, I think you're right.  Allowing C to run
> before A is way too subtle behavior.  I'll change the wording.

For things like this, is it possible to write templated tests? Then anytime
someone writes a new implementation, he/she can instantiate a new set of tests
to verify it meets the basic guarantees?

I think that'd be nice, but wouldn't want to slow down this changelist for it.
It'd be great to start using these new interfaces ASAP.

[akalin, 2012-02-14 19:41:15]

> Okay, after thinking about it, I think you're right.  Allowing C to run
> before A is way too subtle behavior.  I'll change the wording.

Okay, uploaded a new patch with wording reflecting your change.

Also replaced the verb 'execute' with 'run'.

PTAL.

[akalin, 2012-02-14 19:41:54]

> For things like this, is it possible to write templated tests? Then anytime
> someone writes a new implementation, he/she can instantiate a new set of tests
> to verify it meets the basic guarantees?
> 
> I think that'd be nice, but wouldn't want to slow down this changelist for it.
> It'd be great to start using these new interfaces ASAP.

I think gtest makes that possible.  Added that to the bug.

[darin (slow to review), 2012-02-14 19:45:17]

I'm happy with the class and method names.  I defer to Will for the final
review.

[willchan no longer on Chromium, 2012-02-14 23:19:20]

Everything else looks great to me. I probably missed something minor in the
comments because the CL is large, but by and large, this is a great change.
Thanks for doing this!

LGTM.

[akalin, 2012-02-14 23:21:56]

On 2012/02/14 23:19:20, willchan wrote:
> Everything else looks great to me. I probably missed something minor in the
> comments because the CL is large, but by and large, this is a great change.
> Thanks for doing this!
> 
> LGTM.

Okay.  If you see anything else, feel free to let me know and I can fix in a
follow up.

After I check this in, I'll send an e-mail to chromium-dev announcing it.

[commit-bot: I haz the power, 2012-02-14 23:25:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/9169037/34006

[commit-bot: I haz the power, 2012-02-14 23:25:57]

Presubmit check for 9169037-34006 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
remoting/base/plugin_message_loop_proxy.cc,remoting/base/plugin_message_loop_proxy.h

Presubmit checks took 4.4s to calculate.

[akalin, 2012-02-14 23:27:11]

+sergeyu for review of remoting code

[garykac, 2012-02-15 00:05:57]

LGTM for remoting code.

[Sergey Ulanov, 2012-02-15 00:39:17]

This is awesome, thanks for working on it! remoting changes LGTM.

[commit-bot: I haz the power, 2012-02-15 01:00:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/9169037/38009