Refactor TimeDomain::MaybeAdvanceTime in preparation for DoWork deduping

Refactor TimeDomain::MaybeAdvanceTime in preparation for DoWork deduping

Patch https://codereview.chromium.org/2546423002/ is being broken up
into smaller parts.  This patch refactors way DoWork figures out what
kind of continuuation is needed without actually changing policy

BUG=578176
Review-Url: https://codereview.chromium.org/2644553002
Cr-Commit-Position: refs/heads/master@{#444780}
Committed: https://chromium.googlesource.com/chromium/src/+/63aa3c1e6e35803f1e9564ae1e2dd0cf74e58e2e

[alex clarke (OOO till 29th), 2017-01-18 14:43:31]

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

[alex clarke (OOO till 29th), 2017-01-18 14:43:49]

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

[alex clarke (OOO till 29th), 2017-01-18 14:43:50]

PTAL

[commit-bot: I haz the power, 2017-01-18 14:43:50]

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-18 16:36:38]

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

[commit-bot: I haz the power, 2017-01-18 16:36:39]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[alex clarke (OOO till 29th), 2017-01-18 17:32:55]

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

[commit-bot: I haz the power, 2017-01-18 17:33:20]

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

[Sami, 2017-01-18 18:45:58]

https://codereview.chromium.org/2644553002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scheduler/base/real_time_domain.cc
(right):

https://codereview.chromium.org/2644553002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/base/real_time_domain.cc:64: // run
at the right time..
nit: s/.././

https://codereview.chromium.org/2644553002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc
(right):

https://codereview.chromium.org/2644553002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:236:
LazyNow lazy_now(real_time_domain()->CreateLazyNow());
General comment re: |lazy_now|, not sure if it concerns this patch: it's pretty
hard to see what the period of validity of each instance is and to accidentally
hang on to one for too long. I was thinking we might want some debug-only thing
that invalidates all outstanding LazyNows in key places (e.g., when a task is
run) and blows a DCHECK if the LazyNow is held on to across that boundary.

Alternatively we could just simplify things and create one LazyNow per batch
iteration.

https://codereview.chromium.org/2644553002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:327:
*lazy_now = real_time_domain()->CreateLazyNow();
We were assuming that operations not involving running tasks were infinitely
fast, so |lazy_now| was still valid. Do you think we should change that
assumption?

[commit-bot: I haz the power, 2017-01-18 20:45:05]

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

[commit-bot: I haz the power, 2017-01-18 20:45:06]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[alex clarke (OOO till 29th), 2017-01-19 08:56:10]

https://codereview.chromium.org/2644553002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scheduler/base/real_time_domain.cc
(right):

https://codereview.chromium.org/2644553002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/base/real_time_domain.cc:64: // run
at the right time..
On 2017/01/18 18:45:57, Sami wrote:
> nit: s/.././

Done.

https://codereview.chromium.org/2644553002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc
(right):

https://codereview.chromium.org/2644553002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:327:
*lazy_now = real_time_domain()->CreateLazyNow();
On 2017/01/18 18:45:57, Sami wrote:
> We were assuming that operations not involving running tasks were infinitely
> fast, so |lazy_now| was still valid. Do you think we should change that
> assumption?

Yes because task selection takes time.  Constructing a LazyNow is very fast, we
could move this out of the if if you prefer.

[alex clarke (OOO till 29th), 2017-01-19 08:57:02]

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

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

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-19 09:09:30]

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

[commit-bot: I haz the power, 2017-01-19 09:09:31]

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

[alex clarke (OOO till 29th), 2017-01-19 09:30:33]

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

[commit-bot: I haz the power, 2017-01-19 09:30: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-01-19 10:48:40]

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

[commit-bot: I haz the power, 2017-01-19 10:48:53]

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

[Sami, 2017-01-19 12:01:01]

https://codereview.chromium.org/2644553002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc
(right):

https://codereview.chromium.org/2644553002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:327:
*lazy_now = real_time_domain()->CreateLazyNow();
On 2017/01/19 08:56:10, alex clarke wrote:
> On 2017/01/18 18:45:57, Sami wrote:
> > We were assuming that operations not involving running tasks were infinitely
> > fast, so |lazy_now| was still valid. Do you think we should change that
> > assumption?
> 
> Yes because task selection takes time.  Constructing a LazyNow is very fast,
we
> could move this out of the if if you prefer.

Yeah, actually I think we could just create one at the beginning of the loop
where we're calling this and not rely on ProcessTaskFromWorkQueue keeping it up
to date. That way it's really clear when we sample the time.

[Sami, 2017-01-19 12:10:15]

https://codereview.chromium.org/2644553002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc
(right):

https://codereview.chromium.org/2644553002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:327:
*lazy_now = real_time_domain()->CreateLazyNow();
On 2017/01/19 12:01:00, Sami wrote:
> On 2017/01/19 08:56:10, alex clarke wrote:
> > On 2017/01/18 18:45:57, Sami wrote:
> > > We were assuming that operations not involving running tasks were
infinitely
> > > fast, so |lazy_now| was still valid. Do you think we should change that
> > > assumption?
> > 
> > Yes because task selection takes time.  Constructing a LazyNow is very fast,
> we
> > could move this out of the if if you prefer.
> 
> Yeah, actually I think we could just create one at the beginning of the loop
> where we're calling this and not rely on ProcessTaskFromWorkQueue keeping it
up
> to date. That way it's really clear when we sample the time.

Offline: turns out we can't because of time observers. Decided to split this
into two different lazy nows: lazy_now_before_task and lazy_now_after_task.

[commit-bot: I haz the power, 2017-01-19 12:18:29]

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

[commit-bot: I haz the power, 2017-01-19 12:18:30]

Dry run: This issue passed the CQ dry run.

[alex clarke (OOO till 29th), 2017-01-19 14:25:02]

https://codereview.chromium.org/2644553002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc
(right):

https://codereview.chromium.org/2644553002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:327:
*lazy_now = real_time_domain()->CreateLazyNow();
On 2017/01/19 12:10:15, Sami wrote:
> On 2017/01/19 12:01:00, Sami wrote:
> > On 2017/01/19 08:56:10, alex clarke wrote:
> > > On 2017/01/18 18:45:57, Sami wrote:
> > > > We were assuming that operations not involving running tasks were
> infinitely
> > > > fast, so |lazy_now| was still valid. Do you think we should change that
> > > > assumption?
> > > 
> > > Yes because task selection takes time.  Constructing a LazyNow is very
fast,
> > we
> > > could move this out of the if if you prefer.
> > 
> > Yeah, actually I think we could just create one at the beginning of the loop
> > where we're calling this and not rely on ProcessTaskFromWorkQueue keeping it
> up
> > to date. That way it's really clear when we sample the time.
> 
> Offline: turns out we can't because of time observers. Decided to split this
> into two different lazy nows: lazy_now_before_task and lazy_now_after_task.

Done.

[Sami, 2017-01-19 15:22:29]

lgtm, thanks for simplifying the use of LazyNow!

https://codereview.chromium.org/2644553002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/renderer/throttled_time_domain.cc
(right):

https://codereview.chromium.org/2644553002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/renderer/throttled_time_domain.cc:36:
// Unlike RealTimeDomain::ContinuationNeeded we don't request a wake up here,
ContinuationNeeded doesn't seem to exist?

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

Thanks

https://codereview.chromium.org/2644553002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/renderer/throttled_time_domain.cc
(right):

https://codereview.chromium.org/2644553002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/renderer/throttled_time_domain.cc:36:
// Unlike RealTimeDomain::ContinuationNeeded we don't request a wake up here,
On 2017/01/19 15:22:29, Sami wrote:
> ContinuationNeeded doesn't seem to exist?

Fixed the comment.

[alex clarke (OOO till 29th), 2017-01-19 15:51:08]

The CQ bit was checked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2017-01-19 15:51:09]

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/2644553002/#ps120001
(title: "Rebased")

[commit-bot: I haz the power, 2017-01-19 15:51:27]

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-19 17:55:51]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1484841068882710,
"parent_rev": "5f7ae5f82dca5cc06ccc068b7471ee5123163ff2", "commit_rev":
"63aa3c1e6e35803f1e9564ae1e2dd0cf74e58e2e"}

[commit-bot: I haz the power, 2017-01-19 17:56:25]

Description was changed from

==========
Refactor TimeDomain::MaybeAdvanceTime in preparation for DoWork deduping

Patch https://codereview.chromium.org/2546423002/ is being broken up
into smaller parts.  This patch refactors way DoWork figures out what
kind of continuuation is needed without actually changing policy

BUG=578176
==========

to

==========
Refactor TimeDomain::MaybeAdvanceTime in preparation for DoWork deduping

Patch https://codereview.chromium.org/2546423002/ is being broken up
into smaller parts.  This patch refactors way DoWork figures out what
kind of continuuation is needed without actually changing policy

BUG=578176

Review-Url: https://codereview.chromium.org/2644553002
Cr-Commit-Position: refs/heads/master@{#444780}
Committed:
https://chromium.googlesource.com/chromium/src/+/63aa3c1e6e35803f1e9564ae1e2d...
==========

[commit-bot: I haz the power, 2017-01-19 17:56:27]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/63aa3c1e6e35803f1e9564ae1e2d...