Make PendingTask move-only and pass it by value on retaining params

Make PendingTask move-only and pass it by value on retaining params

PendingTask is copied several time when it's created and consumed in
MessageLoop, and each copy operation implies two atomicops on ref-count
bump on Callback. This CL removes them by migrating to pass-by-move.

Chrome has performed ~100k atomic ops for Callback ref-count (50k inc
and 50k dec) on the first 15s after boot before this CL, and it will
be reduced to ~50k after this CL.

Committed: https://crrev.com/b6769d5fc8557a8bc03cc3a24fde154ce7a4d1eb
Cr-Commit-Position: refs/heads/master@{#404229}

[tzik, 2016-04-13 04:11:12]

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

[commit-bot: I haz the power, 2016-04-13 04:11:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1886453003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1886453003/1

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

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

[commit-bot: I haz the power, 2016-04-13 04:30:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1886453003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1886453003/20001

[commit-bot: I haz the power, 2016-04-13 05:41:37]

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

[commit-bot: I haz the power, 2016-04-13 05:41:37]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[tzik, 2016-04-13 11:07:57]

Description was changed from

==========
Make PendingTask move-only

123456789012345678901234567890123456789012345678901234567890123456789012

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

to

==========
Make PendingTask move-only and pass it by value on retaining params

PendingTask is copied several time when it's created and consumed in
MessageLoop, and each copy operation implies two atomicops on ref-count
bump on Callback. This CL removes them by migrating to pass-by-move.

Chrome has performed ~100k atomic ops for Callback ref-count (50k inc
and 50k dec) on the first 15s after boot before this CL, and it will
be reduced to ~50k after this CL.
==========

[tzik, 2016-04-13 11:12:34]

tzik@chromium.org changed reviewers:
+ skyostil@chromium.org, thakis@chromium.org

[tzik, 2016-04-13 11:12:35]

PTAL

[Sami, 2016-04-13 11:33:01]

Thanks for the optimization! How did you measure the number of atomic ops by the
way?

https://codereview.chromium.org/1886453003/diff/20001/base/message_loop/incom...
File base/message_loop/incoming_task_queue.cc (right):

https://codereview.chromium.org/1886453003/diff/20001/base/message_loop/incom...
base/message_loop/incoming_task_queue.cc:159:
DCHECK(pending_task->task.is_null());
Is this a valid assumption? I thought we should treat the moved-from object as
undefined after the move.

https://codereview.chromium.org/1886453003/diff/20001/base/threading/worker_p...
File base/threading/worker_pool_posix.cc (right):

https://codereview.chromium.org/1886453003/diff/20001/base/threading/worker_p...
base/threading/worker_pool_posix.cc:154: DCHECK(pending_task->task.is_null());
Ditto.

https://codereview.chromium.org/1886453003/diff/20001/components/scheduler/ba...
File components/scheduler/base/task_queue_impl.cc (right):

https://codereview.chromium.org/1886453003/diff/20001/components/scheduler/ba...
components/scheduler/base/task_queue_impl.cc:673: std::queue<Task>&
mutable_queue = const_cast<std::queue<Task>&>(queue);
Should this be a pointer instead of a mutable reference?

https://codereview.chromium.org/1886453003/diff/20001/components/scheduler/ba...
File components/scheduler/base/work_queue.cc (right):

https://codereview.chromium.org/1886453003/diff/20001/components/scheduler/ba...
components/scheduler/base/work_queue.cc:21: std::queue<TaskQueueImpl::Task>&
mutable_queue =
Ditto.

[tzik, 2016-04-13 14:34:58]

On 2016/04/13 11:33:01, Sami wrote:
> Thanks for the optimization! How did you measure the number of atomic ops by
the
> way?
> 

I added a LOG into CallbackBase as PS1 do, ran chrome on a empty profile and hit
Ctrl-C after 15s sleep.

[tzik, 2016-04-13 14:35:37]

Updated!

https://codereview.chromium.org/1886453003/diff/20001/base/message_loop/incom...
File base/message_loop/incoming_task_queue.cc (right):

https://codereview.chromium.org/1886453003/diff/20001/base/message_loop/incom...
base/message_loop/incoming_task_queue.cc:159:
DCHECK(pending_task->task.is_null());
On 2016/04/13 11:33:00, Sami wrote:
> Is this a valid assumption? I thought we should treat the moved-from object as
> undefined after the move.

Done.

https://codereview.chromium.org/1886453003/diff/20001/base/threading/worker_p...
File base/threading/worker_pool_posix.cc (right):

https://codereview.chromium.org/1886453003/diff/20001/base/threading/worker_p...
base/threading/worker_pool_posix.cc:154: DCHECK(pending_task->task.is_null());
On 2016/04/13 11:33:00, Sami wrote:
> Ditto.

Done.

https://codereview.chromium.org/1886453003/diff/20001/components/scheduler/ba...
File components/scheduler/base/task_queue_impl.cc (right):

https://codereview.chromium.org/1886453003/diff/20001/components/scheduler/ba...
components/scheduler/base/task_queue_impl.cc:673: std::queue<Task>&
mutable_queue = const_cast<std::queue<Task>&>(queue);
On 2016/04/13 11:33:00, Sami wrote:
> Should this be a pointer instead of a mutable reference?

Done.

https://codereview.chromium.org/1886453003/diff/20001/components/scheduler/ba...
File components/scheduler/base/work_queue.cc (right):

https://codereview.chromium.org/1886453003/diff/20001/components/scheduler/ba...
components/scheduler/base/work_queue.cc:21: std::queue<TaskQueueImpl::Task>&
mutable_queue =
On 2016/04/13 11:33:00, Sami wrote:
> Ditto.

Done.

[Sami, 2016-04-13 15:17:32]

https://codereview.chromium.org/1886453003/diff/20001/base/message_loop/incom...
File base/message_loop/incoming_task_queue.cc (right):

https://codereview.chromium.org/1886453003/diff/20001/base/message_loop/incom...
base/message_loop/incoming_task_queue.cc:159:
DCHECK(pending_task->task.is_null());
On 2016/04/13 14:35:37, tzik wrote:
> On 2016/04/13 11:33:00, Sami wrote:
> > Is this a valid assumption? I thought we should treat the moved-from object
as
> > undefined after the move.
> 
> Done.

I'm not sure we need to even do this. We've moved pending_task to the incoming
queue so it's no longer our concern :)

[tzik, 2016-04-14 19:10:28]

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

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

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1886453003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1886453003/40001

[commit-bot: I haz the power, 2016-04-14 19:17:26]

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

[commit-bot: I haz the power, 2016-04-14 19:17:27]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[tzik, 2016-04-14 19:44:23]

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

[tzik, 2016-04-14 19:44:30]

https://codereview.chromium.org/1886453003/diff/20001/base/message_loop/incom...
File base/message_loop/incoming_task_queue.cc (right):

https://codereview.chromium.org/1886453003/diff/20001/base/message_loop/incom...
base/message_loop/incoming_task_queue.cc:159:
DCHECK(pending_task->task.is_null());
On 2016/04/13 15:17:32, Sami wrote:
> On 2016/04/13 14:35:37, tzik wrote:
> > On 2016/04/13 11:33:00, Sami wrote:
> > > Is this a valid assumption? I thought we should treat the moved-from
object
> as
> > > undefined after the move.
> > 
> > Done.
> 
> I'm not sure we need to even do this. We've moved pending_task to the incoming
> queue so it's no longer our concern :)

Right, it's redundant since we know the move construction on Callback clears the
object. I added it just for readability and for making sure, since it's legal to
leave something non-null there as WTF containers do.

[commit-bot: I haz the power, 2016-04-14 19:44:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1886453003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1886453003/60001

[commit-bot: I haz the power, 2016-04-14 20:45:47]

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

[commit-bot: I haz the power, 2016-04-14 20:45:48]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Sami, 2016-04-15 13:02:53]

Thanks, lgtm.

[tzik, 2016-04-15 13:45:25]

I'll look into Windows build failure next week after I go back to my home
office...

[tzik, 2016-04-20 08:11:32]

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

[commit-bot: I haz the power, 2016-04-20 08:11:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1886453003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1886453003/80001

[tzik, 2016-04-20 08:58:59]

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

[commit-bot: I haz the power, 2016-04-20 08:59:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1886453003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1886453003/100001

[tzik, 2016-04-20 10:08:35]

Updated. I had to prevent instantiations of copy-ctor of std::queue to fix
windows build.
PTAL.

[commit-bot: I haz the power, 2016-04-20 10:16:24]

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

[commit-bot: I haz the power, 2016-04-20 10:16:25]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Sami, 2016-04-20 10:34:07]

Still lgtm.

[tzik, 2016-04-22 15:22:15]

Nico: Could you review this?

[Nico, 2016-04-22 15:32:16]

sorry, I missed this. basically lgtm. do you have any data on how expensive the
atomic ops are?

https://codereview.chromium.org/1886453003/diff/100001/base/stl_util.h
File base/stl_util.h (right):

https://codereview.chromium.org/1886453003/diff/100001/base/stl_util.h#newcod...
base/stl_util.h:261: template <typename PriorityQueue>
this is only called from two places. the loop is in at least three. not sure if
it makes sense to put this here but don't put the other thing here

https://codereview.chromium.org/1886453003/diff/100001/components/scheduler/b...
File components/scheduler/base/task_queue_impl.cc (right):

https://codereview.chromium.org/1886453003/diff/100001/components/scheduler/b...
components/scheduler/base/task_queue_impl.cc:678: mutable_queue->pop();
this loop pop()s from mutable_queue, the one in the next function doesn't. is
that an intentional difference?

https://codereview.chromium.org/1886453003/diff/100001/components/scheduler/b...
File components/scheduler/base/work_queue.cc (right):

https://codereview.chromium.org/1886453003/diff/100001/components/scheduler/b...
components/scheduler/base/work_queue.cc:27: mutable_queue->pop();
this one pop()s too

[tzik, 2016-04-25 11:36:11]

On 2016/04/22 15:32:16, Nico wrote:
> sorry, I missed this. basically lgtm. do you have any data on how expensive
the
> atomic ops are?

Hmm, it's hard to say. It's fast enough if there's no data contention: each
atomic inc takes 6-8ns on both Z620 and Nexus 9, where a non-atomic increment on
a volatile int takes 2-3ns.
If the object is on another CPU core, the atomic op causes a L3 cache miss and
its penalty is 40-70 cycle, which will be 20-50ns.
So, 100k atomic ops will be 0.6-5ms.

[tzik, 2016-04-25 11:36:27]

https://codereview.chromium.org/1886453003/diff/100001/base/stl_util.h
File base/stl_util.h (right):

https://codereview.chromium.org/1886453003/diff/100001/base/stl_util.h#newcod...
base/stl_util.h:261: template <typename PriorityQueue>
On 2016/04/22 15:32:16, Nico wrote:
> this is only called from two places. the loop is in at least three. not sure
if
> it makes sense to put this here but don't put the other thing here

Hmm, there are several user of the pattern. And I think this is a hacky code
that should not be distributed throughout Chromium.
Anyway, let me inline this function to the caller now, and propose it
separately.

https://codereview.chromium.org/1886453003/diff/100001/components/scheduler/b...
File components/scheduler/base/task_queue_impl.cc (right):

https://codereview.chromium.org/1886453003/diff/100001/components/scheduler/b...
components/scheduler/base/task_queue_impl.cc:678: mutable_queue->pop();
eOn 2016/04/22 15:32:16, Nico wrote:
> this loop pop()s from mutable_queue, the one in the next function doesn't. is
> that an intentional difference?

Yes, it's intentional. In priority_queue version, the first item of
mutable_queue was popped out in PopOutFromPriorityQueue().

Anyway, I inlined PopOutFromPriorityQueue into the caller.

https://codereview.chromium.org/1886453003/diff/100001/components/scheduler/b...
File components/scheduler/base/work_queue.cc (right):

https://codereview.chromium.org/1886453003/diff/100001/components/scheduler/b...
components/scheduler/base/work_queue.cc:27: mutable_queue->pop();
On 2016/04/22 15:32:16, Nico wrote:
> this one pop()s too

same to above.

[tzik, 2016-04-25 11:37:29]

The CQ bit was checked by tzik@chromium.org

[tzik, 2016-04-25 11:37:30]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org,
skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/1886453003/#ps120001
(title: "remove PopOutFromPriorityQueue")

[commit-bot: I haz the power, 2016-04-25 11:37:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1886453003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1886453003/120001

[commit-bot: I haz the power, 2016-04-25 13:22:39]

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

[commit-bot: I haz the power, 2016-04-25 13:22:40]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tzik, 2016-07-07 13:27:58]

Let me resurrect this. I finally understand why the mac test was failing.
It was due to broken PostDelayedTask impl in this CL, which should be fixed by
diff of PS8 and PS9.

Nico: Could you take another look to the PS8 and PS9 diff?

[Nico, 2016-07-07 13:30:10]

ah! lgtm.

[tzik, 2016-07-07 14:05:22]

The CQ bit was checked by tzik@chromium.org

[tzik, 2016-07-07 14:05:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/1886453003/#ps160001
(title: "mac test fix")

[commit-bot: I haz the power, 2016-07-07 14:05:34]

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-07-07 16:07:56]

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

[commit-bot: I haz the power, 2016-07-07 16:07:57]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[tzik, 2016-07-07 18:33:49]

The CQ bit was checked by tzik@chromium.org

[commit-bot: I haz the power, 2016-07-07 18:34: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-07-07 20:20:23]

Description was changed from

==========
Make PendingTask move-only and pass it by value on retaining params

PendingTask is copied several time when it's created and consumed in
MessageLoop, and each copy operation implies two atomicops on ref-count
bump on Callback. This CL removes them by migrating to pass-by-move.

Chrome has performed ~100k atomic ops for Callback ref-count (50k inc
and 50k dec) on the first 15s after boot before this CL, and it will
be reduced to ~50k after this CL.
==========

to

==========
Make PendingTask move-only and pass it by value on retaining params

PendingTask is copied several time when it's created and consumed in
MessageLoop, and each copy operation implies two atomicops on ref-count
bump on Callback. This CL removes them by migrating to pass-by-move.

Chrome has performed ~100k atomic ops for Callback ref-count (50k inc
and 50k dec) on the first 15s after boot before this CL, and it will
be reduced to ~50k after this CL.
==========

[commit-bot: I haz the power, 2016-07-07 20:20:24]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-07-07 20:20:53]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-07 20:23:20]

Description was changed from

==========
Make PendingTask move-only and pass it by value on retaining params

PendingTask is copied several time when it's created and consumed in
MessageLoop, and each copy operation implies two atomicops on ref-count
bump on Callback. This CL removes them by migrating to pass-by-move.

Chrome has performed ~100k atomic ops for Callback ref-count (50k inc
and 50k dec) on the first 15s after boot before this CL, and it will
be reduced to ~50k after this CL.
==========

to

==========
Make PendingTask move-only and pass it by value on retaining params

PendingTask is copied several time when it's created and consumed in
MessageLoop, and each copy operation implies two atomicops on ref-count
bump on Callback. This CL removes them by migrating to pass-by-move.

Chrome has performed ~100k atomic ops for Callback ref-count (50k inc
and 50k dec) on the first 15s after boot before this CL, and it will
be reduced to ~50k after this CL.

Committed: https://crrev.com/b6769d5fc8557a8bc03cc3a24fde154ce7a4d1eb
Cr-Commit-Position: refs/heads/master@{#404229}
==========

[commit-bot: I haz the power, 2016-07-07 20:23:21]

Patchset 9 (id:??) landed as
https://crrev.com/b6769d5fc8557a8bc03cc3a24fde154ce7a4d1eb
Cr-Commit-Position: refs/heads/master@{#404229}