[scheduler] Move TimeBudgetPool to a separate file.

[scheduler] Move TimeBudgetPool to a separate file.

This is one of the few CLs refactoring budget pool mechanisms for the
addition of the new WakeupBudgetPool.

- Move TimeBudgetPool into budget_pool.h to keep task_queue_throttler
size manageable.
- Split TimeBudgetPool into BudgetPool (interface) and TimeBudgetPool
(realisation).

There will be a separate CL uncoupling TaskQueueThrottler from BudgetPool.

R=alexclarke@chromium.org,skyostil@chromium.org

BUG=699541
Review-Url: https://codereview.chromium.org/2741473002
Cr-Commit-Position: refs/heads/master@{#455747}
Committed: https://chromium.googlesource.com/chromium/src/+/e6d316b45c718f370db99f9a319646be8f892aba

[altimin, 2017-03-08 15:19:22]

PTAL

[Sami, 2017-03-08 16:18:01]

Seems like a good refactoring.

https://codereview.chromium.org/2741473002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc
(right):

https://codereview.chromium.org/2741473002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc:37:
TimeBudgetPool::TimeBudgetPool(
Could we extract some unit tests for this too? (I assume the wake-up pool will
need some tests too.)

https://codereview.chromium.org/2741473002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc:104:
"TaskQueueThrottler_TimeBudgetPool_EnableThrottling");
Please remove TaskQueueThrottler from these trace events.

https://codereview.chromium.org/2741473002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h
(right):

https://codereview.chromium.org/2741473002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h:34:
~BudgetPool();
virtual

https://codereview.chromium.org/2741473002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h:75:
base::TimeTicks now) const = 0;
Is this formatted right?

https://codereview.chromium.org/2741473002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h:113: void
AsValueInto(base::trace_event::TracedValue* state, base::TimeTicks now) const
override;
Line too long?

https://codereview.chromium.org/2741473002/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h
(right):

https://codereview.chromium.org/2741473002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:113:
friend class BudgetPool;
I guess these will be removed later?

[altimin, 2017-03-08 16:50:19]

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

[altimin, 2017-03-08 16:50:33]

PTAL

https://codereview.chromium.org/2741473002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc
(right):

https://codereview.chromium.org/2741473002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc:37:
TimeBudgetPool::TimeBudgetPool(
On 2017/03/08 16:18:01, Sami wrote:
> Could we extract some unit tests for this too? (I assume the wake-up pool will
> need some tests too.)

Done.

https://codereview.chromium.org/2741473002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc:104:
"TaskQueueThrottler_TimeBudgetPool_EnableThrottling");
On 2017/03/08 16:18:00, Sami wrote:
> Please remove TaskQueueThrottler from these trace events.

Done.

https://codereview.chromium.org/2741473002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h
(right):

https://codereview.chromium.org/2741473002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h:34:
~BudgetPool();
On 2017/03/08 16:18:01, Sami wrote:
> virtual

Shame. Shame. Shame.

https://codereview.chromium.org/2741473002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h:75:
base::TimeTicks now) const = 0;
On 2017/03/08 16:18:01, Sami wrote:
> Is this formatted right?

Done.

https://codereview.chromium.org/2741473002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h:113: void
AsValueInto(base::trace_event::TracedValue* state, base::TimeTicks now) const
override;
On 2017/03/08 16:18:01, Sami wrote:
> Line too long?

Done.

https://codereview.chromium.org/2741473002/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h
(right):

https://codereview.chromium.org/2741473002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:113:
friend class BudgetPool;
On 2017/03/08 16:18:01, Sami wrote:
> I guess these will be removed later?

Yep, the comment in the patch description.

[commit-bot: I haz the power, 2017-03-08 16:50:45]

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-03-08 16:57:43]

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

[commit-bot: I haz the power, 2017-03-08 16:57:44]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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-03-08 17:19:42]

lgtm with a minor tweak, thanks.

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

https://codereview.chromium.org/2741473002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/budget_pool_unittest.cc:23:
#include "platform/scheduler/renderer/web_frame_scheduler_impl.h"
Some of these includes look unnecessary -- could you double-check?

[altimin, 2017-03-09 12:28:46]

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

[altimin, 2017-03-09 12:28:47]

PTAL

+haraken@ for platform/BUILD.gn

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

https://codereview.chromium.org/2741473002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/budget_pool_unittest.cc:23:
#include "platform/scheduler/renderer/web_frame_scheduler_impl.h"
On 2017/03/08 17:19:42, Sami wrote:
> Some of these includes look unnecessary -- could you double-check?

Done.

[altimin, 2017-03-09 12:28:52]

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

[commit-bot: I haz the power, 2017-03-09 12:29:05]

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-09 12:33:06]

https://codereview.chromium.org/2741473002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h
(right):

https://codereview.chromium.org/2741473002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h:30: // on a
resource. This limit applies when task queues are already throttled
Maybe mention that the resource is measured in time?

https://codereview.chromium.org/2741473002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h:68: virtual
bool HasEnoughBudgetToRun(base::TimeTicks now) = 0;
Does this need to be virtual? Could it be:

bool HasEnoughBudgetToRun(base::TimeTicks now) const {
  return now >= GetNextAllowedRunTime();
}

https://codereview.chromium.org/2741473002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h:75:
base::TimeTicks now) const = 0;
nit: indent looks odd.

https://codereview.chromium.org/2741473002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h:80: class
TimeBudgetPool : public BudgetPool {
bike-shed: CpuTimeBudgetPool?  I was wondering why BudgetPool knew about times
when there was a TimeBidget pool.

[altimin, 2017-03-09 12:50:34]

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

[commit-bot: I haz the power, 2017-03-09 12:50:46]

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

[haraken, 2017-03-09 12:54:04]

BUILD.gn LGTM

[altimin, 2017-03-09 13:36:02]

https://codereview.chromium.org/2741473002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h
(right):

https://codereview.chromium.org/2741473002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h:68: virtual
bool HasEnoughBudgetToRun(base::TimeTicks now) = 0;
On 2017/03/09 12:33:05, alex clarke wrote:
> Does this need to be virtual? Could it be:
> 
> bool HasEnoughBudgetToRun(base::TimeTicks now) const {
>   return now >= GetNextAllowedRunTime();
> }

At this moment HasEnoughBudgetToRun updates last known time. It may be worth
changing, but I don't want to do it in this patch.

https://codereview.chromium.org/2741473002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h:80: class
TimeBudgetPool : public BudgetPool {
On 2017/03/09 12:33:05, alex clarke wrote:
> bike-shed: CpuTimeBudgetPool?  I was wondering why BudgetPool knew about times
> when there was a TimeBidget pool.

Done.

[commit-bot: I haz the power, 2017-03-09 14:31:37]

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

[commit-bot: I haz the power, 2017-03-09 14:31:38]

Dry run: This issue passed the CQ dry run.

[altimin, 2017-03-09 15:05:56]

The CQ bit was checked by altimin@chromium.org

[altimin, 2017-03-09 15:05:57]

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/2741473002/#ps60001
(title: "Addressed comments from alexclarke@")

[commit-bot: I haz the power, 2017-03-09 15:06:14]

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-09 15:10:49]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489071956999370,
"parent_rev": "b45090204d015152986601bb275f55d7064368a5", "commit_rev":
"e6d316b45c718f370db99f9a319646be8f892aba"}

[commit-bot: I haz the power, 2017-03-09 15:11:28]

Description was changed from

==========
[scheduler] Move TimeBudgetPool to a separate file.

This is one of the few CLs refactoring budget pool mechanisms for the
addition of the new WakeupBudgetPool.

- Move TimeBudgetPool into budget_pool.h to keep task_queue_throttler
size manageable.
- Split TimeBudgetPool into BudgetPool (interface) and TimeBudgetPool
(realisation).

There will be a separate CL uncoupling TaskQueueThrottler from BudgetPool.

R=alexclarke@chromium.org,skyostil@chromium.org

BUG=699541
==========

to

==========
[scheduler] Move TimeBudgetPool to a separate file.

This is one of the few CLs refactoring budget pool mechanisms for the
addition of the new WakeupBudgetPool.

- Move TimeBudgetPool into budget_pool.h to keep task_queue_throttler
size manageable.
- Split TimeBudgetPool into BudgetPool (interface) and TimeBudgetPool
(realisation).

There will be a separate CL uncoupling TaskQueueThrottler from BudgetPool.

R=alexclarke@chromium.org,skyostil@chromium.org

BUG=699541

Review-Url: https://codereview.chromium.org/2741473002
Cr-Commit-Position: refs/heads/master@{#455747}
Committed:
https://chromium.googlesource.com/chromium/src/+/e6d316b45c718f370db99f9a3196...
==========

[commit-bot: I haz the power, 2017-03-09 15:11:29]

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