Allow tasks scheduled for the instant virtual time budges expires to run

Allow tasks scheduled for the instant virtual time budges expires to run

It feels strange that if a task is due to run at time t=1.0 and we gave
1s of virtual time budget that that task doesn't run.  This patch changes
that so the task will run by using a fence instead of disabling the queue.

BUG=696001
Review-Url: https://codereview.chromium.org/2909293002
Cr-Commit-Position: refs/heads/master@{#476225}
Committed: https://chromium.googlesource.com/chromium/src/+/255feb99a2941caa310d02b0af24e1936533021c

[alex clarke (OOO till 29th), 2017-05-30 14:28:41]

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

[alex clarke (OOO till 29th), 2017-05-30 14:29:04]

Description was changed from

==========
Allow tasks scheduled for the instant virtual time budges expires to run

It feels strange that if a task is due to run at time t=1.0 and we gave
1s of virtual time budget that that task doesn't run.  This patch changes
that so the task will run by using a fence instead of disabling the queue.

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

to

==========
Allow tasks scheduled for the instant virtual time budges expires to run

It feels strange that if a task is due to run at time t=1.0 and we gave
1s of virtual time budget that that task doesn't run.  This patch changes
that so the task will run by using a fence instead of disabling the queue.

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

[alex clarke (OOO till 29th), 2017-05-30 14:29:04]

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

[commit-bot: I haz the power, 2017-05-30 14:29:06]

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-05-30 14:29:17]

PTAL :)

[commit-bot: I haz the power, 2017-05-30 14:59:24]

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

[commit-bot: I haz the power, 2017-05-30 14:59:25]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[alex clarke (OOO till 29th), 2017-05-31 07:21:37]

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

[commit-bot: I haz the power, 2017-05-31 07:21:52]

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-05-31 08:40:39]

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

[commit-bot: I haz the power, 2017-05-31 08:40:40]

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

[Sami, 2017-05-31 10:47:43]

lgtm, this makes a lot of sense.

Just to confirm, we still need to post the virtual time control tasks as actual
control tasks to make sure they fire first at the exact deadline, right?

https://codereview.chromium.org/2909293002/diff/20001/content/browser/devtool...
File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right):

https://codereview.chromium.org/2909293002/diff/20001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_browsertest.cc:1425: // The
second timer shold not fire.
Please update this comment.

https://codereview.chromium.org/2909293002/diff/20001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_browsertest.cc:1428: // Let
virtual time advance for another second, which should make the second
This one looks out of date too.

[alex clarke (OOO till 29th), 2017-05-31 11:01:33]

On 2017/05/31 10:47:43, Sami wrote:
> lgtm, this makes a lot of sense.
> 
> Just to confirm, we still need to post the virtual time control tasks as
actual
> control tasks to make sure they fire first at the exact deadline, right?

Probably yes, or we risk other work sneaking in (although if it did, that should
be deterministic).

> 
>
https://codereview.chromium.org/2909293002/diff/20001/content/browser/devtool...
> File content/browser/devtools/protocol/devtools_protocol_browsertest.cc
(right):
> 
>
https://codereview.chromium.org/2909293002/diff/20001/content/browser/devtool...
> content/browser/devtools/protocol/devtools_protocol_browsertest.cc:1425: //
The
> second timer shold not fire.
> Please update this comment.
> 
>
https://codereview.chromium.org/2909293002/diff/20001/content/browser/devtool...
> content/browser/devtools/protocol/devtools_protocol_browsertest.cc:1428: //
Let
> virtual time advance for another second, which should make the second
> This one looks out of date too.

[alex clarke (OOO till 29th), 2017-05-31 11:01:48]

https://codereview.chromium.org/2909293002/diff/20001/content/browser/devtool...
File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right):

https://codereview.chromium.org/2909293002/diff/20001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_browsertest.cc:1425: // The
second timer shold not fire.
On 2017/05/31 10:47:43, Sami wrote:
> Please update this comment.

Done.

https://codereview.chromium.org/2909293002/diff/20001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_browsertest.cc:1428: // Let
virtual time advance for another second, which should make the second
On 2017/05/31 10:47:43, Sami wrote:
> This one looks out of date too.

Done.

[alex clarke (OOO till 29th), 2017-05-31 11:02:23]

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

[alex clarke (OOO till 29th), 2017-05-31 11:02:23]

+dgozman@ Could you please review the change in
content/browser/devtools/protocol/devtools_protocol_browsertest.cc

[alex clarke (OOO till 29th), 2017-05-31 11:02:31]

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

[commit-bot: I haz the power, 2017-05-31 11:02:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[altimin, 2017-05-31 11:44:05]

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

[altimin, 2017-05-31 11:44:06]

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

https://codereview.chromium.org/2909293002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:374:
if (GetMainThreadOnly().virtual_time_suspended)
Is virtual_time_expired a better name?

https://codereview.chromium.org/2909293002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1422:
DCHECK(!task_queue_throttler_->IsThrottled(pair.first.get()));
Also DCHECK for not having a fence already and virtual time not being suspended
twice?

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

https://codereview.chromium.org/2909293002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc:192:
if (!virtual_time_ || allow_virtual_time_to_advance_) {
This part is slightly hard to follow. Maybe something like that can work better:

bool timers_should_be_suspended = virtual_time_ &&
!allow_virtual_time_to_advance_;
if (timers_suspended_ != timers_should_be_suspended) {
  
if (timers_should_be_suspended) {
     renderer_scheduler_->InsertTimerFence();
  } else {
     renderer_scheduler_->RemoveTimerFence();
  }
  timers_suspended_ = timers_should_be_suspended;
}

[alex clarke (OOO till 29th), 2017-05-31 12:47:47]

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

[commit-bot: I haz the power, 2017-05-31 12:47:57]

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-05-31 15:06:40]

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

[commit-bot: I haz the power, 2017-05-31 15:06:41]

Dry run: This issue passed the CQ dry run.

[alex clarke (OOO till 29th), 2017-05-31 15:11:36]

PTAL

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

https://codereview.chromium.org/2909293002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:374:
if (GetMainThreadOnly().virtual_time_suspended)
On 2017/05/31 11:44:06, altimin wrote:
> Is virtual_time_expired a better name?

virtual_time_paused is possibly the most accurate name.

https://codereview.chromium.org/2909293002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1422:
DCHECK(!task_queue_throttler_->IsThrottled(pair.first.get()));
On 2017/05/31 11:44:06, altimin wrote:
> Also DCHECK for not having a fence already and virtual time not being
suspended
> twice?

Done.

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

https://codereview.chromium.org/2909293002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc:192:
if (!virtual_time_ || allow_virtual_time_to_advance_) {
On 2017/05/31 11:44:06, altimin wrote:
> This part is slightly hard to follow. Maybe something like that can work
better:
> 
> bool timers_should_be_suspended = virtual_time_ &&
> !allow_virtual_time_to_advance_;
> if (timers_suspended_ != timers_should_be_suspended) {
>   
> if (timers_should_be_suspended) {
>      renderer_scheduler_->InsertTimerFence();
>   } else {
>      renderer_scheduler_->RemoveTimerFence();
>   }
>   timers_suspended_ = timers_should_be_suspended;
> }

I've done something similar.

[alex clarke (OOO till 29th), 2017-05-31 16:48:49]

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

https://codereview.chromium.org/2909293002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:374:
if (GetMainThreadOnly().virtual_time_suspended)
On 2017/05/31 15:11:36, alex clarke wrote:
> On 2017/05/31 11:44:06, altimin wrote:
> > Is virtual_time_expired a better name?
> 
> virtual_time_paused is possibly the most accurate name.

I understand Alexander has a strong preference for _expired due to the budget
however I respectfully disagree. This state is entered when
WebViewSchedulerImpl::allow_virtual_time_to_advance_ is false and
WebViewSchedulerImpl::virtual_time_ is true.  That only happens when we are in
VirtualTimePolicy::PAUSE (hence my preferred name).

Now it is true that the layer above sets the policy to VirtualTimePolicy::PAUSE
when the virtual time budget expires, but that concept doesn't exist at this
level.

[altimin, 2017-05-31 16:51:24]

lgtm

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

https://codereview.chromium.org/2909293002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:374:
if (GetMainThreadOnly().virtual_time_suspended)
On 2017/05/31 16:48:48, alex clarke wrote:
> On 2017/05/31 15:11:36, alex clarke wrote:
> > On 2017/05/31 11:44:06, altimin wrote:
> > > Is virtual_time_expired a better name?
> > 
> > virtual_time_paused is possibly the most accurate name.
> 
> I understand Alexander has a strong preference for _expired due to the budget
> however I respectfully disagree. This state is entered when
> WebViewSchedulerImpl::allow_virtual_time_to_advance_ is false and
> WebViewSchedulerImpl::virtual_time_ is true.  That only happens when we are in
> VirtualTimePolicy::PAUSE (hence my preferred name).
> 
> Now it is true that the layer above sets the policy to
VirtualTimePolicy::PAUSE
> when the virtual time budget expires, but that concept doesn't exist at this
> level.

Yes, it makes sense, thanks for the explanation.

[dgozman, 2017-05-31 17:04:10]

devtools lgtm

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

The CQ bit was checked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2017-06-01 08:03:39]

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/2909293002/#ps60001
(title: "Changes for altimin@")

[commit-bot: I haz the power, 2017-06-01 08:04:20]

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-06-01 08:08:10]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496304218915160,
"parent_rev": "0caff95cfdcd6a8c2ce6d4a2f16dbbece1763072", "commit_rev":
"255feb99a2941caa310d02b0af24e1936533021c"}

[commit-bot: I haz the power, 2017-06-01 08:08:21]

Description was changed from

==========
Allow tasks scheduled for the instant virtual time budges expires to run

It feels strange that if a task is due to run at time t=1.0 and we gave
1s of virtual time budget that that task doesn't run.  This patch changes
that so the task will run by using a fence instead of disabling the queue.

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

to

==========
Allow tasks scheduled for the instant virtual time budges expires to run

It feels strange that if a task is due to run at time t=1.0 and we gave
1s of virtual time budget that that task doesn't run.  This patch changes
that so the task will run by using a fence instead of disabling the queue.

BUG=696001

Review-Url: https://codereview.chromium.org/2909293002
Cr-Commit-Position: refs/heads/master@{#476225}
Committed:
https://chromium.googlesource.com/chromium/src/+/255feb99a2941caa310d02b0af24...
==========

[commit-bot: I haz the power, 2017-06-01 08:08:23]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/255feb99a2941caa310d02b0af24...