scheduler: Make virtual time expiration deterministic

scheduler: Make virtual time expiration deterministic

Since the execution order of tasks across task queues is not
guaranteed[1], the task that controls the expiration of the virtual time
budget can run in a random order w.r.t. the tasks virtual time is
actually controlling. This patch makes the ordering deterministic by
using the control task queue (i.e., always the highest priority) to
reset the virtual time policy when the budget expires.

BUG=696001, 701223
[1] https://codereview.chromium.org/2786083005/

Review-Url: https://codereview.chromium.org/2841463003
Cr-Commit-Position: refs/heads/master@{#468604}
Committed: https://chromium.googlesource.com/chromium/src/+/6b5d2e208a399b53facc7ab80fca62b35f427c46

[Sami, 2017-04-24 16:23:28]

Description was changed from

==========
scheduler: Make virtual time expiration deterministic

Since the execution order of tasks across task queues is not
guaranteed[1], the task that controls the expiration of the virtual time
budget can run in a random order w.r.t. the tasks virtual time is
actually controlling. This patch makes the ordering deterministic by
using the control task queue (i.e., always the highest priority) to
reset the virtual time policy when the budget expires.

BUG=696001,701223

[1] https://codereview.chromium.org/2786083005/
==========

to

==========
scheduler: Make virtual time expiration deterministic

Since the execution order of tasks across task queues is not
guaranteed[1], the task that controls the expiration of the virtual time
budget can run in a random order w.r.t. the tasks virtual time is
actually controlling. This patch makes the ordering deterministic by
using the control task queue (i.e., always the highest priority) to
reset the virtual time policy when the budget expires.

BUG=696001,701223

[1] https://codereview.chromium.org/2786083005/
==========

[Sami, 2017-04-24 16:23:28]

skyostil@chromium.org changed reviewers:
+ alexclarke@chromium.org, dgozman@chromium.org

[Sami, 2017-04-24 16:25:53]

alexclarke@:

third_party/WebKit/Source/platform/scheduler/..

dgozman@:

content/browser/devtools/protocol/devtools_protocol_browsertest.cc
third_party/WebKit/Source/web/InspectorEmulationAgent.cpp

PTAL.

[dgozman, 2017-04-24 17:10:30]

lgtm

https://codereview.chromium.org/2841463003/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/InspectorEmulationAgent.cpp (left):

https://codereview.chromium.org/2841463003/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/InspectorEmulationAgent.cpp:175:
virtual_time_budget_expired_task_handle_ =
nit: remove the field.

[Sami, 2017-04-24 17:33:47]

Thanks!

https://codereview.chromium.org/2841463003/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/InspectorEmulationAgent.cpp (left):

https://codereview.chromium.org/2841463003/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/InspectorEmulationAgent.cpp:175:
virtual_time_budget_expired_task_handle_ =
On 2017/04/24 17:10:30, dgozman wrote:
> nit: remove the field.

Done!

[alex clarke (OOO till 29th), 2017-04-25 11:53:17]

https://codereview.chromium.org/2841463003/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
(right):

https://codereview.chromium.org/2841463003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1861:
control_task_runner_->SetTimeDomain(time_domain);
I'm a little nervous about that, generally we don't ever expect the control task
queue to be throttled / disabled.  Do we need a second lazily created control
task queue?

[Sami, 2017-04-25 16:58:21]

https://codereview.chromium.org/2841463003/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
(right):

https://codereview.chromium.org/2841463003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1861:
control_task_runner_->SetTimeDomain(time_domain);
On 2017/04/25 11:53:17, alex clarke wrote:
> I'm a little nervous about that, generally we don't ever expect the control
task
> queue to be throttled / disabled.  Do we need a second lazily created control
> task queue?

That's a great point, I agree that we should be a little careful here. Added a
secondary queue instead.

[alex clarke (OOO till 29th), 2017-04-26 13:33:39]

lgtm

https://codereview.chromium.org/2841463003/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc
(right):

https://codereview.chromium.org/2841463003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc:182:
control_task_runner_ = WebTaskRunnerImpl::Create(
s/control_task_runner_/virtual_time_control_task_runner_

[Sami, 2017-04-26 15:26:49]

Thanks!

https://codereview.chromium.org/2841463003/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc
(right):

https://codereview.chromium.org/2841463003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc:182:
control_task_runner_ = WebTaskRunnerImpl::Create(
On 2017/04/26 13:33:39, alex clarke wrote:
> s/control_task_runner_/virtual_time_control_task_runner_

Indeed, done!

[Sami, 2017-04-26 15:26:56]

The CQ bit was checked by skyostil@chromium.org

[Sami, 2017-04-26 15:26:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
dgozman@chromium.org, alexclarke@chromium.org
Link to the patchset: https://codereview.chromium.org/2841463003/#ps60001
(title: "Review comments")

[commit-bot: I haz the power, 2017-04-26 15:27:17]

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-04-26 16:05:37]

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

[commit-bot: I haz the power, 2017-04-26 16:05:38]

Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sami, 2017-04-27 15:24:56]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2017-04-27 15:25:23]

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-04-27 15:48:54]

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

[commit-bot: I haz the power, 2017-04-27 15:48:55]

Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Sami, 2017-05-02 10:28:12]

The CQ bit was checked by skyostil@chromium.org

[Sami, 2017-05-02 10:28:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
dgozman@chromium.org, alexclarke@chromium.org
Link to the patchset: https://codereview.chromium.org/2841463003/#ps100001
(title: "Rebased")

[commit-bot: I haz the power, 2017-05-02 10:28:25]

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-05-02 12:03:14]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493720892581820,
"parent_rev": "252666c9c21919c74b0cbf1c4893f0aecfa41798", "commit_rev":
"6b5d2e208a399b53facc7ab80fca62b35f427c46"}

[commit-bot: I haz the power, 2017-05-02 12:03:25]

Description was changed from

==========
scheduler: Make virtual time expiration deterministic

Since the execution order of tasks across task queues is not
guaranteed[1], the task that controls the expiration of the virtual time
budget can run in a random order w.r.t. the tasks virtual time is
actually controlling. This patch makes the ordering deterministic by
using the control task queue (i.e., always the highest priority) to
reset the virtual time policy when the budget expires.

BUG=696001,701223

[1] https://codereview.chromium.org/2786083005/
==========

to

==========
scheduler: Make virtual time expiration deterministic

Since the execution order of tasks across task queues is not
guaranteed[1], the task that controls the expiration of the virtual time
budget can run in a random order w.r.t. the tasks virtual time is
actually controlling. This patch makes the ordering deterministic by
using the control task queue (i.e., always the highest priority) to
reset the virtual time policy when the budget expires.

BUG=696001,701223

[1] https://codereview.chromium.org/2786083005/

Review-Url: https://codereview.chromium.org/2841463003
Cr-Commit-Position: refs/heads/master@{#468604}
Committed:
https://chromium.googlesource.com/chromium/src/+/6b5d2e208a399b53facc7ab80fca...
==========

[commit-bot: I haz the power, 2017-05-02 12:03:26]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/6b5d2e208a399b53facc7ab80fca...