Move has_incoming_immediate_work to the TaskQueueManager

Move has_incoming_immediate_work to the TaskQueueManager

Reduces the number of locks per DoWork by moving the
has_incoming_immediate_work set to the TaskQueueManager.

Previously TimeDomain::UpdateWorkQueues was called for every
time domain (at least 2 time domains, maybe more in some circumstances)
which would take a lock.  Now we only need the one lock.

Originally part of https://codereview.chromium.org/2546423002/

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

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

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

[alex clarke (OOO till 29th), 2017-01-23 15:25:41]

Description was changed from

==========
Move has_incoming_immediate_work to the TaskQueueManager

Reduces the number of locks per DoWork by moving the
has_incoming_immediate_work set to the TaskQueueManager.

Origionally part of https://codereview.chromium.org/2546423002/

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

to

==========
Move has_incoming_immediate_work to the TaskQueueManager

Reduces the number of locks per DoWork by moving the
has_incoming_immediate_work set to the TaskQueueManager.

Previously TimeDomain::UpdateWorkQueues was called for every
time domain (at least 2 time domains, maybe more in some circumstances)
which would take a lock.  Now we only need the one lock.

Origionally part of https://codereview.chromium.org/2546423002/

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

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

Description was changed from

==========
Move has_incoming_immediate_work to the TaskQueueManager

Reduces the number of locks per DoWork by moving the
has_incoming_immediate_work set to the TaskQueueManager.

Previously TimeDomain::UpdateWorkQueues was called for every
time domain (at least 2 time domains, maybe more in some circumstances)
which would take a lock.  Now we only need the one lock.

Origionally part of https://codereview.chromium.org/2546423002/

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

to

==========
Move has_incoming_immediate_work to the TaskQueueManager

Reduces the number of locks per DoWork by moving the
has_incoming_immediate_work set to the TaskQueueManager.

Previously TimeDomain::UpdateWorkQueues was called for every
time domain (at least 2 time domains, maybe more in some circumstances)
which would take a lock.  Now we only need the one lock.

Originally part of https://codereview.chromium.org/2546423002/

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

[alex clarke (OOO till 29th), 2017-01-23 15:35:11]

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

[alex clarke (OOO till 29th), 2017-01-24 08:35:04]

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

[commit-bot: I haz the power, 2017-01-24 08:35:17]

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-24 09:11:38]

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

[commit-bot: I haz the power, 2017-01-24 09:11:49]

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-24 11:12:25]

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

[commit-bot: I haz the power, 2017-01-24 11:12:26]

Dry run: This issue passed the CQ dry run.

[Sami, 2017-01-24 14:52:41]

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

https://codereview.chromium.org/2653643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:335: bool
ensure_do_work_posted =
To separate the concerns a bit, how about calling this something like
|can_run_immediately|? (Alternatively the inverse of that since that's really
the only thing we can guarantee here, but I can't think of a good name for it.)

Should we also DCHECK() that ImmediateTaskCouldRun is consistent with what we
compute here?

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

https://codereview.chromium.org/2653643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:161: //
case). Secondly
Secondly...? :)

https://codereview.chromium.org/2653643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:289:
std::set<internal::TaskQueueImpl*> queues_to_reload;
Probably doesn't make a huge difference, but unordered_set?

https://codereview.chromium.org/2653643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:357:
base::AutoLock lock(any_thread_lock_);
Any lock inversion issues here? Basically I think we need to make sure
any_thread_lock_ and the per-queue-locks aren't held at the same time if
possible.

[alex clarke (OOO till 29th), 2017-01-24 15:28:14]

PTAL

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

https://codereview.chromium.org/2653643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:335: bool
ensure_do_work_posted =
On 2017/01/24 14:52:40, Sami wrote:
> To separate the concerns a bit, how about calling this something like
> |can_run_immediately|? (Alternatively the inverse of that since that's really
> the only thing we can guarantee here, but I can't think of a good name for
it.)

I suppose task_currently_blocked?

> 
> Should we also DCHECK() that ImmediateTaskCouldRun is consistent with what we
> compute here?

Well ImmediateTaskCouldRun has gone away so a DCHECK doesn't make sense.

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

https://codereview.chromium.org/2653643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:161: //
case). Secondly
On 2017/01/24 14:52:40, Sami wrote:
> Secondly...? :)

Done.

https://codereview.chromium.org/2653643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:289:
std::set<internal::TaskQueueImpl*> queues_to_reload;
On 2017/01/24 14:52:40, Sami wrote:
> Probably doesn't make a huge difference, but unordered_set?

I guess.

https://codereview.chromium.org/2653643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:357:
base::AutoLock lock(any_thread_lock_);
On 2017/01/24 14:52:40, Sami wrote:
> Any lock inversion issues here? Basically I think we need to make sure
> any_thread_lock_ and the per-queue-locks aren't held at the same time if
> possible.

Yeah actually that could be one.  Can probably get the majority of the benefit
of this by asserting it doesn't have a fence and it's not disabled. Neither of
which need a lock.

[alex clarke (OOO till 29th), 2017-01-24 15:28:23]

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

[commit-bot: I haz the power, 2017-01-24 15:28:32]

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-24 15:29:11]

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

https://codereview.chromium.org/2653643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:357:
base::AutoLock lock(any_thread_lock_);
On 2017/01/24 15:28:14, alex clarke wrote:
> On 2017/01/24 14:52:40, Sami wrote:
> > Any lock inversion issues here? Basically I think we need to make sure
> > any_thread_lock_ and the per-queue-locks aren't held at the same time if
> > possible.
> 
> Yeah actually that could be one.  Can probably get the majority of the benefit
> of this by asserting it doesn't have a fence and it's not disabled. Neither of
> which need a lock.

Actually on second thoughts I think I'm trying to be too clever here.  I'll call
ReloadEmptyWorkQueues.

[commit-bot: I haz the power, 2017-01-24 15:32:16]

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

[commit-bot: I haz the power, 2017-01-24 15:32:17]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  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-01-24 16:54:40]

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

[commit-bot: I haz the power, 2017-01-24 16:54:51]

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-24 17:43:49]

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

[commit-bot: I haz the power, 2017-01-24 17:43:50]

Dry run: This issue passed the CQ dry run.

[Sami, 2017-01-24 18:23:02]

lgtm, thanks!

[alex clarke (OOO till 29th), 2017-01-24 19:04:41]

The CQ bit was checked by alexclarke@chromium.org

[commit-bot: I haz the power, 2017-01-24 19:05:16]

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-24 19:11:12]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485284681226530,
"parent_rev": "1fc58f0e3b68581b5832176bfd922bc28ecb7b54", "commit_rev":
"cdf5b1fbdbfed58d0a0450597d3666c5a2159c30"}

[commit-bot: I haz the power, 2017-01-24 19:11:43]

Description was changed from

==========
Move has_incoming_immediate_work to the TaskQueueManager

Reduces the number of locks per DoWork by moving the
has_incoming_immediate_work set to the TaskQueueManager.

Previously TimeDomain::UpdateWorkQueues was called for every
time domain (at least 2 time domains, maybe more in some circumstances)
which would take a lock.  Now we only need the one lock.

Originally part of https://codereview.chromium.org/2546423002/

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

to

==========
Move has_incoming_immediate_work to the TaskQueueManager

Reduces the number of locks per DoWork by moving the
has_incoming_immediate_work set to the TaskQueueManager.

Previously TimeDomain::UpdateWorkQueues was called for every
time domain (at least 2 time domains, maybe more in some circumstances)
which would take a lock.  Now we only need the one lock.

Originally part of https://codereview.chromium.org/2546423002/

BUG=578176

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

[commit-bot: I haz the power, 2017-01-24 19:11:44]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/cdf5b1fbdbfed58d0a0450597d36...