Renaming TaskRunner::RunsTasksOnCurrentThread()

80 views
Skip to first unread message

François Doray

unread,
Sep 20, 2016, 4:25:20 PM9/20/16
to Chromium-dev, g...@chromium.org, rob...@chromium.org
tl;dr: The name and description of TaskRunner::RunsTasksOnCurrentThread() doesn't match all its impls. Should we rename it to TaskRunner::IsSequencedWithCurrentTask()?

This is the current declaration of TaskRunner::RunsTasksOnCurrentThread in task_runner.h:
  // Returns true if the current thread is a thread on which a task
  // may be run, and false if no task will be run on the current
  // thread.
  //
  // It is valid for an implementation to always return true, or in
  // general to use 'true' as a default value.
  virtual bool RunsTasksOnCurrentThread() const = 0;

However, if you call RunsTasksOnCurrentThread() on a SequencedTaskRunner returned by SequencedWorkerPool, it won't tell you if "the current thread is a thread on which a task may be run"! Rather than that, it will tell you if the current task is part of the sequence associated with the SequencedTaskRunner.

I believe that we should change TaskRunner::RunsTasksOnCurrentThread to:
  // Returns true iff a task different than the current task  // that is posted to this TaskRunner can't run at the same  // time as the current task.  //  // In particular, returns true if this is a  // SequencedTaskRunner to which the current task was posted  // and always returns false if this is a TaskRunner that  // can run its tasks in parallel.  virtual bool IsSequencedWithCurrentTask() const = 0;

Pros:
  • The new name and description match the existing behavior of all TaskRunners returned by MessageLoop and SequencedWorkerPool.
  • Tasks posted to base/task_scheduler may run on any base/task_scheduler thread. It wouldn't be very helpful to have a method to know whether a task runs in base/task_scheduler. It's more helpful to have a method to know whether the task is part of a given sequence.
  • IsSequencedWithCurrentTask() make it clear that it can be used to DCHECK() that the current task runs as part of a sequence.
  • IsSequencedWithCurrentTask() make it clear that it can be used to do this: (the previous description said that it is valid for RunsTasksOnCurrentThread to always return true):
void DoSomething() {
  if (task_runner->IsSequencedWithCurrentTask())
    // This doesn't run at the same time as a task posted to |task_runner|.
  task_runner->PostTask(FROM_HERE, Bind(&DoSomething));
}

Cons:
  • The new name and description change the existing semantic. For example, IsSequencedWithCurrentTask() will always return false for a TaskRunner returned by base::WorkerPool.

What do you think?

Gabriel Charette

unread,
Sep 20, 2016, 5:36:45 PM9/20/16
to François Doray, Chromium-dev, g...@chromium.org, rob...@chromium.org
Sounds great to me!

Such calls only make sense for code that wants to verify/document it's running on the WorkerPool at all, right? Those could use the static WorkerPool::RunsTasksOnCurrentThread() instead?

Ryan Sleevi

unread,
Sep 21, 2016, 6:34:26 PM9/21/16
to Francois, Chromium-dev, Gabriel Charette, rob...@chromium.org
On Tue, Sep 20, 2016 at 1:24 PM, François Doray <fdo...@chromium.org> wrote:
However, if you call RunsTasksOnCurrentThread() on a SequencedTaskRunner returned by SequencedWorkerPool, it won't tell you if "the current thread is a thread on which a task may be run"! Rather than that, it will tell you if the current task is part of the sequence associated with the SequencedTaskRunner.

But it does, in your example, meet the API contract. The current thread is a thread in which the task may be run. It doesn't say will, just may.
 
I believe that we should change TaskRunner::RunsTasksOnCurrentThread to:
  // Returns true iff a task different than the current task  // that is posted to this TaskRunner can't run at the same  // time as the current task.

For what it's worth, this reads very difficult for me. I'm not sure if others find it as difficult to parse as I do.

In every attempt I've made to reword this, it seems to me that your attempt is to express a different semantic than what's currently being expressed.

For example,

// Returns true if tasks posted to this TaskRunner will not be executed until the current task completes.

However, as that's not the semantics being expressed by RunsTasksOnCurrentThread, I don't believe you're simply expressing an API change.
 
Pros:
  • The new name and description match the existing behavior of all TaskRunners returned by MessageLoop and SequencedWorkerPool.
As explained above, I don't believe it does.
 
  • Tasks posted to base/task_scheduler may run on any base/task_scheduler thread. It wouldn't be very helpful to have a method to know whether a task runs in base/task_scheduler. It's more helpful to have a method to know whether the task is part of a given sequence.
I disagree. There are a number of cases in //net where we've concretely needed to know whether the target (Sequenced)TaskRunner is executing on the same thread, and if so, can avoid additional thread-hopping setup and safely execute code directly.

I realize //net has been largely dismissed by those working on base/task_scheduler as a special snowflake, but it feels this use case is important, especially when considering the places in which there are thread-locals or other assignments.
 
  • IsSequencedWithCurrentTask() make it clear that it can be used to DCHECK() that the current task runs as part of a sequence.
  • IsSequencedWithCurrentTask() make it clear that it can be used to do this: (the previous description said that it is valid for RunsTasksOnCurrentThread to always return true):
void DoSomething() {
  if (task_runner->IsSequencedWithCurrentTask())
    // This doesn't run at the same time as a task posted to |task_runner|.
  task_runner->PostTask(FROM_HERE, Bind(&DoSomething));
}

Cons:
  • The new name and description change the existing semantic. For example, IsSequencedWithCurrentTask() will always return false for a TaskRunner returned by base::WorkerPool.

What do you think?


I'm not supportive of this change, but I fully admit that I may have misunderstood the motivations. I think this conveys a different semantic and the subtleties are even less obvious. 

François Doray

unread,
Sep 22, 2016, 9:26:13 AM9/22/16
to rsl...@chromium.org, Chromium-dev, Gabriel Charette, rob...@chromium.org
On Wed, Sep 21, 2016 at 6:33 PM Ryan Sleevi <rsl...@chromium.org> wrote:
On Tue, Sep 20, 2016 at 1:24 PM, François Doray <fdo...@chromium.org> wrote:
However, if you call RunsTasksOnCurrentThread() on a SequencedTaskRunner returned by SequencedWorkerPool, it won't tell you if "the current thread is a thread on which a task may be run"! Rather than that, it will tell you if the current task is part of the sequence associated with the SequencedTaskRunner.

But it does, in your example, meet the API contract. The current thread is a thread in which the task may be run. It doesn't say will, just may.

No it doesn't.
  • Contract: "Returns true if the current thread is a thread on which a task may be run, and false if no task will be run on the current thread."
  • What SequencedWorkerPool::PoolSequencedTaskRunner::RunsTasksOnCurrentThread() really does: Returns true if the current task is part of the sequence associated with the SequencedTaskRunner. Returns false if the current task isn't part of the sequence, even if "the current thread is a thread on which a task may be run".
 
I believe that we should change TaskRunner::RunsTasksOnCurrentThread to:
  // Returns true iff a task different than the current task  // that is posted to this TaskRunner can't run at the same  // time as the current task.

For what it's worth, this reads very difficult for me. I'm not sure if others find it as difficult to parse as I do. 

In every attempt I've made to reword this, it seems to me that your attempt is to express a different semantic than what's currently being expressed.

For example,

// Returns true if tasks posted to this TaskRunner will not be executed until the current task completes.

However, as that's not the semantics being expressed by RunsTasksOnCurrentThread, I don't believe you're simply expressing an API change.

I try to express something different than what the current description expresses, but that correctly describes what every (Sequenced|SingleThread)TaskRunner *already does*. The new description will however not match what not sequenced and not single-threaded TaskRunner do (the implementation will need to change for those).

New description attempt:

  // Returns true iff any task posted to this TaskRunner (in the past  // or in the future) which isn't the current task can't run at the  // same time as the current task.  //  // In particular:  // - Returns true if this is a SequencedTaskRunner to which the  //   current task was posted.  // - Returns true if this is a SequencedTaskRunner bound to the  //   same sequence as the SequencedTaskRunner to which the current  //   task was posted.  // - Returns true if this is a SingleThreadTaskRunner bound to  //   the current thread.  // - Returns false if this is a TaskRunner that can run its tasks  //   in parallel.  virtual bool IsSequencedWithCurrentTask() const = 0;

  
Pros:
  • The new name and description match the existing behavior of all TaskRunners returned by MessageLoop and SequencedWorkerPool.
As explained above, I don't believe it does.
 
  • Tasks posted to base/task_scheduler may run on any base/task_scheduler thread. It wouldn't be very helpful to have a method to know whether a task runs in base/task_scheduler. It's more helpful to have a method to know whether the task is part of a given sequence.
I disagree. There are a number of cases in //net where we've concretely needed to know whether the target (Sequenced)TaskRunner is executing on the same thread, and if so, can avoid additional thread-hopping setup and safely execute code directly.

I analyzed every call site of RunsTaskOnCurrentThread() in //net. RunsTaskOnCurrentThread() is always called on a (Sequenced|SingleThread)TaskRunner and the behavior of RunsTaskOnCurrentThread() won't change for those. I simply want to have a more accurate name and description for this method.

If you think I forgot a call site that would be negatively impacted by this change, don't hesitate to share a link. We'll make sure that to find a solution that meets the needs of //net.

The TaskRunners are as SingleThreadTaskRunner. The behavior of RunsTasksOnCurrentThread() doesn't change for SingleThreadTaskRunners.

These are explicit SingleThreadTaskRunners. The behavior of RunsTasksOnCurrentThread() doesn't change for SingleThreadTaskRunners.  

These are explicit SequencedTaskRunners. The behavior of RunsTasksOnCurrentThread() doesn't change for SequencedTaskRunners.

network_task_runner_ is always initialized with a SingleThreadTaskRunner. The behavior of RunsTasksOnCurrentThread() doesn't change for SingleThreadTaskRunners.
See:

Sami Kyostila

unread,
Sep 22, 2016, 11:07:50 AM9/22/16
to fdo...@chromium.org, rsl...@chromium.org, Chromium-dev, Gabriel Charette, rob...@chromium.org
I would support improving the naming here, mainly because I find TaskRunner::RunsTasksOnCurrentThread() easy to confuse with SingleThreadTaskRunner::BelongsToCurrentThread().

(ob. bikeshed: "BelongsToCurrentSequence"?)

- Sami

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

Gabriel Charette

unread,
Sep 22, 2016, 11:20:11 AM9/22/16
to Sami Kyostila, fdo...@chromium.org, rsl...@chromium.org, Chromium-dev, Gabriel Charette, rob...@chromium.org
On Thu, Sep 22, 2016 at 11:07 AM Sami Kyostila <skyo...@chromium.org> wrote:
I would support improving the naming here, mainly because I find TaskRunner::RunsTasksOnCurrentThread() easy to confuse with SingleThreadTaskRunner::BelongsToCurrentThread().

(ob. bikeshed: "BelongsToCurrentSequence"?)

I like "BelongsToCurrentSequence".

Colin Blundell

unread,
Sep 22, 2016, 11:37:17 AM9/22/16
to g...@chromium.org, Sami Kyostila, fdo...@chromium.org, rsl...@chromium.org, Chromium-dev, rob...@chromium.org
On Thu, Sep 22, 2016 at 5:19 PM Gabriel Charette <g...@chromium.org> wrote:
On Thu, Sep 22, 2016 at 11:07 AM Sami Kyostila <skyo...@chromium.org> wrote:
I would support improving the naming here, mainly because I find TaskRunner::RunsTasksOnCurrentThread() easy to confuse with SingleThreadTaskRunner::BelongsToCurrentThread().

(ob. bikeshed: "BelongsToCurrentSequence"?)

I like "BelongsToCurrentSequence".
 


I'm not a domain expert here at all, so these are basically layman's questions. Does a TaskRunner "belong" to a sequence? I had thought of it as something that itself sequenced tasks (or didn't, as fdoray@ has pointed out ;).

Would "SequencesCurrentTask()" make sense?

François Doray

unread,
Sep 22, 2016, 3:37:59 PM9/22/16
to Colin Blundell, g...@chromium.org, Sami Kyostila, rsl...@chromium.org, Chromium-dev, rob...@chromium.org
One or many SequencedTaskRunners may post to a given sequence. We need to find a name that expresses "does this TaskRunner posts its tasks to the sequence to which the current task belongs?".

BelongsToCurrentSequence() sounds good. We would keep BelongsToCurrentThread() as an alias for SingleThreadTaskRunners.

Alternatively, we could have PostsToCurrentSequence() and PostsToCurrentThread().

Colin Blundell

unread,
Sep 22, 2016, 4:42:28 PM9/22/16
to François Doray, Colin Blundell, g...@chromium.org, Sami Kyostila, rsl...@chromium.org, Chromium-dev, rob...@chromium.org

How about just changing RunsTasksOnCurrentThread() to RunsTasksInCurrentSequence()?

François Doray

unread,
Sep 23, 2016, 8:16:47 AM9/23/16
to Colin Blundell, g...@chromium.org, Sami Kyostila, rsl...@chromium.org, Chromium-dev, rob...@chromium.org
Renaming RunsTasksOnCurrentThread() -> RunsTasksInCurrentSeqeuence() is my preferred solution so far.

  // Returns true iff tasks posted to this TaskRunner are sequenced  // with this call.  //  // In particular:  // - Returns true if this is a SequencedTaskRunner to which the  //   current task was posted.  // - Returns true if this is a SequencedTaskRunner bound to the  //   same sequence as the SequencedTaskRunner to which the current  //   task was posted.  // - Returns true if this is a SingleThreadTaskRunner bound to  //   the current thread.  // - Returns false if this is a TaskRunner that can run its tasks  //   in parallel.  virtual bool RunsTasksInCurrentSequence() const = 0;

Colin Blundell

unread,
Sep 23, 2016, 8:18:54 AM9/23/16
to François Doray, Colin Blundell, g...@chromium.org, Sami Kyostila, rsl...@chromium.org, Chromium-dev, rob...@chromium.org
This SGTM (but of course it does ;).

Given that (a) there's a comment on task_runner.h saying "TaskRunner does not guarantee the order in which posted tasks are run, whether tasks overlap, or whether they're run on a particular thread." and (b) TaskRunner already has virtual methods with default impls (e.g., OnDestruct()), I think that you could sharpen this by giving TaskRunner a default impl that returns false. It would then be clear that a subclass overriding RunsTasksInCurrentSequence() implies that that subclass is providing *some* kind of sequencing.

Gabriel Charette

unread,
Sep 23, 2016, 10:05:13 AM9/23/16
to blun...@chromium.org, François Doray, g...@chromium.org, Sami Kyostila, rsl...@chromium.org, Chromium-dev, rob...@chromium.org
On Fri, Sep 23, 2016 at 8:17 AM Colin Blundell <blun...@chromium.org> wrote:
This SGTM (but of course it does ;).

Given that (a) there's a comment on task_runner.h saying "TaskRunner does not guarantee the order in which posted tasks are run, whether tasks overlap, or whether they're run on a particular thread." and (b) TaskRunner already has virtual methods with default impls (e.g., OnDestruct()), I think that you could sharpen this by giving TaskRunner a default impl that returns false. It would then be clear that a subclass overriding RunsTasksInCurrentSequence() implies that that subclass is providing *some* kind of sequencing.

SGTM
Reply all
Reply to author
Forward
0 new messages