Implement async AbortAll for the compiler dispatcher

Implement async AbortAll for the compiler dispatcher

BUG=v8:5215
R=marja@chromium.org,vogelheim@chromium.org

Review-Url: https://codereview.chromium.org/2615603002
Cr-Commit-Position: refs/heads/master@{#42068}
Committed: https://chromium.googlesource.com/v8/v8/+/e426fdd52bf99364eaf335f547b30a026d079996

[jochen (gone - plz use gerrit), 2017-01-04 12:13:23]

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

[commit-bot: I haz the power, 2017-01-04 12:13:26]

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, 2017-01-04 12:41:25]

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

[commit-bot: I haz the power, 2017-01-04 12:41:26]

Dry run: This issue passed the CQ dry run.

[marja, 2017-01-04 12:54:43]

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

https://codereview.chromium.org/2615603002/diff/1/src/cancelable-task.cc#newc...
src/cancelable-task.cc:37: if (canceled_) {
Why is this needed now?

Is it because we might now cancel without waiting for all tasks to finish, and
when those tasks keep running, they might schedule new tasks?

(A comment would be good.)

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

https://codereview.chromium.org/2615603002/diff/1/src/cancelable-task.h#newco...
src/cancelable-task.h:57: TryAbortResult Cancel();
The naming is confusing but it's there because of this CL...

We should either have:
TryAbort
TryAbortAllAndWait (now CancelAndWait)
TryAbortAll (now Cancel)

or:

Cancel (now TryAbort)
CancelAllAndWait (now CancelAndWait)
CancelAll (now Cancel)

wdyt? (But this probably shouldn't be fixed by this CL?)

https://codereview.chromium.org/2615603002/diff/1/test/unittests/compiler-dis...
File test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc (right):

https://codereview.chromium.org/2615603002/diff/1/test/unittests/compiler-dis...
test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc:600: // Kick
off background tasks, racing with the AbortAll() call.
Instead of making your tests racy, you could use condition variables to ensure
that things happen in the order you want to. The task would wait on a condition
variable, and then you'd call your func.

https://codereview.chromium.org/2615603002/diff/1/test/unittests/compiler-dis...
test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc:605: while
(dispatcher.block_for_testing_.Value()) {
Here too you can wait on a condition variable.

https://codereview.chromium.org/2615603002/diff/1/test/unittests/compiler-dis...
test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc:624: // Busy
loop until the background task scheduled another AbortTask task.
Ditto

https://codereview.chromium.org/2615603002/diff/1/test/unittests/compiler-dis...
test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc:678: // Kick
off background tasks, racing with the AbortAll() call.
Ditto

[marja, 2017-01-04 12:55:15]

s/it's there because of this CL/it's not there because of this CL/

[marja, 2017-01-04 12:55:44]

On 2017/01/04 12:55:15, marja wrote:
> s/it's there because of this CL/it's not there because of this CL/

Or better: it's there before this CL :)

[vogelheim, 2017-01-04 12:58:08]

lgtm, but nitpicks & questions.

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

https://codereview.chromium.org/2615603002/diff/1/src/cancelable-task.cc#newc...
src/cancelable-task.cc:38: CHECK(task->Cancel());
Why are we sure that Cancel() will always return true in this context?

https://codereview.chromium.org/2615603002/diff/1/src/cancelable-task.cc#newc...
src/cancelable-task.cc:121: canceled_ = false;
I'm not sure I understang this code... 

Most CancelableTaskmanager calls lock the mutex, so noone should be able to
observe the changes to canceled_. Do those have any effect?


I guess I'm nitpicking/opinionating here, but maybe an easier way to write this
would be to split the loop, in one cancel-loop and one remove-loop. Something
like this:

for (auto it : cancelable_tasks_) it->second->Cancel();
std::remove_if(cancelable_tasks_.begin(), cancelable_tasks.end(),
               [](...whatever...) { return whatever.second->status_ !=
kRunning();});
return cancelable_tasks_.empty() ? ... : ...;

https://codereview.chromium.org/2615603002/diff/1/src/cancelable-task.cc#newc...
src/cancelable-task.cc:123: return tasks_running ? kTaskRunning : kTaskAborted;
tasks_running == !cancelable_tasks_.empty() ?
If so, you can drop that variable.

https://codereview.chromium.org/2615603002/diff/1/src/compiler-dispatcher/com...
File src/compiler-dispatcher/compiler-dispatcher.cc (right):

https://codereview.chromium.org/2615603002/diff/1/src/compiler-dispatcher/com...
src/compiler-dispatcher/compiler-dispatcher.cc:269: for (auto& kv : jobs_) {
kv ???

https://codereview.chromium.org/2615603002/diff/1/src/compiler-dispatcher/com...
src/compiler-dispatcher/compiler-dispatcher.cc:270:
WaitForJobIfRunningOnBackground(kv.second.get());
If !background_tasks_running, we expect WaitForJobIfRunningOnBackground to
always be a no-op, right?

[jochen (gone - plz use gerrit), 2017-01-04 13:18:29]

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

[jochen (gone - plz use gerrit), 2017-01-04 13:18:31]

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

https://codereview.chromium.org/2615603002/diff/1/src/cancelable-task.cc#newc...
src/cancelable-task.cc:121: canceled_ = false;
On 2017/01/04 at 12:58:08, vogelheim wrote:
> I'm not sure I understang this code... 
> 
> Most CancelableTaskmanager calls lock the mutex, so noone should be able to
observe the changes to canceled_. Do those have any effect?
> 
> 
> I guess I'm nitpicking/opinionating here, but maybe an easier way to write
this would be to split the loop, in one cancel-loop and one remove-loop.
Something like this:
> 
> for (auto it : cancelable_tasks_) it->second->Cancel();
> std::remove_if(cancelable_tasks_.begin(), cancelable_tasks.end(),
>                [](...whatever...) { return whatever.second->status_ !=
kRunning();});
> return cancelable_tasks_.empty() ? ... : ...;

ah, fair enough. I forgot about the lock..

remove_if doesn't work on maps... so I kept my loop

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

https://codereview.chromium.org/2615603002/diff/1/src/cancelable-task.h#newco...
src/cancelable-task.h:57: TryAbortResult Cancel();
On 2017/01/04 at 12:54:43, marja wrote:
> The naming is confusing but it's there because of this CL...
> 
> We should either have:
> TryAbort
> TryAbortAllAndWait (now CancelAndWait)
> TryAbortAll (now Cancel)
> 
> or:
> 
> Cancel (now TryAbort)
> CancelAllAndWait (now CancelAndWait)
> CancelAll (now Cancel)
> 
> wdyt? (But this probably shouldn't be fixed by this CL?)

what about just renaming Cancel to TryAbortAll? CancelAll() doesn't try, it just
does - but the other two just try their best

https://codereview.chromium.org/2615603002/diff/1/src/compiler-dispatcher/com...
File src/compiler-dispatcher/compiler-dispatcher.cc (right):

https://codereview.chromium.org/2615603002/diff/1/src/compiler-dispatcher/com...
src/compiler-dispatcher/compiler-dispatcher.cc:269: for (auto& kv : jobs_) {
On 2017/01/04 at 12:58:08, vogelheim wrote:
> kv ???

key/value... guess "it" is the more common name

https://codereview.chromium.org/2615603002/diff/1/src/compiler-dispatcher/com...
src/compiler-dispatcher/compiler-dispatcher.cc:270:
WaitForJobIfRunningOnBackground(kv.second.get());
On 2017/01/04 at 12:58:08, vogelheim wrote:
> If !background_tasks_running, we expect WaitForJobIfRunningOnBackground to
always be a no-op, right?

right

https://codereview.chromium.org/2615603002/diff/1/test/unittests/compiler-dis...
File test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc (right):

https://codereview.chromium.org/2615603002/diff/1/test/unittests/compiler-dis...
test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc:600: // Kick
off background tasks, racing with the AbortAll() call.
On 2017/01/04 at 12:54:43, marja wrote:
> Instead of making your tests racy, you could use condition variables to ensure
that things happen in the order you want to. The task would wait on a condition
variable, and then you'd call your func.

ah, the comment is just wrong, it's no longer a race

https://codereview.chromium.org/2615603002/diff/1/test/unittests/compiler-dis...
test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc:605: while
(dispatcher.block_for_testing_.Value()) {
On 2017/01/04 at 12:54:43, marja wrote:
> Here too you can wait on a condition variable.

then I'd need even more members just for testing...

[commit-bot: I haz the power, 2017-01-04 13:18:38]

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

[marja, 2017-01-04 13:28:36]

lgtm

https://codereview.chromium.org/2615603002/diff/1/test/unittests/compiler-dis...
File test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc (right):

https://codereview.chromium.org/2615603002/diff/1/test/unittests/compiler-dis...
test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc:605: while
(dispatcher.block_for_testing_.Value()) {
On 2017/01/04 13:18:31, jochen wrote:
> On 2017/01/04 at 12:54:43, marja wrote:
> > Here too you can wait on a condition variable.
> 
> then I'd need even more members just for testing...

Ah, right, because you can't mock the task and just test the dispatcher. And the
task just calls the real compilation interface (instead of calling something on
the mock platform), so can't mock there either.

[jochen (gone - plz use gerrit), 2017-01-04 13:34:50]

The CQ bit was unchecked by jochen@chromium.org

[jochen (gone - plz use gerrit), 2017-01-04 13:34:52]

The CQ bit was checked by jochen@chromium.org

[jochen (gone - plz use gerrit), 2017-01-04 13:34:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
vogelheim@chromium.org
Link to the patchset: https://codereview.chromium.org/2615603002/#ps20001
(title: "updates")

[commit-bot: I haz the power, 2017-01-04 13:34:59]

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, 2017-01-04 13:45:57]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1483536892879880,
"parent_rev": "20defd29e017df7019de9b00134c1c9565287723", "commit_rev":
"e426fdd52bf99364eaf335f547b30a026d079996"}

[commit-bot: I haz the power, 2017-01-04 13:46:01]

Description was changed from

==========
Implement async AbortAll for the compiler dispatcher

BUG=v8:5215
R=marja@chromium.org,vogelheim@chromium.org
==========

to

==========
Implement async AbortAll for the compiler dispatcher

BUG=v8:5215
R=marja@chromium.org,vogelheim@chromium.org

Review-Url: https://codereview.chromium.org/2615603002
Cr-Commit-Position: refs/heads/master@{#42068}
Committed:
https://chromium.googlesource.com/v8/v8/+/e426fdd52bf99364eaf335f547b30a026d0...
==========

[commit-bot: I haz the power, 2017-01-04 13:46:02]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/v8/v8/+/e426fdd52bf99364eaf335f547b30a026d0...

[Michael Achenbach, 2017-01-05 09:36:19]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/2615533005/ by machenbach@chromium.org.

The reason for reverting is: Times out on win dbg.