Request for feedback: The Future of TaskRunnerHandles

[Gabriel Charette]

Hello Chromium Devs,

You might have heard of the upcoming base/task_scheduler by now.

One of the few remaining blockers to migrating SequencedWorkerPool tasks to it is the current SequencedTaskRunnerHandle contract to create a sequence from an unsequenced task if required.

Which is depended upon by at least the combination of SafeJsonParserImpl and one of its callers which happens to run from an unsequenced task.

Turns out that specific use case would be happy with a plain TaskRunner and creating a sequence for it is overkill.

As such we think that bending TaskScheduler to support this use case when it's not actually the intended semantics is wrong.

This brought us to rethink the intended semantics of TaskRunnerHandles and we would like your opinion on the following doc: The Future of TaskRunnerHandles [1].

Thanks!

TaskScheduler team.

[1] Opened to a...@chromium.org and everyone else who requests access (not "public" only to prevent unintentionally anonymous comments..).

[Brett Wilson]

[I'm finding the doc sidebar comments to be difficult to track for this very high-level discussion, so I'm popping up into the email thread.]

Summary

If I were to distill this proposal, it would be: "Currently we have SequencedTaskRunnerHandle which offers strong guarantees. Migrating this is nontrivial and currently only one caller currently takes advantage of those guarantees. If we weaken the guarantees things will be simpler to implement and we'll fix the one place that uses it."

Currently, the notion of the "current context" as defined by SequencedTaskRunnerHandle is

 1. It's the current thread if the current thread has a normal message loop

 2. It's the current sequence if the code is running on a sequenced worker pool with a sequence token.

 3. It's the current task if the code is running on a pool with no sequence token.

The proposal is effectively to change:

 3. It's the current pool if the code is running on a pool with no sequence token.

In practice this means that things posted to the "current context" when running without a sequence token previously would be guaranteed to run after the current task, but now can run in parallel.

An alternative proposed in the comments by Gab was:

3. It's the current pool but all tasks are guaranteed to run after the current task completes if it's running on a pool with no sequence token.

[Please correct my understanding if any of the above is wrong, since it will mean everything below is also wrong.]

Theoretical ruminations

Defining a "current context" to mean an entire worker pool that can also be overlapped with the current task is so broad as to be not useful. In this case, it is better to say there's no current context at all and Get() doesn't exist. If some code needs to post tasks that can be called in such a context, perhaps it should explicitly post them on some thread or worker pool according to its requirements.

The alternative of deferring posted tasks until after the current task completes, but potentially running the in parallel to each other seems more plausible than having Get return the current pool with no restrictions. In the context of an API that also includes GetSequenced() and GetSingleThreaded(), it would stand to reason that the thing returned by Get() would not necessarily be sequenced or single threaded. If the tasks have requirements between them, the calling code can ensure that they're posted with some kind of sequence.

But independent of this, I have a growing concern that out task runner stuff is already too complicated to be understood by humans, and TaskScheduler is making it worse (not to criticize the team, it seems unavoidable in the short term). I also don't know how in practice using our current primitives one would actually implement "I want to post a sequence of 3 tasks that run after my function on whatever the current context is" and maybe implementing that is actually too complicated. I wonder how much we can actually reason about such general code and write it reliably.

The point

Can we just delete SequencedTaskRunnerHandle? Although this makes it possible to write some very general code that can be called in any context, it also makes it difficult to reason about. If your code expects to be called from various threads and pools in different contexts and needs to post tasks, it should probably do so to a specific place with the requirements it needs. Storing thread-local state of the context of the current sequence seems undesirably global-variable-like to me and makes things difficult to reason about. If the tasks that it posts need to be sequenced with respect to the calling code, the SequencedTaskRunner could be passed in. In this case, everything is very explicit about what's run where and what the ordering requirements are.

If we can delete this, it seems like we don't actually need the proposed change at all. For those people more familiar with how we hope people will use the scheduler to work, does this seem possible?

Brett

[Gabriel Charette]

On Fri, Jul 8, 2016 at 2:27 PM Brett Wilson <bre...@chromium.org> wrote:

[I'm finding the doc sidebar comments to be difficult to track for this very high-level discussion, so I'm popping up into the email thread.]

Thanks, was indeed getting hard to discuss in the sidebar.

 

Summary

If I were to distill this proposal, it would be: "Currently we have SequencedTaskRunnerHandle which offers strong guarantees. Migrating this is nontrivial and currently only one caller currently takes advantage of those guarantees. If we weaken the guarantees things will be simpler to implement and we'll fix the one place that uses it."

Currently, the notion of the "current context" as defined by SequencedTaskRunnerHandle is

 1. It's the current thread if the current thread has a normal message loop

 2. It's the current sequence if the code is running on a sequenced worker pool with a sequence token.

 3. It's the current task if the code is running on a pool with no sequence token.

s/it's the current task/it's a new sequence with the current task at its head/

The proposal is effectively to change:

 3. It's the current pool if the code is running on a pool with no sequence token.

In practice this means that things posted to the "current context" when running without a sequence token previously would be guaranteed to run after the current task, but now can run in parallel.

Yes, but only for callers who explicitly switch to using TaskRunnerHandle::GetAny(), others will merely lose (3.) altogether (but only one caller depends on that FWICT).

An alternative proposed in the comments by Gab was:

3. It's the current pool but all tasks are guaranteed to run after the current task completes if it's running on a pool with no sequence token.

[Please correct my understanding if any of the above is wrong, since it will mean everything below is also wrong.]

Theoretical ruminations

Defining a "current context" to mean an entire worker pool that can also be overlapped with the current task is so broad as to be not useful. In this case, it is better to say there's no current context at all and Get() doesn't exist. If some code needs to post tasks that can be called in such a context, perhaps it should explicitly post them on some thread or worker pool according to its requirements.

The point was more that the "current context" *could* be an entire pool but it could also be a specific thread or sequence depending on the caller. Components like SafeJsonParserImpl merely want to send the callback back to the caller without caring about who he is.

Also note that it's more precise than just the "entire worker pool". In the scheduler world, every task is bound to TaskTraits and that's really what the context is, you need TaskTraits to post to the scheduler's pool (for instance: a generic component's response would inherit the original requester's priority).

The alternative of deferring posted tasks until after the current task completes, but potentially running the in parallel to each other seems more plausible than having Get return the current pool with no restrictions. In the context of an API that also includes GetSequenced() and GetSingleThreaded(), it would stand to reason that the thing returned by Get() would not necessarily be sequenced or single threaded. If the tasks have requirements between them, the calling code can ensure that they're posted with some kind of sequence.

But independent of this, I have a growing concern that out task runner stuff is already too complicated to be understood by humans, and TaskScheduler is making it worse (not to criticize the team, it seems unavoidable in the short term). I also don't know how in practice using our current primitives one would actually implement "I want to post a sequence of 3 tasks that run after my function on whatever the current context is" and maybe implementing that is actually too complicated. I wonder how much we can actually reason about such general code and write it reliably.

I don't see what such a use case would be, typically the "current context" is used for single replies. Wanting to do 3 tasks in sequence sounds more like internal work in which case the component can just grab its own sequence via the scheduler's CreateTaskRunnerWithTraits().

But assuming such a use case exist, this is getting into the world of promises (something we've been ruminating for a little while). The idea is that PostTask() would return some handle which could be used for things like cancellation and/or posting a task to run after the current one (which allows chaining from an otherwise unsequenced task as you mention here).

We're not nearly there yet though so for now such a use case would either have to document it needs to be called from a sequence and use TaskRunnerHandle::GetSequenced() or use TaskRunnerHandle::Get() and implement "sequencing" manually (first task takes second callback as parameter and posts it manually).

The point

Can we just delete SequencedTaskRunnerHandle? Although this makes it possible to write some very general code that can be called in any context, it also makes it difficult to reason about. If your code expects to be called from various threads and pools in different contexts and needs to post tasks, it should probably do so to a specific place with the requirements it needs. Storing thread-local state of the context of the current sequence seems undesirably global-variable-like to me and makes things difficult to reason about. If the tasks that it posts need to be sequenced with respect to the calling code, the SequencedTaskRunner could be passed in. In this case, everything is very explicit about what's run where and what the ordering requirements are.

This means that many APIs that merely take a Callback as input today would need to take both a Callback and a (.*)TaskRunner (unless we introduce a Bind like construct that also binds the TaskRunner to be used? -- that might make sense, a Callback is a bit vague, what the caller actually wants is to be called back in its context -- today it assumes the component will "do the right thing" by using task runner handle if replying asynchronously but binding the TaskRunner in the Callback would be a stronger way to achieve this; caveat: the TaskRunner is often not available at the Bind callsite either so Thread/SequencedTaskRunnerHandle are still required barring some major plumbing but it could get rid of the need for the more generic one proposed).

If we can delete this, it seems like we don't actually need the proposed change at all. For those people more familiar with how we hope people will use the scheduler to work, does this seem possible?

In the scheduler world we want to make it possible for most consumers to grab ExecutionMode::SEQUENCED TaskRunners instead of ExecutionMode::SINGLE_THREADED TaskRunners as most things in Chrome merely need thread-safety not thread-affinity and ExecutionMode::SINGLE_THREADED strongly hinders our ability to do scheduling at all. As such if we believe ThreadTaskRunnerHandle is useful (it's definitely used *all* over the place right now -- 1789 callsites), we need at least SequencedTaskRunnerHandle to migrate many of them.

This proposal came to be specifically because I think we need SequencedTaskRunnerHandle but I don't feel creating implicit sequences off of the current task is correct (per the sequence creation being overhead which the caller doesn't need in the scenarios 1.1-1.3 I'm proposing TaskRunnerHandle::GetAny() as appropriate).

Thanks for your deep dive on this,

Gab

Brett

[Brett Wilson]

I realize in another doc (the POSIX watcher thing) I was telling them it was OK to use SequencedTaskRunnerHandle so I concede this is an important use case.

In that case I think we should not have the automatic promotion of the current task to a sequence (it should assert), and not have Get/GetAny functions in the new API either. If somebody wants to call an API that issues a callback, they will need to set up a context where such callbacks make sense (either a regular thread or a sequence on a pool). If the API being called needs to support more general uses, it should accept a task runner. It seems very rare to have an API where it wants to issue a callback and the caller wants to be called on any thread in a pool with no sequencing.

Brett

[Michael Nordman]

^^^ sounds like a reasonable proposal, I think GetAny semantics would make it very difficult to reason about control flow when reading code. It might be easier to reason about if we make an explicit getter for the current unsequenced task running context?

class TaskRunnerHandle {

static bool HasUnsequencedScope(); // returns false if currently in a sequenced or single-threaded scope

static scoped_refptr<SequencedTaskRunner> GetUnsequenced(); // can only be called when HasUnsequencedScope

static bool HasSequencedScope();

static scoped_refptr<SequencedTaskRunner> GetSequenced();

static bool HasSingleThreadScope();

static scoped_refptr<SingleThreadTaskRunner> GetSingleThreaded();

}

PostTaskAndReply and PostTaskAndReplyWithResult take care of the most common cross-thread callback scenarios. How would those function behave if invoked from an unsequenced context?

ps: from the other michaeln@ this time, sorry for the spam

 

Brett

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

[John Abd-El-Malek]

Another +1 to getting rid of the automatic creation of a sequenced task runner. I'm sure it has some use cases that make it nice, but we should balance that with the extra complexity it adds in understanding how tasks are posted.

[Gabriel Charette]

@ "Keep SequencedTaskRunnerHandle but don't create implicit sequences (assert)" : the one thing I don't like about this is that it feels like a design smell to me that a component would need to be refactored to support a seemingly innocent caller (e.g. SafeJsonParserImpl needs to be changed to take a TaskRunner parameter to support being called from LoadSTHsFromDisk() because that happens to run as a one-off task in the BlockingPool -- it would be totally fine to call the "result callback" as another one-off task in the blocking pool).

This proposal was aiming to get rid of the need for that smelly refactor (instead SafeJsonParserImpl declares it doesn't care where the callback goes by using TaskRunnerHandle::GetAny() and any caller can call in without a problem).

We could decide that we're okay living with this potential design smell (and requiring the occasional refactor) at the advantage of not dealing with potential misuses of TaskRunnerHandle::GetAny(), just want to make sure we make this decision in full awareness.

I guess that would mean that ExecutionMode::PARALLEL would only make sense for leaf components (i.e. that don't use any components that may need the current context) -- and of course that are okay with PARALLEL for themselves (not suggesting it for all leaf components..!).

@ "PostTaskAndReply and PostTaskAndReplyWithResult take care of the most common cross-thread callback scenarios. How would those function behave if invoked from an unsequenced context?" : PostTaskAndReply, despite being an API on the generic TaskRunner, is actually also designed around SingleThreadTaskRunner -- it uses ThreadTaskRunnerHandle :-(. It will at least need to use SequencedTaskRunnnerHandle in the scheduler world but could use TaskRunnerHandle::GetAny() if we were to adopt it (actually not sure why the relay doesn't just take the TaskRunner* directly instead of using global state..?). Either way... something to figure out in a follow-up proposal.

[Gabriel Charette]

Actually, this makes me think : essentially the use cases I think TaskRunnerHandle::GetAny() applies to are PostTaskAndReply use cases in which the component keeps the "origin" TaskRunner for an eventual reply (or has the PostTaskAndReplyRelay do so implicitly).

In those cases I think it's okay to allow the "origin" TaskRunner to merely be backed by a pool (i.e. "parallel") so long as the reply is not executed while the original poster is still running.

So perhaps what we need (as was discussed on doc) is something like TaskRunnerHandle::GetDeferred() instead of GetAny() which would return a plain TaskRunner but would guarantee tasks posted to it don't run until the task it was obtained from has completed?

Then we get the benefits of this being usable from components that merely care about replying (i.e. that don't need to force the caller to be sequenced), without the headaches of an occasional reply unintentionally running in parallel with its request.

Does that make sense? If so I can update the doc to reflect that before everyone takes another look.

[Gabriel Charette]

ping: any thoughts on last 2 replies? Thanks!

[Michael Nordman]

On Tue, Jul 12, 2016 at 3:08 PM, Gabriel Charette <g...@chromium.org> wrote:

ping: any thoughts on last 2 replies? Thanks!

Le lun. 11 juil. 2016 21:35, Gabriel Charette <g...@chromium.org> a écrit :

Actually, this makes me think : essentially the use cases I think TaskRunnerHandle::GetAny() applies to are PostTaskAndReply use cases in which the component keeps the "origin" TaskRunner for an eventual reply (or has the PostTaskAndReplyRelay do so implicitly).

In those cases I think it's okay to allow the "origin" TaskRunner to merely be backed by a pool (i.e. "parallel") so long as the reply is not executed while the original poster is still running.

So perhaps what we need (as was discussed on doc) is something like TaskRunnerHandle::GetDeferred() instead of GetAny() which would return a plain TaskRunner but would guarantee tasks posted to it don't run until the task it was obtained from has completed?

Then we get the benefits of this being usable from components that merely care about replying (i.e. that don't need to force the caller to be sequenced), without the headaches of an occasional reply unintentionally running in parallel with its request.

I don't see mention of GetDeferred() in the doc so don't know quite what the details are?

+1 this is mostly about satisfying PostTaskAndReply() use cases.

The existing PostTaskAndReply helpers ensure the "reply" does not run concurrent with the the "task" or the caller.

When the caller and the callee are loosely coupled, I think the caller needs to determine how a cross thread callback it provides should be run. So having the callee use GetAny() or GetSomething() to compute how the reply should be handled seems off.

 I think we should bake this logic into helpers so most callers / callees don't have to roll their own.

There might be one PostTaskAndReply helper that we're missing. We have these two..

  PostTaskAndReply()  --> basic completion callback

  PostTaskAndReplyWithResult()  --> plumbs the function return of the task to the reply

... here's the missing one

  PostTaskAndReplyWithDelayedResult() --> let's the  task delay producing a result

That last one would provide a resultCallback to the original task which would 'relay' the result back. Like Gab mentioned earlier, a base::Bind closure that bound the taskrunner up too. By default, the taskrunner the reply would be delivered to is computed on the initiating thread by...

  if (HasUnsequencedScope) return GetUnsequenced();

  if (HasSequencedScope) return GetSequenced();

  if (HasSingleThreadedScope) return GetSingleThreaded();

I think that's probably a good default, however, we could have variants of these helpers that take as input a targetRunner and a replyRunner so if code running in a SingleThreadedScope really didn't care to process the reply back on its thread, it could provide a replyRunner that posts to a an unsequenced pool (or whatnot).

The helpers would guarantee the (caller and reply) and the (task and reply)  are not run concurrent with one another.

[Brett Wilson]

On Mon, Jul 11, 2016 at 6:35 PM, Gabriel Charette <g...@chromium.org> wrote:

Actually, this makes me think : essentially the use cases I think TaskRunnerHandle::GetAny() applies to are PostTaskAndReply use cases in which the component keeps the "origin" TaskRunner for an eventual reply (or has the PostTaskAndReplyRelay do so implicitly).

In those cases I think it's okay to allow the "origin" TaskRunner to merely be backed by a pool (i.e. "parallel") so long as the reply is not executed while the original poster is still running.

So perhaps what we need (as was discussed on doc) is something like TaskRunnerHandle::GetDeferred() instead of GetAny() which would return a plain TaskRunner but would guarantee tasks posted to it don't run until the task it was obtained from has completed?

Then we get the benefits of this being usable from components that merely care about replying (i.e. that don't need to force the caller to be sequenced), without the headaches of an occasional reply unintentionally running in parallel with its request.

Does that make sense? If so I can update the doc to reflect that before everyone takes another look.

First, I agree that PostTaskAndReply should never run the reply in parallel. Also, we agree what PostTaskAndReply should do on single threads or worker pool tasks with sequences and nothing is being changed.

But it's not clear to me even what PostTaskAndReply means in the context of requesting it from a task with no sequence and no well-known thread. Replying to a worker pool and saying "just run on any thread" isn't a "reply" at all and actually this has no meaning in Chrome's current architecture. If you don't care what thread the callback is run on, it seems like you really want to post a task that calls your function and then does something on that thread.

With the task scheduler the calling pool could have a different priority or something so theoretically this could have meaning. But I still fail to see how this could be used in practice and doing this actually seems likely to be a bug. It seems actually beneficial to not support PostTaskAndReply with the reply being an unsequenced worker pool. If you want a reply, you need to have a reasonably specific place that the reply goes to.

Brett

[Greg Thompson]

On Wed, Jul 13, 2016, 7:07 AM Brett Wilson <bre...@chromium.org> wrote:

On Mon, Jul 11, 2016 at 6:35 PM, Gabriel Charette <g...@chromium.org> wrote:

Actually, this makes me think : essentially the use cases I think TaskRunnerHandle::GetAny() applies to are PostTaskAndReply use cases in which the component keeps the "origin" TaskRunner for an eventual reply (or has the PostTaskAndReplyRelay do so implicitly).

In those cases I think it's okay to allow the "origin" TaskRunner to merely be backed by a pool (i.e. "parallel") so long as the reply is not executed while the original poster is still running.

So perhaps what we need (as was discussed on doc) is something like TaskRunnerHandle::GetDeferred() instead of GetAny() which would return a plain TaskRunner but would guarantee tasks posted to it don't run until the task it was obtained from has completed?

Then we get the benefits of this being usable from components that merely care about replying (i.e. that don't need to force the caller to be sequenced), without the headaches of an occasional reply unintentionally running in parallel with its request.

Does that make sense? If so I can update the doc to reflect that before everyone takes another look.

First, I agree that PostTaskAndReply should never run the reply in parallel. Also, we agree what PostTaskAndReply should do on single threads or worker pool tasks with sequences and nothing is being changed.

But it's not clear to me even what PostTaskAndReply means in the context of requesting it from a task with no sequence and no well-known thread.

+1 to this point. I think it's an accident that this has ever worked. Is it widespread?

Replying to a worker pool and saying "just run on any thread" isn't a "reply" at all and actually this has no meaning in Chrome's current architecture. If you don't care what thread the callback is run on, it seems like you really want to post a task that calls your function and then does something on that thread.

With the task scheduler the calling pool could have a different priority or something so theoretically this could have meaning. But I still fail to see how this could be used in practice and doing this actually seems likely to be a bug. It seems actually beneficial to not support PostTaskAndReply with the reply being an unsequenced worker pool. If you want a reply, you need to have a reasonably specific place that the reply goes to.

Brett

[Gabriel Charette]

On Wed, Jul 13, 2016 at 12:12 PM Greg Thompson <g...@chromium.org> wrote:

On Wed, Jul 13, 2016, 7:07 AM Brett Wilson <bre...@chromium.org> wrote:

On Mon, Jul 11, 2016 at 6:35 PM, Gabriel Charette <g...@chromium.org> wrote:

Actually, this makes me think : essentially the use cases I think TaskRunnerHandle::GetAny() applies to are PostTaskAndReply use cases in which the component keeps the "origin" TaskRunner for an eventual reply (or has the PostTaskAndReplyRelay do so implicitly).

In those cases I think it's okay to allow the "origin" TaskRunner to merely be backed by a pool (i.e. "parallel") so long as the reply is not executed while the original poster is still running.

So perhaps what we need (as was discussed on doc) is something like TaskRunnerHandle::GetDeferred() instead of GetAny() which would return a plain TaskRunner but would guarantee tasks posted to it don't run until the task it was obtained from has completed?

Then we get the benefits of this being usable from components that merely care about replying (i.e. that don't need to force the caller to be sequenced), without the headaches of an occasional reply unintentionally running in parallel with its request.

Does that make sense? If so I can update the doc to reflect that before everyone takes another look.

First, I agree that PostTaskAndReply should never run the reply in parallel. Also, we agree what PostTaskAndReply should do on single threads or worker pool tasks with sequences and nothing is being changed.

But it's not clear to me even what PostTaskAndReply means in the context of requesting it from a task with no sequence and no well-known thread.

+1 to this point. I think it's an accident that this has ever worked. Is it widespread?

Note: PostTaskAndReply itself doesn't actually work from parallel tasks today (it is meant to from the API though -- i.e. the plain TaskRunner API exposes it -- but the impl (PostTaskAndReplyRelay) uses ThreadTaskRunnerHandle which is specific to SingleThreadTaskRunners...).

Replying to a worker pool and saying "just run on any thread" isn't a "reply" at all and actually this has no meaning in Chrome's current architecture. If you don't care what thread the callback is run on, it seems like you really want to post a task that calls your function and then does something on that thread.

With the task scheduler the calling pool could have a different priority or something so theoretically this could have meaning. But I still fail to see how this could be used in practice and doing this actually seems likely to be a bug. It seems actually beneficial to not support PostTaskAndReply with the reply being an unsequenced worker pool. If you want a reply, you need to have a reasonably specific place that the reply goes to.

I think post-task-and-reply semantics from unsequenced tasks actually has valid use cases. e.g. a one-off task (LoadSTHsFromDisk) wants to post another one-off task to do some parsing (through SafeJsonParserImpl) and then do something with the response (once again as a one-off task).

This is merely a chain of callbacks (which I guess is "sequenced" in a way but not in the traditional sense) and I don't see why it shouldn't be supported (at least the current API intends for it to be by (1) declaring PostTaskAndReply on the plain TaskRunner and (2) PostTaskAndReplyWithResult taking plain TaskRunners as arguments).

Re. Michael's reply: not sure we want another PostTaskAndReply variant. But I could see us introduce something like base::RelayCallback created by a new base::BindRelay() call which also binds a TaskRunner. That would mean that APIs requiring GetAny() for replies could instead take a base::RelayCallback that incorporates the TaskRunner already.

The downside to this IMO is that callers have to do something like:

  task_runner->PostTask(..., base::BindRelay(task_runner, ...));

and every callsite thus has to specify the same |task_runner| twice in the Post statement (subject to forgetting to change one of the two in a future refactoring). Whereas doing TaskRunnerHandle::GetAny() from the component side always guarantees it's replying to the TaskRunner the call originated from (doubt any existing callsites need the flexibility of the reply coming back on a different TaskRunner).

[Gabriel Charette]

On the flip-side the advantage of the RelayCallback is that it makes sure that the potentially unsequenced TaskRunner backing the relay is only used for the callback (whereas TaskRunnerHandle::GetAny() lets the component also use it for other unrelated things which could get hairy if misused).

[Brett Wilson]

On Wed, Jul 13, 2016 at 10:06 AM, Gabriel Charette <g...@chromium.org> wrote:

On Wed, Jul 13, 2016 at 12:12 PM Greg Thompson <g...@chromium.org> wrote:

On Wed, Jul 13, 2016, 7:07 AM Brett Wilson <bre...@chromium.org> wrote:

On Mon, Jul 11, 2016 at 6:35 PM, Gabriel Charette <g...@chromium.org> wrote:

Actually, this makes me think : essentially the use cases I think TaskRunnerHandle::GetAny() applies to are PostTaskAndReply use cases in which the component keeps the "origin" TaskRunner for an eventual reply (or has the PostTaskAndReplyRelay do so implicitly).

In those cases I think it's okay to allow the "origin" TaskRunner to merely be backed by a pool (i.e. "parallel") so long as the reply is not executed while the original poster is still running.

So perhaps what we need (as was discussed on doc) is something like TaskRunnerHandle::GetDeferred() instead of GetAny() which would return a plain TaskRunner but would guarantee tasks posted to it don't run until the task it was obtained from has completed?

Then we get the benefits of this being usable from components that merely care about replying (i.e. that don't need to force the caller to be sequenced), without the headaches of an occasional reply unintentionally running in parallel with its request.

Does that make sense? If so I can update the doc to reflect that before everyone takes another look.

First, I agree that PostTaskAndReply should never run the reply in parallel. Also, we agree what PostTaskAndReply should do on single threads or worker pool tasks with sequences and nothing is being changed.

But it's not clear to me even what PostTaskAndReply means in the context of requesting it from a task with no sequence and no well-known thread.

+1 to this point. I think it's an accident that this has ever worked. Is it widespread?

Note: PostTaskAndReply itself doesn't actually work from parallel tasks today (it is meant to from the API though -- i.e. the plain TaskRunner API exposes it -- but the impl (PostTaskAndReplyRelay) uses ThreadTaskRunnerHandle which is specific to SingleThreadTaskRunners...).

Replying to a worker pool and saying "just run on any thread" isn't a "reply" at all and actually this has no meaning in Chrome's current architecture. If you don't care what thread the callback is run on, it seems like you really want to post a task that calls your function and then does something on that thread.

With the task scheduler the calling pool could have a different priority or something so theoretically this could have meaning. But I still fail to see how this could be used in practice and doing this actually seems likely to be a bug. It seems actually beneficial to not support PostTaskAndReply with the reply being an unsequenced worker pool. If you want a reply, you need to have a reasonably specific place that the reply goes to.

I think post-task-and-reply semantics from unsequenced tasks actually has valid use cases. e.g. a one-off task (LoadSTHsFromDisk) wants to post another one-off task to do some parsing (through SafeJsonParserImpl) and then do something with the response (once again as a one-off task).

This is merely a chain of callbacks (which I guess is "sequenced" in a way but not in the traditional sense) and I don't see why it shouldn't be supported (at least the current API intends for it to be by (1) declaring PostTaskAndReply on the plain TaskRunner and (2) PostTaskAndReplyWithResult taking plain TaskRunners as arguments).

I still don't think this should be supported.  In your example, it would be more efficient to post a task with no reply of a function with two lines in it: 1. calls the thing you want to do, and 2. does the parsing. In this type of sequenced operation, it's not likely each step will have different flags on prioritization.

So the only case where this could even be used in a useful way is posting from a pool to a well-known thread, and then poarsing the result back to the pool. But I would argue if you're doing work like that, you're doing sequenced work and want a sequence token. To support reasonable semantics otherwise, you're having to invent a new thing that special-case delays work until after the first task is done.

This special-casing seems like unnecessary complexity and I don't see a problem with requiring one make a sequence token if you want to pick up on a pool with a reply somewhere.

I would also argue that when this comes up, it's more likely a bug than that the caller actually wanted the behavior you describe. So I think it's reasonable to ask people actually wanting to do this to do some extra work to prevent those bugs.

Brett

[Gabriel Charette]

Ok, so conclusion:

Keep ThreadTaskRunnerHandle and SequencedTaskRunnerHandle (forget doc proposal). Ban implicit creation of sequences. Force any caller that uses a component that cares to have a SequencedTaskRunnerHandle available to create a sequence token (in our canonical example, the poster of LoadSTHsFromDisk would have to not post directly to the BlockingPool but rather to a sequence on it)?

That solves my immediate problem and puts the burden on the occasional odd caller which I think is reasonable.

While we're on this topic: do we think PostTaskAndReply should only be supported on SequencedTaskRunners? I'm happy to make this change (it "should" be a simple replacement per the impl actually only working from SingleThreadTaskRunners in practice and existing usage thus always coming from one today in theory -- though some (hopefully not many :-S!) callsites may have a generic TaskRunner which happens to be a more specific one).

And I would of course also add support for PostTaskAndReply to work from sequences in practice (have PostTaskAndReplyRelay use SequencedTaskRunnerHandle instead of ThreadTaskRunnerHandle) -- another prereq for the TaskScheduler.

[Brett Wilson]

On Wed, Jul 13, 2016 at 1:10 PM, Gabriel Charette <g...@chromium.org> wrote:

Ok, so conclusion:

Keep ThreadTaskRunnerHandle and SequencedTaskRunnerHandle (forget doc proposal). Ban implicit creation of sequences. Force any caller that uses a component that cares to have a SequencedTaskRunnerHandle available to create a sequence token (in our canonical example, the poster of LoadSTHsFromDisk would have to not post directly to the BlockingPool but rather to a sequence on it)?

That solves my immediate problem and puts the burden on the occasional odd caller which I think is reasonable.

While we're on this topic: do we think PostTaskAndReply should only be supported on SequencedTaskRunners? I'm happy to make this change (it "should" be a simple replacement per the impl actually only working from SingleThreadTaskRunners in practice and existing usage thus always coming from one today in theory -- though some (hopefully not many :-S!) callsites may have a generic TaskRunner which happens to be a more specific one).

And I would of course also add support for PostTaskAndReply to work from sequences in practice (have PostTaskAndReplyRelay use SequencedTaskRunnerHandle instead of ThreadTaskRunnerHandle) -- another prereq for the TaskScheduler.

This all sounds right to me. We can always relax the rules later if we find we're too limited.

Brett

[Gabriel Charette]

Update:

Implicit sequence creation from parallel tasks is banned (DCHECKs) as of r408992 (incidentally only user was removed just-in-time as part of an unrelated issue :-)).

PostTaskAndReply supports being called from sequenced contexts (used to be only single-threaded contexts) as of r408794.

We don't need to move PostTaskAndReply to SequencedTaskRunner. I got confused for a sec when proposing that: it's okay to post to a parallel TaskRunner so long as the reply comes back on a sequence (hence the requirement is on the caller not the callee. TaskRunner::PostTaskAndReply's documentation was updated to make that clear.

Cheers,

Gab