Replace base::Callback with base::OnceCallback in base::PendingTask

Replace base::Callback with base::OnceCallback in base::PendingTask

After this CL, PendingTask holds the posted task as OnceClosure. And to
do that, PendingTasks get non-const on a task invocation, due to the
mutation the posted task for the consumption by the task invocation.

BUG=554299
Committed: https://crrev.com/739ffe3fd7b83bcb9ef7eb2e4b5c52fdbf35f59d
Cr-Commit-Position: refs/heads/master@{#425318}

[tzik, 2016-09-30 14:25:21]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 14:25:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 14:47:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-30 14:47:34]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[tzik, 2016-10-06 04:37:16]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-06 04:37:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tzik, 2016-10-06 04:48:29]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-06 04:48:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-06 05:05:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-06 05:05:22]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[tzik, 2016-10-06 05:27:37]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-06 05:27:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-06 05:44:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-06 05:44:15]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[tzik, 2016-10-06 06:07:56]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-06 06:08:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-06 06:40:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-06 06:40:17]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[tzik, 2016-10-06 06:58:41]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-06 06:58:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-06 07:30:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-06 07:30:51]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[tzik, 2016-10-06 09:23:43]

Description was changed from

==========
Replace legacy base::Callback with base::OnceCallback in base::PendingTask

BUG=
==========

to

==========
Replace legacy base::Callback with base::OnceCallback in base::PendingTask

TODO: Fix ios_simulator failure. Maybe broken on BrowserScheduler experiment.

BUG=
==========

[tzik, 2016-10-06 09:23:58]

tzik@chromium.org changed reviewers:
+ fdoray@chromium.org

[tzik, 2016-10-06 11:56:53]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-06 11:57:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-06 14:11:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-06 14:11:19]

Dry run: This issue passed the CQ dry run.

[fdoray, 2016-10-06 17:37:23]

https://codereview.chromium.org/2386653002/diff/140001/base/bind.h
File base/bind.h (right):

https://codereview.chromium.org/2386653002/diff/140001/base/bind.h#newcode86
base/bind.h:86: BASE_EXPORT internal::RepeatingClosure
UnsafeConvertOnceClosureToRepeating(
Do you plan to use OnceClosure instead of Closure in TaskRunner? I would be in
favor of that change.

If so, add a TODO to remove this function once TaskRunner uses OnceClosure
(third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc will no
longer need this conversion)

https://codereview.chromium.org/2386653002/diff/140001/base/debug/task_annota...
File base/debug/task_annotator.cc (right):

https://codereview.chromium.org/2386653002/diff/140001/base/debug/task_annota...
base/debug/task_annotator.cc:52: std::move(pending_task->task).Run();
It's annoying that Run() can only be called on an l-value. What would have
prevented Run() from being implemented like this?

  R Run(Args... args) {
    CallbackType& cb = static_cast<CallbackType&>(*this);
    PolymorphicInvoke f =
        reinterpret_cast<PolymorphicInvoke>(cb.polymorphic_invoke());

    // Move |bind_state| to ensure that it is deleted after the invocation. It's
    // not safe to touch |this| after the invocation, since running the bound
    // function may destroy |this|.
    scoped_refptr<BindStateBase> bind_state = std::move(cb.bind_state_);

    return f(bind_state.get(), std::forward<Args>(args)...);
  }

https://codereview.chromium.org/2386653002/diff/140001/base/pending_task.h
File base/pending_task.h (right):

https://codereview.chromium.org/2386653002/diff/140001/base/pending_task.h#ne...
base/pending_task.h:18: using internal::OnceClosure;
Avoid using "using" in a header file.

https://codereview.chromium.org/2386653002/diff/140001/base/task_scheduler/se...
File base/task_scheduler/sequence.h (right):

https://codereview.chromium.org/2386653002/diff/140001/base/task_scheduler/se...
base/task_scheduler/sequence.h:48: // Returns the task in front of the
sequence's queue, if any.
// The returned Task is non-const to allow running its OnceClosure. Any other
mutation may result in undefined behavior.

https://codereview.chromium.org/2386653002/diff/140001/base/task_scheduler/ta...
File base/task_scheduler/task_tracker.h (right):

https://codereview.chromium.org/2386653002/diff/140001/base/task_scheduler/ta...
base/task_scheduler/task_tracker.h:59: // |task| to be posted before this is
called.
// |task| is non-const to allow running its OnceClosure. It will not be mutated
in other ways.

https://codereview.chromium.org/2386653002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc
(right):

https://codereview.chromium.org/2386653002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:323:
base::UnsafeConvertOnceClosureToRepeating(
Add a TODO to remove this unsafe conversion once TaskRunner uses OnceClosure.

[tzik, 2016-10-12 07:44:01]

Patchset #9 (id:160001) has been deleted

[tzik, 2016-10-13 04:26:10]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 04:26:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tzik, 2016-10-13 05:29:39]

Description was changed from

==========
Replace legacy base::Callback with base::OnceCallback in base::PendingTask

TODO: Fix ios_simulator failure. Maybe broken on BrowserScheduler experiment.

BUG=
==========

to

==========
Replace legacy base::Callback with base::OnceCallback in base::PendingTask

BUG=
==========

[tzik, 2016-10-13 05:37:31]

Description was changed from

==========
Replace legacy base::Callback with base::OnceCallback in base::PendingTask

BUG=
==========

to

==========
Replace legacy base::Callback with base::OnceCallback in base::PendingTask

After this CL, PendingTask holds the posted task as OnceClosure. And to
do that, PendingTasks get non-const on a task invocation, due to the
mutation the posted task for the consumption by the task invocation.

BUG=554299
==========

[tzik, 2016-10-13 05:38:24]

tzik@chromium.org changed reviewers:
+ derat@chromium.org

[tzik, 2016-10-13 05:39:11]

tzik@chromium.org changed reviewers:
+ dcheng@chromium.org

[tzik, 2016-10-13 05:39:52]

tzik@chromium.org changed reviewers:
+ alexclarke@chromium.org

[tzik, 2016-10-13 05:52:36]

Description was changed from

==========
Replace legacy base::Callback with base::OnceCallback in base::PendingTask

After this CL, PendingTask holds the posted task as OnceClosure. And to
do that, PendingTasks get non-const on a task invocation, due to the
mutation the posted task for the consumption by the task invocation.

BUG=554299
==========

to

==========
Replace base::Callback with base::OnceCallback in base::PendingTask

After this CL, PendingTask holds the posted task as OnceClosure. And to
do that, PendingTasks get non-const on a task invocation, due to the
mutation the posted task for the consumption by the task invocation.

BUG=554299
==========

[tzik, 2016-10-13 05:58:04]

Adding owners for review.
PTAL to:
dcheng: //base,
fdoray: //base/task_scheduler and overall,
derat: //components/timers,
alexclarke: Blink platform/scheduler.

https://codereview.chromium.org/2386653002/diff/140001/base/bind.h
File base/bind.h (right):

https://codereview.chromium.org/2386653002/diff/140001/base/bind.h#newcode86
base/bind.h:86: BASE_EXPORT internal::RepeatingClosure
UnsafeConvertOnceClosureToRepeating(
On 2016/10/06 17:37:22, fdoray wrote:
> Do you plan to use OnceClosure instead of Closure in TaskRunner? I would be in
> favor of that change.
> 

Yes, here is a CL to do that, though it's a bit stale now.
https://codereview.chromium.org/2122543002/

> If so, add a TODO to remove this function once TaskRunner uses OnceClosure
> (third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc will
no
> longer need this conversion)

Done.

https://codereview.chromium.org/2386653002/diff/140001/base/debug/task_annota...
File base/debug/task_annotator.cc (right):

https://codereview.chromium.org/2386653002/diff/140001/base/debug/task_annota...
base/debug/task_annotator.cc:52: std::move(pending_task->task).Run();
On 2016/10/06 17:37:22, fdoray wrote:
> It's annoying that Run() can only be called on an l-value. What would have
> prevented Run() from being implemented like this?
> 
>   R Run(Args... args) {
>     CallbackType& cb = static_cast<CallbackType&>(*this);
>     PolymorphicInvoke f =
>         reinterpret_cast<PolymorphicInvoke>(cb.polymorphic_invoke());
> 
>     // Move |bind_state| to ensure that it is deleted after the invocation.
It's
>     // not safe to touch |this| after the invocation, since running the bound
>     // function may destroy |this|.
>     scoped_refptr<BindStateBase> bind_state = std::move(cb.bind_state_);
> 
>     return f(bind_state.get(), std::forward<Args>(args)...);
>   }

Per discussion on the mail thread:
In the new semantics, OnceClosure::Run() consumes the closure object itself and
clears itself. So we'd like the caller to mark it by std::move() explicitly.

https://codereview.chromium.org/2386653002/diff/140001/base/pending_task.h
File base/pending_task.h (right):

https://codereview.chromium.org/2386653002/diff/140001/base/pending_task.h#ne...
base/pending_task.h:18: using internal::OnceClosure;
On 2016/10/06 17:37:22, fdoray wrote:
> Avoid using "using" in a header file.

Removed. This is no longer needed after I moved it out from the internal
namespace.

https://codereview.chromium.org/2386653002/diff/140001/base/task_scheduler/se...
File base/task_scheduler/sequence.h (right):

https://codereview.chromium.org/2386653002/diff/140001/base/task_scheduler/se...
base/task_scheduler/sequence.h:48: // Returns the task in front of the
sequence's queue, if any.
On 2016/10/06 17:37:22, fdoray wrote:
> // The returned Task is non-const to allow running its OnceClosure. Any other
> mutation may result in undefined behavior.

Reverted this part per your CL.

https://codereview.chromium.org/2386653002/diff/140001/base/task_scheduler/ta...
File base/task_scheduler/task_tracker.h (right):

https://codereview.chromium.org/2386653002/diff/140001/base/task_scheduler/ta...
base/task_scheduler/task_tracker.h:59: // |task| to be posted before this is
called.
On 2016/10/06 17:37:22, fdoray wrote:
> // |task| is non-const to allow running its OnceClosure. It will not be
mutated
> in other ways.

It's now obsolete after your CL, right?
Its type is now std::unique_ptr<Task>, and now it's clear that RunTask retains
the ownership.

https://codereview.chromium.org/2386653002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc
(right):

https://codereview.chromium.org/2386653002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:323:
base::UnsafeConvertOnceClosureToRepeating(
On 2016/10/06 17:37:22, fdoray wrote:
> Add a TODO to remove this unsafe conversion once TaskRunner uses OnceClosure.

Done.

[commit-bot: I haz the power, 2016-10-13 07:45:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-13 07:45:35]

Dry run: This issue passed the CQ dry run.

[alex clarke (OOO till 29th), 2016-10-13 09:01:31]

Interesting! I suspect the majority of closures posted do run at most once, so I
can see why you'd want to do this.

A possible performance concern: The blink scheduler does reuse some closures
since base::Bind does have some overhead.

TaskQueueImpl::from_main_thread_immediate_do_work_closure_
TaskQueueImpl::from_other_thread_immediate_do_work_closure
RendererSchedulerImpl::update_policy_closure_
CancelableClosureHolder::callback_

Can you still post a RepeatingClosure or will that be banned?

[tzik, 2016-10-13 09:09:23]

On 2016/10/13 09:01:31, alex clarke wrote:
> Interesting! I suspect the majority of closures posted do run at most once, so
I
> can see why you'd want to do this.
> 
> A possible performance concern: The blink scheduler does reuse some closures
> since base::Bind does have some overhead.
> 
> TaskQueueImpl::from_main_thread_immediate_do_work_closure_
> TaskQueueImpl::from_other_thread_immediate_do_work_closure
> RendererSchedulerImpl::update_policy_closure_
> CancelableClosureHolder::callback_
> 
> Can you still post a RepeatingClosure or will that be banned?

We'll keep able to post them since RepeatingClosure is convertible to
OnceClosure without extra cost.

[alex clarke (OOO till 29th), 2016-10-13 09:11:38]

On 2016/10/13 09:09:23, tzik wrote:
> On 2016/10/13 09:01:31, alex clarke wrote:
> > Interesting! I suspect the majority of closures posted do run at most once,
so
> I
> > can see why you'd want to do this.
> > 
> > A possible performance concern: The blink scheduler does reuse some closures
> > since base::Bind does have some overhead.
> > 
> > TaskQueueImpl::from_main_thread_immediate_do_work_closure_
> > TaskQueueImpl::from_other_thread_immediate_do_work_closure
> > RendererSchedulerImpl::update_policy_closure_
> > CancelableClosureHolder::callback_
> > 
> > Can you still post a RepeatingClosure or will that be banned?
> 
> We'll keep able to post them since RepeatingClosure is convertible to
> OnceClosure without extra cost.

scheduler/ LGTM

[fdoray, 2016-10-13 13:58:34]

lgtm

[Daniel Erat, 2016-10-13 18:02:54]

lgtm for components/timers/alarm_timer_chromeos.cc

[dcheng, 2016-10-14 02:35:02]

https://codereview.chromium.org/2386653002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc
(right):

https://codereview.chromium.org/2386653002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:325:
base::UnsafeConvertOnceClosureToRepeating(
Can we just define UnsafeConvert directly here instead of defining it in base? I
think it's just used here, unless I'm missing something.

[tzik, 2016-10-14 05:04:34]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-14 05:04:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tzik, 2016-10-14 05:07:43]

https://codereview.chromium.org/2386653002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc
(right):

https://codereview.chromium.org/2386653002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:325:
base::UnsafeConvertOnceClosureToRepeating(
On 2016/10/14 02:35:02, dcheng wrote:
> Can we just define UnsafeConvert directly here instead of defining it in base?
I
> think it's just used here, unless I'm missing something.

Done.

[dcheng, 2016-10-14 05:24:32]

lgtm

[commit-bot: I haz the power, 2016-10-14 06:58:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-14 06:58:01]

Dry run: This issue passed the CQ dry run.

[fdoray, 2016-10-14 14:28:44]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2016-10-14 14:28:45]

The patchset sent to the CQ was uploaded after l-g-t-m from fdoray@chromium.org,
derat@chromium.org, alexclarke@chromium.org
Link to the patchset: https://codereview.chromium.org/2386653002/#ps220001
(title: "move UnsafeConvertOnceClosureToRepeating")

[commit-bot: I haz the power, 2016-10-14 14:28:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-14 14:35:22]

Description was changed from

==========
Replace base::Callback with base::OnceCallback in base::PendingTask

After this CL, PendingTask holds the posted task as OnceClosure. And to
do that, PendingTasks get non-const on a task invocation, due to the
mutation the posted task for the consumption by the task invocation.

BUG=554299
==========

to

==========
Replace base::Callback with base::OnceCallback in base::PendingTask

After this CL, PendingTask holds the posted task as OnceClosure. And to
do that, PendingTasks get non-const on a task invocation, due to the
mutation the posted task for the consumption by the task invocation.

BUG=554299
==========

[commit-bot: I haz the power, 2016-10-14 14:35:24]

Committed patchset #11 (id:220001)

[commit-bot: I haz the power, 2016-10-14 14:39:37]

Description was changed from

==========
Replace base::Callback with base::OnceCallback in base::PendingTask

After this CL, PendingTask holds the posted task as OnceClosure. And to
do that, PendingTasks get non-const on a task invocation, due to the
mutation the posted task for the consumption by the task invocation.

BUG=554299
==========

to

==========
Replace base::Callback with base::OnceCallback in base::PendingTask

After this CL, PendingTask holds the posted task as OnceClosure. And to
do that, PendingTasks get non-const on a task invocation, due to the
mutation the posted task for the consumption by the task invocation.

BUG=554299

Committed: https://crrev.com/739ffe3fd7b83bcb9ef7eb2e4b5c52fdbf35f59d
Cr-Commit-Position: refs/heads/master@{#425318}
==========

[commit-bot: I haz the power, 2016-10-14 14:39:38]

Patchset 11 (id:??) landed as
https://crrev.com/739ffe3fd7b83bcb9ef7eb2e4b5c52fdbf35f59d
Cr-Commit-Position: refs/heads/master@{#425318}