Optimize away updatable_queue_set_

Optimize away updatable_queue_set_

Previously TimeDomain::UpdateWorkQueues would scan updatable_queue_set_
every time, but we don't need to do this if
WorkQueue::TakeTaskFromWorkQueue reloads the queue when it becomes
empty.

Was origionally part of https://codereview.chromium.org/2546423002/

BUG=578176
Review-Url: https://codereview.chromium.org/2640763003
Cr-Commit-Position: refs/heads/master@{#444739}
Committed: https://chromium.googlesource.com/chromium/src/+/61a1c6769f03b31d349ca19a44a295da6a2e58f9

[alex clarke (OOO till 29th), 2017-01-18 10:57:09]

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

[commit-bot: I haz the power, 2017-01-18 10:57:21]

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-18 10:57:23]

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

[commit-bot: I haz the power, 2017-01-18 12:38:04]

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

[commit-bot: I haz the power, 2017-01-18 12:38:04]

Dry run: This issue passed the CQ dry run.

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

PTAL

[Sami, 2017-01-18 17:57:04]

Seems like a nice simplification.

https://codereview.chromium.org/2640763003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h
(right):

https://codereview.chromium.org/2640763003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h:143: //
Returns the current queue of immediate incoming tasks.
nit: Something like "Extracts all the tasks from the immediate incoming queue
and clears it." to make the side effect more obvious? Also, can be called from
any thread?

Does this need to be public, by the way? WorkQueue is a friend.

https://codereview.chromium.org/2640763003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scheduler/base/time_domain.cc (right):

https://codereview.chromium.org/2640763003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/base/time_domain.cc:57:
destination_time_domain->has_incoming_immediate_work_.insert(queue);
Looks like we need to grab the destination lock here. Could add insert/erase
helpers that handle locking for this too if you prefer.

https://codereview.chromium.org/2640763003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scheduler/base/time_domain.h (right):

https://codereview.chromium.org/2640763003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/base/time_domain.h:91: // Adds
|queue| to the set of task queues that UpdateWorkQueues calls
This comment looks out of date now.

Also, could you mention that this can be called on any thread?

https://codereview.chromium.org/2640763003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/base/time_domain.h:165: // Set of
task queues with avaliable work on the incoming queue.  This should
Is the second sentence still valid? If it's main-thread only, then it doesn't
need a lock :)

https://codereview.chromium.org/2640763003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scheduler/base/work_queue.cc (right):

https://codereview.chromium.org/2640763003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/base/work_queue.cc:116: work_queue_
= task_queue_->TakeImmediateIncomingQueue();
Not sure why we're not calling OnPushQueue here? Could we just call
ReloadEmptyImmediateQueue()?

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

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:49:28]

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 08:50:53]

PTAL

https://codereview.chromium.org/2640763003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h
(right):

https://codereview.chromium.org/2640763003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h:143: //
Returns the current queue of immediate incoming tasks.
On 2017/01/18 17:57:04, Sami wrote:
> nit: Something like "Extracts all the tasks from the immediate incoming queue
> and clears it." to make the side effect more obvious? Also, can be called from
> any thread?
> 
> Does this need to be public, by the way? WorkQueue is a friend.

Done.

https://codereview.chromium.org/2640763003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scheduler/base/time_domain.cc (right):

https://codereview.chromium.org/2640763003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/base/time_domain.cc:57:
destination_time_domain->has_incoming_immediate_work_.insert(queue);
On 2017/01/18 17:57:04, Sami wrote:
> Looks like we need to grab the destination lock here. Could add insert/erase
> helpers that handle locking for this too if you prefer.

I'd like has_incoming_immediate_work_ to end up on the TaskQueueManager, which
means this code will go away.

https://codereview.chromium.org/2640763003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scheduler/base/time_domain.h (right):

https://codereview.chromium.org/2640763003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/base/time_domain.h:91: // Adds
|queue| to the set of task queues that UpdateWorkQueues calls
On 2017/01/18 17:57:04, Sami wrote:
> This comment looks out of date now.
> 
> Also, could you mention that this can be called on any thread?

Done.

https://codereview.chromium.org/2640763003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/base/time_domain.h:165: // Set of
task queues with avaliable work on the incoming queue.  This should
On 2017/01/18 17:57:04, Sami wrote:
> Is the second sentence still valid? If it's main-thread only, then it doesn't
> need a lock :)

Done.

https://codereview.chromium.org/2640763003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scheduler/base/work_queue.cc (right):

https://codereview.chromium.org/2640763003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/base/work_queue.cc:116: work_queue_
= task_queue_->TakeImmediateIncomingQueue();
On 2017/01/18 17:57:04, Sami wrote:
> Not sure why we're not calling OnPushQueue here? Could we just call
> ReloadEmptyImmediateQueue()?

No, we'd have to do that after OnPopQueue() (because OnPushQueue assumes the
work_queue_ is empty).  It's more efficient to do this.

[commit-bot: I haz the power, 2017-01-19 11:48:33]

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

[commit-bot: I haz the power, 2017-01-19 11:48:34]

Dry run: This issue passed the CQ dry run.

[Sami, 2017-01-19 11:57:38]

https://codereview.chromium.org/2640763003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scheduler/base/time_domain.cc (right):

https://codereview.chromium.org/2640763003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/base/time_domain.cc:57:
destination_time_domain->has_incoming_immediate_work_.insert(queue);
On 2017/01/19 08:50:52, alex clarke wrote:
> On 2017/01/18 17:57:04, Sami wrote:
> > Looks like we need to grab the destination lock here. Could add insert/erase
> > helpers that handle locking for this too if you prefer.
> 
> I'd like has_incoming_immediate_work_ to end up on the TaskQueueManager, which
> means this code will go away. 

That sounds fine, but for the time being don't we need to grab the lock on
|destination_time_domain|?

https://codereview.chromium.org/2640763003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scheduler/base/work_queue.cc (right):

https://codereview.chromium.org/2640763003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/base/work_queue.cc:116: work_queue_
= task_queue_->TakeImmediateIncomingQueue();
On 2017/01/19 08:50:53, alex clarke wrote:
> On 2017/01/18 17:57:04, Sami wrote:
> > Not sure why we're not calling OnPushQueue here? Could we just call
> > ReloadEmptyImmediateQueue()?
> 
> No, we'd have to do that after OnPopQueue() (because OnPushQueue assumes the
> work_queue_ is empty).  It's more efficient to do this.

Okay, I think I understand now: we're short-circuiting the queue reload so that
OnPopQueue does the right thing. Might be worth adding some comment to that
effect here.

(N.B. it feels like OnPushQueue/OnPopQueue could use better names since they
aren't quite symmetric. Maybe something like OnTaskPushedToEmptyQueue and
OnTaskPoppedFromQueue? Up to you.)

[alex clarke (OOO till 29th), 2017-01-19 12:34:20]

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

[alex clarke (OOO till 29th), 2017-01-19 12:34:32]

PTAL

https://codereview.chromium.org/2640763003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scheduler/base/time_domain.cc (right):

https://codereview.chromium.org/2640763003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/base/time_domain.cc:57:
destination_time_domain->has_incoming_immediate_work_.insert(queue);
On 2017/01/19 11:57:38, Sami wrote:
> On 2017/01/19 08:50:52, alex clarke wrote:
> > On 2017/01/18 17:57:04, Sami wrote:
> > > Looks like we need to grab the destination lock here. Could add
insert/erase
> > > helpers that handle locking for this too if you prefer.
> > 
> > I'd like has_incoming_immediate_work_ to end up on the TaskQueueManager,
which
> > means this code will go away. 
> 
> That sounds fine, but for the time being don't we need to grab the lock on
> |destination_time_domain|?

Done.

https://codereview.chromium.org/2640763003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scheduler/base/work_queue.cc (right):

https://codereview.chromium.org/2640763003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/base/work_queue.cc:116: work_queue_
= task_queue_->TakeImmediateIncomingQueue();
On 2017/01/19 11:57:38, Sami wrote:
> On 2017/01/19 08:50:53, alex clarke wrote:
> > On 2017/01/18 17:57:04, Sami wrote:
> > > Not sure why we're not calling OnPushQueue here? Could we just call
> > > ReloadEmptyImmediateQueue()?
> > 
> > No, we'd have to do that after OnPopQueue() (because OnPushQueue assumes the
> > work_queue_ is empty).  It's more efficient to do this.
> 
> Okay, I think I understand now: we're short-circuiting the queue reload so
that
> OnPopQueue does the right thing. Might be worth adding some comment to that
> effect here.
> 
> (N.B. it feels like OnPushQueue/OnPopQueue could use better names since they
> aren't quite symmetric. Maybe something like OnTaskPushedToEmptyQueue and
> OnTaskPoppedFromQueue? Up to you.)

Done.

[alex clarke (OOO till 29th), 2017-01-19 12:34:54]

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

[commit-bot: I haz the power, 2017-01-19 12:35:06]

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 14:44:41]

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

[commit-bot: I haz the power, 2017-01-19 14:44:42]

Dry run: This issue passed the CQ dry run.

[Sami, 2017-01-19 15:28:23]

lgtm, thanks!

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

The CQ bit was checked by alexclarke@chromium.org

[commit-bot: I haz the power, 2017-01-19 15:30:07]

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 15:34:17]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484839790446850,
"parent_rev": "9a44ab189f977553b42b9d7706628b959289308a", "commit_rev":
"61a1c6769f03b31d349ca19a44a295da6a2e58f9"}

[commit-bot: I haz the power, 2017-01-19 15:34:48]

Description was changed from

==========
Optimize away updatable_queue_set_

Previously TimeDomain::UpdateWorkQueues would scan updatable_queue_set_
every time, but we don't need to do this if
WorkQueue::TakeTaskFromWorkQueue reloads the queue when it becomes
empty.

Was origionally part of https://codereview.chromium.org/2546423002/

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

to

==========
Optimize away updatable_queue_set_

Previously TimeDomain::UpdateWorkQueues would scan updatable_queue_set_
every time, but we don't need to do this if
WorkQueue::TakeTaskFromWorkQueue reloads the queue when it becomes
empty.

Was origionally part of https://codereview.chromium.org/2546423002/

BUG=578176

Review-Url: https://codereview.chromium.org/2640763003
Cr-Commit-Position: refs/heads/master@{#444739}
Committed:
https://chromium.googlesource.com/chromium/src/+/61a1c6769f03b31d349ca19a44a2...
==========

[commit-bot: I haz the power, 2017-01-19 15:34:49]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/61a1c6769f03b31d349ca19a44a2...