[scheduler] Add options to TaskQueue::InsertFence

[scheduler] Add options to TaskQueue::InsertFence

Make TaskQueue::InsertFence configurable and allow to insert a fence
in the beginning for the TaskQueue, effectively disabling it.
It will allow TaskQueueThrottler to use only fences without disabling
and enabling queues.

R=alexclarke@chromium.org,skyostil@chromium.org
BUG=638542
Committed: https://crrev.com/d15597b8f924879440a66ef0f12f9a939b7c30ea
Cr-Commit-Position: refs/heads/master@{#434542}

[altimin, 2016-11-25 14:51:20]

PTAL

[alex clarke (OOO till 29th), 2016-11-25 15:06:39]

Overall this looks like it's going to be a really nice simplification :)

https://codereview.chromium.org/2533603002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/scheduler/base/task_queue.h (right):

https://codereview.chromium.org/2533603002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/scheduler/base/task_queue.h:180: virtual void
InsertFence() = 0;
sorry for the bike shed, but I'm thinking we should make this take an enum:

enum class InsertFencePosition {
  NOW,
  BEGINNING_OF_TIME
};

Where InsertFence(now) does what InsertFence() currently does and
InsertFence(BEGINNING_OF_TIME) does what your propsoed BlockByFence does.  Sami
and I believe this makes the interface easier to understand.

[altimin, 2016-11-25 15:55:28]

PTAL

https://codereview.chromium.org/2533603002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/scheduler/base/task_queue.h (right):

https://codereview.chromium.org/2533603002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/scheduler/base/task_queue.h:180: virtual void
InsertFence() = 0;
On 2016/11/25 15:06:38, alex clarke wrote:
> sorry for the bike shed, but I'm thinking we should make this take an enum:
> 
> enum class InsertFencePosition {
>   NOW,
>   BEGINNING_OF_TIME
> };
> 
> Where InsertFence(now) does what InsertFence() currently does and
> InsertFence(BEGINNING_OF_TIME) does what your propsoed BlockByFence does. 
Sami
> and I believe this makes the interface easier to understand.

Done.

[altimin, 2016-11-25 16:03:36]

Description was changed from

==========
[scheduler] Add TaskQueue::BlockByFence method.

Add a method to insert fence in the beginning of TaskQueue and
effectively block it. It will allow TaskQueueThrottler to
use only fences without disabling and enabling queues.

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

to

==========
[scheduler] Add options to TaskQueue::InsertFence

Make TaskQueue::InsertFence configurable and allow to insert a fence
in the beginning for the TaskQueue, effectively disabling it.
It will allow TaskQueueThrottler to use only fences without disabling
and enabling queues.

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

[alex clarke (OOO till 29th), 2016-11-25 16:20:50]

https://codereview.chromium.org/2533603002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h
(right):

https://codereview.chromium.org/2533603002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h:198: bool
IsQueueUnblockedForTest() const;
minor: I'd slightly prefer a GetFenceForTest() and implement this as a helper in
the test.

Reason being a queue could be blocked by a fence with an value other than
BLOCKING_FENCE.

https://codereview.chromium.org/2533603002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scheduler/child/idle_helper.cc (right):

https://codereview.chromium.org/2533603002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/child/idle_helper.cc:50:
idle_queue_->InsertFence(TaskQueue::InsertFencePosition::BEGINNING_OF_TIME);
Please add a comment explaining this blocks any idle tasks from running.

https://codereview.chromium.org/2533603002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/child/idle_helper.cc:208:
idle_queue_->InsertFence(TaskQueue::InsertFencePosition::BEGINNING_OF_TIME);
Please add a comment explaining this blocks any idle tasks from running.

https://codereview.chromium.org/2533603002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc
(right):

https://codereview.chromium.org/2533603002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:331:
task_queue->InsertFence(TaskQueue::InsertFencePosition::BEGINNING_OF_TIME);
Please add a comment explaining that we need to stop any tasks posted to
|task_queue| immediately.

https://codereview.chromium.org/2533603002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:452:
task_queue->InsertFence(TaskQueue::InsertFencePosition::NOW);
Please add a comment saying this will allow tasks posted on |task_queue| up
until this point, but no further to run.

https://codereview.chromium.org/2533603002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:629:
queue->InsertFence(TaskQueue::InsertFencePosition::BEGINNING_OF_TIME);
Please add a comment explaining that we need to stop any tasks posted to
|queue| immediately.

https://codereview.chromium.org/2533603002/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/scheduler/base/task_queue.h (right):

https://codereview.chromium.org/2533603002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/scheduler/base/task_queue.h:176: NOW,
I'm thinking ahead in case we ever add a third value so I'd suggest changing the
comments like so:

NOW,  // Tasks posted on the queue up till this point, and no further may run.

BEGINNING_OF_TIME,  // No tasks posted on this queue may run.

// Inserts a barrier into the task queue which prevents tasks with an enqueue
order
// greater the fence from running until either the fence has been removed or a
// subsequent fence has unblocked some tasks within the queue.
// Note delayed tasks, get their enqueue order set once their delay has expired,
// and non-delayed tasks get their enqueue order set when posted.

[altimin, 2016-11-25 16:59:26]

PTAL

https://codereview.chromium.org/2533603002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h
(right):

https://codereview.chromium.org/2533603002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h:198: bool
IsQueueUnblockedForTest() const;
On 2016/11/25 16:20:49, alex clarke wrote:
> minor: I'd slightly prefer a GetFenceForTest() and implement this as a helper
in
> the test.
> 
> Reason being a queue could be blocked by a fence with an value other than
> BLOCKING_FENCE.

Done.

https://codereview.chromium.org/2533603002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scheduler/child/idle_helper.cc (right):

https://codereview.chromium.org/2533603002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/child/idle_helper.cc:50:
idle_queue_->InsertFence(TaskQueue::InsertFencePosition::BEGINNING_OF_TIME);
On 2016/11/25 16:20:49, alex clarke wrote:
> Please add a comment explaining this blocks any idle tasks from running.

Done.

https://codereview.chromium.org/2533603002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/child/idle_helper.cc:208:
idle_queue_->InsertFence(TaskQueue::InsertFencePosition::BEGINNING_OF_TIME);
On 2016/11/25 16:20:49, alex clarke wrote:
> Please add a comment explaining this blocks any idle tasks from running.

Done.

https://codereview.chromium.org/2533603002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc
(right):

https://codereview.chromium.org/2533603002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:331:
task_queue->InsertFence(TaskQueue::InsertFencePosition::BEGINNING_OF_TIME);
On 2016/11/25 16:20:49, alex clarke wrote:
> Please add a comment explaining that we need to stop any tasks posted to
> |task_queue| immediately.

Done.

https://codereview.chromium.org/2533603002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:452:
task_queue->InsertFence(TaskQueue::InsertFencePosition::NOW);
On 2016/11/25 16:20:49, alex clarke wrote:
> Please add a comment saying this will allow tasks posted on |task_queue| up
> until this point, but no further to run.

Done.

https://codereview.chromium.org/2533603002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:629:
queue->InsertFence(TaskQueue::InsertFencePosition::BEGINNING_OF_TIME);
On 2016/11/25 16:20:49, alex clarke wrote:
> Please add a comment explaining that we need to stop any tasks posted to
> |queue| immediately.

Done.

https://codereview.chromium.org/2533603002/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/scheduler/base/task_queue.h (right):

https://codereview.chromium.org/2533603002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/scheduler/base/task_queue.h:176: NOW,
On 2016/11/25 16:20:49, alex clarke wrote:
> I'm thinking ahead in case we ever add a third value so I'd suggest changing
the
> comments like so:
> 
> NOW,  // Tasks posted on the queue up till this point, and no further may run.
> 
> BEGINNING_OF_TIME,  // No tasks posted on this queue may run.
> 
> // Inserts a barrier into the task queue which prevents tasks with an enqueue
> order
> // greater the fence from running until either the fence has been removed or a
> // subsequent fence has unblocked some tasks within the queue.
> // Note delayed tasks, get their enqueue order set once their delay has
expired,
> // and non-delayed tasks get their enqueue order set when posted.

Done.

[altimin, 2016-11-25 17:00:37]

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

[commit-bot: I haz the power, 2016-11-25 17:00:46]

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), 2016-11-25 17:04:12]

lgtm

[Sami, 2016-11-25 17:12:59]

Thanks for cleaning this up!

https://codereview.chromium.org/2533603002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scheduler/base/enqueue_order.cc (right):

https://codereview.chromium.org/2533603002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/base/enqueue_order.cc:11: // Note
we set the first |enqueue_order_| to 1 so that isValid may distinguish a
Looks like this comment needs updating.

https://codereview.chromium.org/2533603002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scheduler/base/enqueue_order.h (right):

https://codereview.chromium.org/2533603002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/base/enqueue_order.h:18: enum class
EnqueueOrderValues {
nit: enum class EnqueueOrderValues : EnqueueOrder { ... } to avoid the cast
(might be c++17 tho)

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

https://codereview.chromium.org/2533603002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:343:
TRACE_EVENT1(tracing_category_, "TaskQueueThrottler_TaskQueueThrottled",
Could you keep this at the top of the function so that any other events emitted
by function calls are logically ordered?

[Sami, 2016-11-25 17:13:21]

Oh and lgtm with the above nits.

[altimin, 2016-11-25 17:29:06]

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

[commit-bot: I haz the power, 2016-11-25 17:29:15]

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

[Sami, 2016-11-25 17:38:43]

Still lgtm.

https://codereview.chromium.org/2533603002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scheduler/base/enqueue_order.cc (right):

https://codereview.chromium.org/2533603002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/base/enqueue_order.cc:11: // Note
we set the first |enqueue_order_| to a non-zero value, because
nit: to a specific non-zero value

https://codereview.chromium.org/2533603002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scheduler/base/enqueue_order.h (right):

https://codereview.chromium.org/2533603002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/base/enqueue_order.h:18: //
TODO(scheduler-dev): Remove explicit casts when c++17 comes.
Might wanna move this next to the actual cast?

[altimin, 2016-11-25 17:44:10]

PTAL

https://codereview.chromium.org/2533603002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scheduler/base/enqueue_order.cc (right):

https://codereview.chromium.org/2533603002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/base/enqueue_order.cc:11: // Note
we set the first |enqueue_order_| to 1 so that isValid may distinguish a
On 2016/11/25 17:12:59, Sami wrote:
> Looks like this comment needs updating.

Done.

https://codereview.chromium.org/2533603002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scheduler/base/enqueue_order.h (right):

https://codereview.chromium.org/2533603002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/base/enqueue_order.h:18: enum class
EnqueueOrderValues {
On 2016/11/25 17:12:59, Sami wrote:
> nit: enum class EnqueueOrderValues : EnqueueOrder { ... } to avoid the cast
> (might be c++17 tho)

Done.

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

https://codereview.chromium.org/2533603002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:343:
TRACE_EVENT1(tracing_category_, "TaskQueueThrottler_TaskQueueThrottled",
On 2016/11/25 17:12:59, Sami wrote:
> Could you keep this at the top of the function so that any other events
emitted
> by function calls are logically ordered?

I want to emit this only when we are actually throttling queue, not just
increasing refcount. But I refactored it a bit.

https://codereview.chromium.org/2533603002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scheduler/base/enqueue_order.cc (right):

https://codereview.chromium.org/2533603002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/base/enqueue_order.cc:11: // Note
we set the first |enqueue_order_| to a non-zero value, because
On 2016/11/25 17:38:43, Sami wrote:
> nit: to a specific non-zero value

Done.

https://codereview.chromium.org/2533603002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scheduler/base/enqueue_order.h (right):

https://codereview.chromium.org/2533603002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/base/enqueue_order.h:18: //
TODO(scheduler-dev): Remove explicit casts when c++17 comes.
On 2016/11/25 17:38:43, Sami wrote:
> Might wanna move this next to the actual cast?

[altimin, 2016-11-25 17:44:56]

The CQ bit was checked by altimin@chromium.org

[altimin, 2016-11-25 17:44:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
alexclarke@chromium.org, skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2533603002/#ps130001
(title: "More nits")

[commit-bot: I haz the power, 2016-11-25 17:45:04]

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, 2016-11-25 19:30:11]

CQ is committing da patch.

Bot data: {"patchset_id": 130001, "attempt_start_ts": 1480095896439660,
"parent_rev": "502aace4f83d1bbea1d2bc6cd307571eed5feee3", "commit_rev":
"6e861aaff19edfec5221cab02eedb49654c00624"}

[commit-bot: I haz the power, 2016-11-25 19:30:34]

Description was changed from

==========
[scheduler] Add options to TaskQueue::InsertFence

Make TaskQueue::InsertFence configurable and allow to insert a fence
in the beginning for the TaskQueue, effectively disabling it.
It will allow TaskQueueThrottler to use only fences without disabling
and enabling queues.

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

to

==========
[scheduler] Add options to TaskQueue::InsertFence

Make TaskQueue::InsertFence configurable and allow to insert a fence
in the beginning for the TaskQueue, effectively disabling it.
It will allow TaskQueueThrottler to use only fences without disabling
and enabling queues.

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

[commit-bot: I haz the power, 2016-11-25 19:30:35]

Committed patchset #8 (id:130001)

[commit-bot: I haz the power, 2016-11-25 19:32:33]

Description was changed from

==========
[scheduler] Add options to TaskQueue::InsertFence

Make TaskQueue::InsertFence configurable and allow to insert a fence
in the beginning for the TaskQueue, effectively disabling it.
It will allow TaskQueueThrottler to use only fences without disabling
and enabling queues.

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

to

==========
[scheduler] Add options to TaskQueue::InsertFence

Make TaskQueue::InsertFence configurable and allow to insert a fence
in the beginning for the TaskQueue, effectively disabling it.
It will allow TaskQueueThrottler to use only fences without disabling
and enabling queues.

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

Committed: https://crrev.com/d15597b8f924879440a66ef0f12f9a939b7c30ea
Cr-Commit-Position: refs/heads/master@{#434542}
==========

[commit-bot: I haz the power, 2016-11-25 19:32:34]

Patchset 8 (id:??) landed as
https://crrev.com/d15597b8f924879440a66ef0f12f9a939b7c30ea
Cr-Commit-Position: refs/heads/master@{#434542}