Add {CancelableTaskManager} to handle {Cancelable} concurrent tasks.

Add {CancelableTaskManager} to handle {Cancelable} concurrent tasks.

This change binds each {Cancelable} task to a so-called {CancelableTaskManager},
which is then used to handle concurrent cancelation as well as synchronizing
shutdown for already running tasks.  Since ownership of tasks is transferred to
the platform executing a task (destructor), handling in the manager uses integer
ids. Note that this also mitigates (modulo integer size) the ABA problem.

All handling of {Cancelable} tasks is now encapsulated into the corresponding
manager, which is instantiated for each isolate.

R=hpayer@chromium.org
BUG=chromium:524425
LOG=N
CQ_EXTRA_TRYBOTS=tryserver.v8:v8_linux_arm64_gc_stress_dbg;tryserver.v8:v8_linux_gc_stress_dbg;tryserver.v8:v8_mac_gc_stress_dbg;tryserver.v8:v8_linux64_msan_rel;tryserver.v8:v8_linux64_tsan_rel;tryserver.v8:v8_mac64_asan_rel

Committed: https://crrev.com/60e7ea8a9970f5a08c14a0aa052c099ac4ea4c08
Cr-Commit-Position: refs/heads/master@{#31997}

[Hannes Payer (out of office), 2015-11-04 23:19:00]

hpayer@chromium.org changed reviewers:
+ hpayer@chromium.org

[Hannes Payer (out of office), 2015-11-04 23:19:00]

https://codereview.chromium.org/1409993012/diff/1/src/cancelable-task.cc
File src/cancelable-task.cc (right):

https://codereview.chromium.org/1409993012/diff/1/src/cancelable-task.cc#newc...
src/cancelable-task.cc:15: : isolate_(isolate), status_(kWaiting) {
atomic set?

https://codereview.chromium.org/1409993012/diff/1/src/cancelable-task.cc#newc...
src/cancelable-task.cc:21: if (CanRun() || IsRunning() || IsCanceled()) {
Why would you CanRun here?

https://codereview.chromium.org/1409993012/diff/1/src/cancelable-task.h
File src/cancelable-task.h (right):

https://codereview.chromium.org/1409993012/diff/1/src/cancelable-task.h#newco...
src/cancelable-task.h:31: bool CanRun() { return status_.TrySetValue(kWaiting,
kRunning); }
I think TryRun matches better the semantics.

https://codereview.chromium.org/1409993012/diff/1/src/cancelable-task.h#newco...
src/cancelable-task.h:49: friend class Isolate;
Why friends?

https://codereview.chromium.org/1409993012/diff/1/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/1409993012/diff/1/src/isolate.cc#newcode1883
src/isolate.cc:1883: while (true) {
Why the while loop?

https://codereview.chromium.org/1409993012/diff/1/src/isolate.cc#newcode2807
src/isolate.cc:2807: base::LockGuard<base::Mutex> guard(&isolate_mutex_);
Are these methods not always called from the main thread? Why the mutex?

[Michael Lippautz, 2015-11-05 22:38:25]

Description was changed from

==========
Revamp {Cancelable} to also be usable for background tasks

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

to

==========
Revamp {Cancelable} to also be usable for background tasks

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

[Michael Lippautz, 2015-11-05 22:38:46]

Description was changed from

==========
Revamp {Cancelable} to also be usable for background tasks

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

to

==========
Revamp {Cancelable} and {Isolate} to be usable for background tasks.

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

[Michael Lippautz, 2015-11-05 22:52:42]

Description was changed from

==========
Revamp {Cancelable} and {Isolate} to be usable for background tasks.

R=hpayer@chromium.org
BUG=
==========

to

==========
Revamp {Cancelable} and {Isolate} to be usable for background tasks.

This change allows {Cancelable} to be used for foreground and background tasks.
In particular, tasks deriving from {Cancelable} can be canceled from other tasks
(foreground and background) and will be canceled upon isolate shutdown. If
cancelling is not possible, e.g. because the background task has already been
started, the isolate will wait during shutdown.


R=hpayer@chromium.org
BUG=
==========

[Michael Lippautz, 2015-11-05 22:55:17]

Patchset #2 (id:20001) has been deleted

[Michael Lippautz, 2015-11-05 23:03:07]

Patchset #2 (id:40001) has been deleted

[Michael Lippautz, 2015-11-05 23:04:16]

PTAL -- this is the rewrite after we discussed the design goals offline.

It does not require blocking, fore- and background tasks share a similar
interface, and it also integrates well with idle tasks.

[Michael Lippautz, 2015-11-05 23:08:17]

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

[commit-bot: I haz the power, 2015-11-05 23:08:31]

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

[commit-bot: I haz the power, 2015-11-05 23:28:33]

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

[commit-bot: I haz the power, 2015-11-05 23:28:34]

Dry run: This issue passed the CQ dry run.

[Michael Lippautz, 2015-11-10 02:25:36]

Description was changed from

==========
Revamp {Cancelable} and {Isolate} to be usable for background tasks.

This change allows {Cancelable} to be used for foreground and background tasks.
In particular, tasks deriving from {Cancelable} can be canceled from other tasks
(foreground and background) and will otherwise be canceled upon isolate
shutdown. If cancelling is not possible, e.g. because the background task has
already been started, the isolate will wait during shutdown.

R=hpayer@chromium.org
BUG=
==========

to

==========
Add {CancelableTaskManager} to handle {Cancelable} concurrent tasks.

This change binds each {Cancelable} task to a so-called {CancelableTaskManager},
which is used to handle concurrent
cancelation as well as blocking for already running tasks during shutdown. Since
ownership of tasks is transferred to the
platform executing a task (destructor), handling in the manager uses ids. Note
that this also mitigates (% integer size) the
ABA problem.

The handling is now encapsulated into the corresponding manger, which is
instantiated for each isolate.

R=hpayer@chromium.org
BUG=
==========

[Michael Lippautz, 2015-11-10 02:32:20]

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

[commit-bot: I haz the power, 2015-11-10 02:32:29]

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

[commit-bot: I haz the power, 2015-11-10 02:50:23]

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

[commit-bot: I haz the power, 2015-11-10 02:50:24]

Dry run: This issue passed the CQ dry run.

[Michael Lippautz, 2015-11-10 18:00:05]

https://codereview.chromium.org/1409993012/diff/80001/src/cancelable-task.cc
File src/cancelable-task.cc (right):

https://codereview.chromium.org/1409993012/diff/80001/src/cancelable-task.cc#...
src/cancelable-task.cc:42: while
(cancelable_tasks_.Lookup(reinterpret_cast<void*>(id), id) != nullptr) {
We could just add a CHECK here for overflow. Also switching to a 64-bit counter
would avoid this completely.

[Michael Lippautz, 2015-11-11 18:18:30]

Patchset #4 (id:100001) has been deleted

[Michael Lippautz, 2015-11-11 19:19:00]

Patchset #5 (id:140001) has been deleted

[Michael Lippautz, 2015-11-11 23:37:23]

Patchset #5 (id:160001) has been deleted

[Michael Lippautz, 2015-11-12 19:21:39]

Patchset #5 (id:180001) has been deleted

[Hannes Payer (out of office), 2015-11-12 22:01:50]

First round of API comments.

https://codereview.chromium.org/1409993012/diff/200001/src/cancelable-task.h
File src/cancelable-task.h (right):

https://codereview.chromium.org/1409993012/diff/200001/src/cancelable-task.h#...
src/cancelable-task.h:26: // Registers a new {Cancelable} task.
Specify what the return value is.

https://codereview.chromium.org/1409993012/diff/200001/src/cancelable-task.h#...
src/cancelable-task.h:30: // task has not been started yet and could be
canceled.
It is not clear to me what the semantics of this API are, could you be more
precise here?

https://codereview.chromium.org/1409993012/diff/200001/src/cancelable-task.h#...
src/cancelable-task.h:57: // Never invoke after handing over the task to the
platform!
Please explain why.

https://codereview.chromium.org/1409993012/diff/200001/src/cancelable-task.h#...
src/cancelable-task.h:60: // Always invoke indirectly through the connected
{CancelableTaskManager}.
Should this really be public then? Or should it be private and friend with the
manager.

https://codereview.chromium.org/1409993012/diff/200001/src/cancelable-task.h#...
src/cancelable-task.h:75: enum Status {
Please explain the states.

[Michael Lippautz, 2015-11-12 22:15:21]

https://codereview.chromium.org/1409993012/diff/200001/src/cancelable-task.h
File src/cancelable-task.h (right):

https://codereview.chromium.org/1409993012/diff/200001/src/cancelable-task.h#...
src/cancelable-task.h:26: // Registers a new {Cancelable} task.
On 2015/11/12 22:01:50, Hannes Payer wrote:
> Specify what the return value is.

Done.

https://codereview.chromium.org/1409993012/diff/200001/src/cancelable-task.h#...
src/cancelable-task.h:30: // task has not been started yet and could be
canceled.
On 2015/11/12 22:01:50, Hannes Payer wrote:
> It is not clear to me what the semantics of this API are, could you be more
> precise here?

Done. Hope the more elaborate comment makes it clear.

https://codereview.chromium.org/1409993012/diff/200001/src/cancelable-task.h#...
src/cancelable-task.h:57: // Never invoke after handing over the task to the
platform!
On 2015/11/12 22:01:50, Hannes Payer wrote:
> Please explain why.

Done.

https://codereview.chromium.org/1409993012/diff/200001/src/cancelable-task.h#...
src/cancelable-task.h:60: // Always invoke indirectly through the connected
{CancelableTaskManager}.
On 2015/11/12 22:01:50, Hannes Payer wrote:
> Should this really be public then? Or should it be private and friend with the
> manager.

Done.

[Michael Lippautz, 2015-11-12 22:44:01]

Description was changed from

==========
Add {CancelableTaskManager} to handle {Cancelable} concurrent tasks.

This change binds each {Cancelable} task to a so-called {CancelableTaskManager},
which is then used to handle concurrent cancelation as well as synchronizing
shutdown for already running tasks.  Since ownership of tasks is transferred to
the platform executing a task (destructor), handling in the manager uses integer
ids. Note that this also mitigates (modulo integer size) the ABA problem.

All handling of {Cancelable} tasks is now encapsulated into the corresponding
manger, which is instantiated for each isolate.

R=hpayer@chromium.org
BUG=
==========

to

==========
Add {CancelableTaskManager} to handle {Cancelable} concurrent tasks.

This change binds each {Cancelable} task to a so-called {CancelableTaskManager},
which is then used to handle concurrent cancelation as well as synchronizing
shutdown for already running tasks.  Since ownership of tasks is transferred to
the platform executing a task (destructor), handling in the manager uses integer
ids. Note that this also mitigates (modulo integer size) the ABA problem.

All handling of {Cancelable} tasks is now encapsulated into the corresponding
manger, which is instantiated for each isolate.

R=hpayer@chromium.org
BUG=chromium:524425
LOG=N
==========

[Hannes Payer (out of office), 2015-11-13 20:03:22]

lgtm

https://codereview.chromium.org/1409993012/diff/220001/src/cancelable-task.cc
File src/cancelable-task.cc (right):

https://codereview.chromium.org/1409993012/diff/220001/src/cancelable-task.cc...
src/cancelable-task.cc:23: if (TryRun() || IsRunning()) {
Setting it to running here is a bit non-logical. Would it possible to abort it
in this case?

https://codereview.chromium.org/1409993012/diff/220001/src/cancelable-task.cc...
src/cancelable-task.cc:44: // If the id is still in use, try to find a new one.
Just say that the while loop is used in the overflow case.

https://codereview.chromium.org/1409993012/diff/220001/src/cancelable-task.h
File src/cancelable-task.h (right):

https://codereview.chromium.org/1409993012/diff/220001/src/cancelable-task.h#...
src/cancelable-task.h:28: // can be used to try to abort running a task by
calling {Abort}.
drop the running, it's cleaner

https://codereview.chromium.org/1409993012/diff/220001/src/cancelable-task.h#...
src/cancelable-task.h:31: // Try to abort running a task  identified by {id}.
The possible outcomes are:
task _remove one whitespace_ identified

https://codereview.chromium.org/1409993012/diff/220001/src/cancelable-task.h#...
src/cancelable-task.h:35: // (3) The task is not yet running (or finished) and
is canceled and removed.
"so it is"

https://codereview.chromium.org/1409993012/diff/220001/src/cancelable-task.h#...
src/cancelable-task.h:77: private:
Please describe the states in a comment.

https://codereview.chromium.org/1409993012/diff/220001/src/cancelable-task.h#...
src/cancelable-task.h:97: AtomicNumber<intptr_t> cancel_counter_;
Let's describe the purpose of this counter.

[Michael Lippautz, 2015-11-13 22:51:03]

Addressed all comments. Additionally I added a check for not generating 0 ids,
as they are not supported by our hashmap.

https://codereview.chromium.org/1409993012/diff/220001/src/cancelable-task.cc
File src/cancelable-task.cc (right):

https://codereview.chromium.org/1409993012/diff/220001/src/cancelable-task.cc...
src/cancelable-task.cc:23: if (TryRun() || IsRunning()) {
On 2015/11/13 20:03:22, Hannes Payer wrote:
> Setting it to running here is a bit non-logical. Would it possible to abort it
> in this case?

Directly aborting is not possible, since the manager might have been destroyed
already. Changing to running avoids an additional state.

https://codereview.chromium.org/1409993012/diff/220001/src/cancelable-task.cc...
src/cancelable-task.cc:44: // If the id is still in use, try to find a new one.
On 2015/11/13 20:03:22, Hannes Payer wrote:
> Just say that the while loop is used in the overflow case.

Done.

https://codereview.chromium.org/1409993012/diff/220001/src/cancelable-task.h
File src/cancelable-task.h (right):

https://codereview.chromium.org/1409993012/diff/220001/src/cancelable-task.h#...
src/cancelable-task.h:28: // can be used to try to abort running a task by
calling {Abort}.
On 2015/11/13 20:03:22, Hannes Payer wrote:
> drop the running, it's cleaner

Done.

https://codereview.chromium.org/1409993012/diff/220001/src/cancelable-task.h#...
src/cancelable-task.h:31: // Try to abort running a task  identified by {id}.
The possible outcomes are:
On 2015/11/13 20:03:22, Hannes Payer wrote:
> task _remove one whitespace_ identified

Done.

https://codereview.chromium.org/1409993012/diff/220001/src/cancelable-task.h#...
src/cancelable-task.h:35: // (3) The task is not yet running (or finished) and
is canceled and removed.
On 2015/11/13 20:03:22, Hannes Payer wrote:
> "so it is"

Done.

https://codereview.chromium.org/1409993012/diff/220001/src/cancelable-task.h#...
src/cancelable-task.h:77: private:
On 2015/11/13 20:03:22, Hannes Payer wrote:
> Please describe the states in a comment.

Done.

https://codereview.chromium.org/1409993012/diff/220001/src/cancelable-task.h#...
src/cancelable-task.h:97: AtomicNumber<intptr_t> cancel_counter_;
On 2015/11/13 20:03:22, Hannes Payer wrote:
> Let's describe the purpose of this counter.

Done.

[Michael Lippautz, 2015-11-13 22:52:26]

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

[commit-bot: I haz the power, 2015-11-13 22:52:29]

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

[commit-bot: I haz the power, 2015-11-13 23:13:33]

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

[commit-bot: I haz the power, 2015-11-13 23:13:34]

Dry run: This issue passed the CQ dry run.

[Michael Lippautz, 2015-11-13 23:18:26]

Description was changed from

==========
Add {CancelableTaskManager} to handle {Cancelable} concurrent tasks.

This change binds each {Cancelable} task to a so-called {CancelableTaskManager},
which is then used to handle concurrent cancelation as well as synchronizing
shutdown for already running tasks.  Since ownership of tasks is transferred to
the platform executing a task (destructor), handling in the manager uses integer
ids. Note that this also mitigates (modulo integer size) the ABA problem.

All handling of {Cancelable} tasks is now encapsulated into the corresponding
manger, which is instantiated for each isolate.

R=hpayer@chromium.org
BUG=chromium:524425
LOG=N
==========

to

==========
Add {CancelableTaskManager} to handle {Cancelable} concurrent tasks.

This change binds each {Cancelable} task to a so-called {CancelableTaskManager},
which is then used to handle concurrent cancelation as well as synchronizing
shutdown for already running tasks.  Since ownership of tasks is transferred to
the platform executing a task (destructor), handling in the manager uses integer
ids. Note that this also mitigates (modulo integer size) the ABA problem.

All handling of {Cancelable} tasks is now encapsulated into the corresponding
manger, which is instantiated for each isolate.

R=hpayer@chromium.org
BUG=chromium:524425
LOG=N
CQ_EXTRA_TRYBOTS=tryserver.v8:v8_linux_arm64_gc_stress_dbg;tryserver.v8:v8_linux_gc_stress_dbg;tryserver.v8:v8_mac_gc_stress_dbg;tryserver.v8:v8_linux64_asan_rel;tryserver.v8:v8_linux64_msan_rel;tryserver.v8:v8_linux64_tsan_rel;tryserver.v8:v8_mac64_asan_rel
==========

[Michael Lippautz, 2015-11-13 23:19:25]

Description was changed from

==========
Add {CancelableTaskManager} to handle {Cancelable} concurrent tasks.

This change binds each {Cancelable} task to a so-called {CancelableTaskManager},
which is then used to handle concurrent cancelation as well as synchronizing
shutdown for already running tasks.  Since ownership of tasks is transferred to
the platform executing a task (destructor), handling in the manager uses integer
ids. Note that this also mitigates (modulo integer size) the ABA problem.

All handling of {Cancelable} tasks is now encapsulated into the corresponding
manger, which is instantiated for each isolate.

R=hpayer@chromium.org
BUG=chromium:524425
LOG=N
CQ_EXTRA_TRYBOTS=tryserver.v8:v8_linux_arm64_gc_stress_dbg;tryserver.v8:v8_linux_gc_stress_dbg;tryserver.v8:v8_mac_gc_stress_dbg;tryserver.v8:v8_linux64_asan_rel;tryserver.v8:v8_linux64_msan_rel;tryserver.v8:v8_linux64_tsan_rel;tryserver.v8:v8_mac64_asan_rel
==========

to

==========
Add {CancelableTaskManager} to handle {Cancelable} concurrent tasks.

This change binds each {Cancelable} task to a so-called {CancelableTaskManager},
which is then used to handle concurrent cancelation as well as synchronizing
shutdown for already running tasks.  Since ownership of tasks is transferred to
the platform executing a task (destructor), handling in the manager uses integer
ids. Note that this also mitigates (modulo integer size) the ABA problem.

All handling of {Cancelable} tasks is now encapsulated into the corresponding
manger, which is instantiated for each isolate.

R=hpayer@chromium.org
BUG=chromium:524425
LOG=N
CQ_EXTRA_TRYBOTS=tryserver.v8:v8_linux_arm64_gc_stress_dbg;tryserver.v8:v8_linux_gc_stress_dbg;tryserver.v8:v8_mac_gc_stress_dbg;tryserver.v8:v8_linux64_msan_rel;tryserver.v8:v8_linux64_tsan_rel;tryserver.v8:v8_mac64_asan_rel
==========

[Michael Lippautz, 2015-11-13 23:20:01]

Description was changed from

==========
Add {CancelableTaskManager} to handle {Cancelable} concurrent tasks.

This change binds each {Cancelable} task to a so-called {CancelableTaskManager},
which is then used to handle concurrent cancelation as well as synchronizing
shutdown for already running tasks.  Since ownership of tasks is transferred to
the platform executing a task (destructor), handling in the manager uses integer
ids. Note that this also mitigates (modulo integer size) the ABA problem.

All handling of {Cancelable} tasks is now encapsulated into the corresponding
manger, which is instantiated for each isolate.

R=hpayer@chromium.org
BUG=chromium:524425
LOG=N
CQ_EXTRA_TRYBOTS=tryserver.v8:v8_linux_arm64_gc_stress_dbg;tryserver.v8:v8_linux_gc_stress_dbg;tryserver.v8:v8_mac_gc_stress_dbg;tryserver.v8:v8_linux64_msan_rel;tryserver.v8:v8_linux64_tsan_rel;tryserver.v8:v8_mac64_asan_rel
==========

to

==========
Add {CancelableTaskManager} to handle {Cancelable} concurrent tasks.

This change binds each {Cancelable} task to a so-called {CancelableTaskManager},
which is then used to handle concurrent cancelation as well as synchronizing
shutdown for already running tasks.  Since ownership of tasks is transferred to
the platform executing a task (destructor), handling in the manager uses integer
ids. Note that this also mitigates (modulo integer size) the ABA problem.

All handling of {Cancelable} tasks is now encapsulated into the corresponding
manager, which is instantiated for each isolate.

R=hpayer@chromium.org
BUG=chromium:524425
LOG=N
CQ_EXTRA_TRYBOTS=tryserver.v8:v8_linux_arm64_gc_stress_dbg;tryserver.v8:v8_linux_gc_stress_dbg;tryserver.v8:v8_mac_gc_stress_dbg;tryserver.v8:v8_linux64_msan_rel;tryserver.v8:v8_linux64_tsan_rel;tryserver.v8:v8_mac64_asan_rel
==========

[Michael Lippautz, 2015-11-13 23:20:13]

The CQ bit was checked by mlippautz@chromium.org

[Michael Lippautz, 2015-11-13 23:20:13]

The patchset sent to the CQ was uploaded after l-g-t-m from hpayer@chromium.org
Link to the patchset: https://codereview.chromium.org/1409993012/#ps240001
(title: "Addressed comments")

[commit-bot: I haz the power, 2015-11-13 23:20:24]

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

[commit-bot: I haz the power, 2015-11-14 01:20:13]

Committed patchset #7 (id:240001)

[commit-bot: I haz the power, 2015-11-14 23:22:05]

Patchset 7 (id:??) landed as
https://crrev.com/60e7ea8a9970f5a08c14a0aa052c099ac4ea4c08
Cr-Commit-Position: refs/heads/master@{#31997}