scheduler: Ensure consistent delayed task ordering between task queues

scheduler: Ensure consistent delayed task ordering between task queues

When wakeups are scheduled for distinct task queues, they should be
ordered according to the tasks which caused the wakeups. Previously this
was only being done by looking at the the scheduled run times fo the
tasks, but we should also consider the sequence numbers for tasks with
identical run times.

BUG=696001
Review-Url: https://codereview.chromium.org/2718293003
Cr-Commit-Position: refs/heads/master@{#453927}
Committed: https://chromium.googlesource.com/chromium/src/+/8579f022a226b0182ba7c5a0a2b83815775a1faa

[Sami, 2017-02-28 18:23:28]

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

[Sami, 2017-02-28 18:23:40]

Description was changed from

==========
scheduler: Ensure consistent delayed task ordering between task queues

When wakeups are scheduled for distinct task queues, they should be
ordered according to the tasks which caused the wakeups. Previously this
was only being done by looking at the the scheduled run times fo the
tasks, but we should also consider the sequence numbers for tasks with
identical run times.

BUG=696001
==========

to

==========
scheduler: Ensure consistent delayed task ordering between task queues

When wakeups are scheduled for distinct task queues, they should be
ordered according to the tasks which caused the wakeups. Previously this
was only being done by looking at the the scheduled run times fo the
tasks, but we should also consider the sequence numbers for tasks with
identical run times.

BUG=696001
==========

[Sami, 2017-02-28 18:23:40]

skyostil@chromium.org changed reviewers:
+ altimin@chromium.org

[commit-bot: I haz the power, 2017-02-28 18:23:46]

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-02-28 20:10:57]

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

[commit-bot: I haz the power, 2017-02-28 20:10:58]

Dry run: This issue passed the CQ dry run.

[altimin, 2017-03-01 11:23:17]

lgtm % nits.

nit: You may want for some time to make sure that Alex's patch sticks for sure.
Reverting it after this patch lands will be hard due to conflicts.

https://codereview.chromium.org/2718293003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h
(right):

https://codereview.chromium.org/2718293003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h:184:
base::TimeTicks* out_next_wake_up_time,
Nit: time ticks and sequence number seem related. Can we group them in a struct
of sorts and return this new struct directly? It should make code more readable.

https://codereview.chromium.org/2718293003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scheduler/base/time_domain_unittest.cc
(right):

https://codereview.chromium.org/2718293003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/base/time_domain_unittest.cc:244:
TEST_F(TimeDomainTest, WakeupReadyDelayedQueuesWithIdenticalRuntimes) {
Nit: I'm slightly worried that we do something wrong, this test may flake
instead of failing consistently. Can we do better? For example, running this
test 10 times in a loop should help.

[Sami, 2017-03-01 12:17:42]

Thanks! I think if we run into problems with Alex's earlier patch, we can just
revert both if necessary.

https://codereview.chromium.org/2718293003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h
(right):

https://codereview.chromium.org/2718293003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h:184:
base::TimeTicks* out_next_wake_up_time,
On 2017/03/01 11:23:17, altimin wrote:
> Nit: time ticks and sequence number seem related. Can we group them in a
struct
> of sorts and return this new struct directly? It should make code more
readable.

The other option I considered was to just return a pointer to the PendingTask,
but that seems like it's exposing too much information. Added a struct instead,
WDYT?

https://codereview.chromium.org/2718293003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scheduler/base/time_domain_unittest.cc
(right):

https://codereview.chromium.org/2718293003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/base/time_domain_unittest.cc:244:
TEST_F(TimeDomainTest, WakeupReadyDelayedQueuesWithIdenticalRuntimes) {
On 2017/03/01 11:23:17, altimin wrote:
> Nit: I'm slightly worried that we do something wrong, this test may flake
> instead of failing consistently. Can we do better? For example, running this
> test 10 times in a loop should help.

Hmm, this should now be completely deterministic (unlike before) because we are
no longer looking at queue memory addresses when deciding how to schedule
wake-ups. Were you thinking of some other source of flake?

[Sami, 2017-03-01 12:18:44]

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

[commit-bot: I haz the power, 2017-03-01 12:18:53]

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

[alex clarke (OOO till 29th), 2017-03-01 13:38:51]

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

[alex clarke (OOO till 29th), 2017-03-01 13:38:51]

LGTM

(I'll get back to non work stuff now I promise)

https://codereview.chromium.org/2718293003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scheduler/base/time_domain_unittest.cc
(right):

https://codereview.chromium.org/2718293003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/base/time_domain_unittest.cc:244:
TEST_F(TimeDomainTest, WakeupReadyDelayedQueuesWithIdenticalRuntimes) {
On 2017/03/01 12:17:42, Sami wrote:
> On 2017/03/01 11:23:17, altimin wrote:
> > Nit: I'm slightly worried that we do something wrong, this test may flake
> > instead of failing consistently. Can we do better? For example, running this
> > test 10 times in a loop should help.
> 
> Hmm, this should now be completely deterministic (unlike before) because we
are
> no longer looking at queue memory addresses when deciding how to schedule
> wake-ups. Were you thinking of some other source of flake?

I believe this is deterministic.

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

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

[commit-bot: I haz the power, 2017-03-01 13:39:47]

Dry run: This issue passed the CQ dry run.

[Sami, 2017-03-01 13:53:02]

The CQ bit was checked by skyostil@chromium.org

[Sami, 2017-03-01 13:53:02]

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

[commit-bot: I haz the power, 2017-03-01 13:53:09]

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-03-01 13:58:00]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488376382026660,
"parent_rev": "a260bac5596e34409d6e7842b98e8024772c2e59", "commit_rev":
"8579f022a226b0182ba7c5a0a2b83815775a1faa"}

[commit-bot: I haz the power, 2017-03-01 13:58:39]

Description was changed from

==========
scheduler: Ensure consistent delayed task ordering between task queues

When wakeups are scheduled for distinct task queues, they should be
ordered according to the tasks which caused the wakeups. Previously this
was only being done by looking at the the scheduled run times fo the
tasks, but we should also consider the sequence numbers for tasks with
identical run times.

BUG=696001
==========

to

==========
scheduler: Ensure consistent delayed task ordering between task queues

When wakeups are scheduled for distinct task queues, they should be
ordered according to the tasks which caused the wakeups. Previously this
was only being done by looking at the the scheduled run times fo the
tasks, but we should also consider the sequence numbers for tasks with
identical run times.

BUG=696001

Review-Url: https://codereview.chromium.org/2718293003
Cr-Commit-Position: refs/heads/master@{#453927}
Committed:
https://chromium.googlesource.com/chromium/src/+/8579f022a226b0182ba7c5a0a2b8...
==========

[commit-bot: I haz the power, 2017-03-01 13:58:40]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/8579f022a226b0182ba7c5a0a2b8...