[scheduler] Fix bug in task queue throttler

[scheduler] Account for disabling throttling in TaskQueueThrottler::IsThrottled

TaskQueueThrottler::IsThrottled didn't respect global on/off throttling
switch. This led to TaskQueueThrottler::TimeBudgetPool::EnableThrottling
permanently disabling some queues.

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

BUG=639852
Committed: https://crrev.com/8991961c94284b66c45ef222bdd3f787327a3c81
Cr-Commit-Position: refs/heads/master@{#434195}

[altimin, 2016-11-22 18:53:03]

PTAL

[altimin, 2016-11-22 18:55:45]

PTAL

[Sami, 2016-11-22 19:27:35]

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

https://codereview.chromium.org/2523673003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:336:
std::make_pair(task_queue, Metadata(0, task_queue->IsQueueEnabled())));
0 /* ref_count */  (I had to look it up)

https://codereview.chromium.org/2523673003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:381:
if (!allow_throttling_)
Is this a necessary change? To me this query says "is queue X managed by the
throttler?" Whether or not throttling is enabled is orthogonal to that.

Maybe we should rename this method and change the call site(s) that depend on
this distinction also check whether throttling is enabled?

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

https://codereview.chromium.org/2523673003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler_unittest.cc:996:
std::vector<base::TimeTicks> run_times;
I think this test could use a narrative summary comment about what exactly is
being tested here.

[Sami, 2016-11-22 19:28:23]

(Also see Primiano's recent email about commit messages :)

[altimin, 2016-11-23 11:05:48]

Description was changed from

==========
[scheduler] Fix bug in task queue throttler

TaskQueueThrottler::IsThrottled didn't respect global on/off throttling
switch. This led to TaskQueueThrottler::TimeBudgetPool::EnableThrottling
permanently disabling some queues.

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

BUG=639852
==========

to

==========
[scheduler] Account for disabling throttling in TaskQueueThrottler::IsThrottled

TaskQueueThrottler::IsThrottled didn't respect global on/off throttling
switch. This led to TaskQueueThrottler::TimeBudgetPool::EnableThrottling
permanently disabling some queues.

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

BUG=639852
==========

[altimin, 2016-11-23 11:11:29]

PTAL

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

https://codereview.chromium.org/2523673003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:336:
std::make_pair(task_queue, Metadata(0, task_queue->IsQueueEnabled())));
On 2016/11/22 19:27:35, Sami wrote:
> 0 /* ref_count */  (I had to look it up)

Done.

https://codereview.chromium.org/2523673003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:381:
if (!allow_throttling_)
On 2016/11/22 19:27:35, Sami wrote:
> Is this a necessary change? To me this query says "is queue X managed by the
> throttler?" Whether or not throttling is enabled is orthogonal to that.
> 
> Maybe we should rename this method and change the call site(s) that depend on
> this distinction also check whether throttling is enabled?

Yes, this is the source of the bug.

I think that comment in header is very clear: "Returns true if the |task_queue|
is throttled". That means that if throttling is disabled, this method should
return false.

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

https://codereview.chromium.org/2523673003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler_unittest.cc:996:
std::vector<base::TimeTicks> run_times;
On 2016/11/22 19:27:35, Sami wrote:
> I think this test could use a narrative summary comment about what exactly is
> being tested here.

Done.

[Sami, 2016-11-23 11:52:06]

Thanks!

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

https://codereview.chromium.org/2523673003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:381:
if (!allow_throttling_)
On 2016/11/23 11:11:29, altimin wrote:
> On 2016/11/22 19:27:35, Sami wrote:
> > Is this a necessary change? To me this query says "is queue X managed by the
> > throttler?" Whether or not throttling is enabled is orthogonal to that.
> > 
> > Maybe we should rename this method and change the call site(s) that depend
on
> > this distinction also check whether throttling is enabled?
> 
> Yes, this is the source of the bug.
> 
> I think that comment in header is very clear: "Returns true if the
|task_queue|
> is throttled". That means that if throttling is disabled, this method should
> return false.

I think this is still pretty confusing. Based on those comments if I call 
IncreaseThrottleRefCount(), I would expect IsThrottled() to return true but
that's not guaranteed at all.

How about this: We rename Metadata::IsThrottled() to CanThrottle() or something
like that and change the inc/decref comments to say they are actually making the
queue *eligible* for throttling -- contingent on the overall throttling state.
WDYT?

Alternatively we could rename this IsThrottled() to CanThrottle() too and make
the callers check both bits. I think I prefer the other model though.

[altimin, 2016-11-23 14:29:17]

PTAL

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

https://codereview.chromium.org/2523673003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:381:
if (!allow_throttling_)
On 2016/11/23 11:52:06, Sami wrote:
> On 2016/11/23 11:11:29, altimin wrote:
> > On 2016/11/22 19:27:35, Sami wrote:
> > > Is this a necessary change? To me this query says "is queue X managed by
the
> > > throttler?" Whether or not throttling is enabled is orthogonal to that.
> > > 
> > > Maybe we should rename this method and change the call site(s) that depend
> on
> > > this distinction also check whether throttling is enabled?
> > 
> > Yes, this is the source of the bug.
> > 
> > I think that comment in header is very clear: "Returns true if the
> |task_queue|
> > is throttled". That means that if throttling is disabled, this method should
> > return false.
> 
> I think this is still pretty confusing. Based on those comments if I call 
> IncreaseThrottleRefCount(), I would expect IsThrottled() to return true but
> that's not guaranteed at all.
> 
> How about this: We rename Metadata::IsThrottled() to CanThrottle() or
something
> like that and change the inc/decref comments to say they are actually making
the
> queue *eligible* for throttling -- contingent on the overall throttling state.
> WDYT?
> 
> Alternatively we could rename this IsThrottled() to CanThrottle() too and make
> the callers check both bits. I think I prefer the other model though.

It is very confusing indeed! I took another look and found more bugs originating
from that. I decided to remove Metadata::IsThrottled and rely on
TaskQueueThrottler::IsThrottler in the majority of cases or just use
Metadata.throttling_ref_count in a few places where it's really needed.

[altimin, 2016-11-23 14:29:28]

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

[commit-bot: I haz the power, 2016-11-23 14:29:38]

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-23 16:47:18]

Looks like a good improvement!

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

https://codereview.chromium.org/2523673003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:298:
if (IsThrottled(task_queue)) {
Hmm, this could end badly if the throttler is shut down while throttling is
disabled, no?

[altimin, 2016-11-23 16:56:08]

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

https://codereview.chromium.org/2523673003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:298:
if (IsThrottled(task_queue)) {
On 2016/11/23 16:47:18, Sami wrote:
> Hmm, this could end badly if the throttler is shut down while throttling is
> disabled, no?

If throttling is disabled then all queues are enabled and we don't have to do
anything during shutdown.

[Sami, 2016-11-23 17:00:59]

lgtm!

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

https://codereview.chromium.org/2523673003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:298:
if (IsThrottled(task_queue)) {
On 2016/11/23 16:56:08, altimin wrote:
> On 2016/11/23 16:47:18, Sami wrote:
> > Hmm, this could end badly if the throttler is shut down while throttling is
> > disabled, no?
> 
> If throttling is disabled then all queues are enabled and we don't have to do
> anything during shutdown.

I see, okay.

[altimin, 2016-11-23 17:06:39]

The CQ bit was unchecked by altimin@chromium.org

[altimin, 2016-11-23 17:06:40]

The CQ bit was checked by altimin@chromium.org

[commit-bot: I haz the power, 2016-11-23 17:06:53]

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-23 17:22:50]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1479920800543210,
"parent_rev": "821d403de03ce6af51581a1dad3b39f5f7663142", "commit_rev":
"64a5917bf288598bb57647753b70fdcd6d3627ac"}

[commit-bot: I haz the power, 2016-11-23 17:23:13]

Description was changed from

==========
[scheduler] Account for disabling throttling in TaskQueueThrottler::IsThrottled

TaskQueueThrottler::IsThrottled didn't respect global on/off throttling
switch. This led to TaskQueueThrottler::TimeBudgetPool::EnableThrottling
permanently disabling some queues.

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

BUG=639852
==========

to

==========
[scheduler] Account for disabling throttling in TaskQueueThrottler::IsThrottled

TaskQueueThrottler::IsThrottled didn't respect global on/off throttling
switch. This led to TaskQueueThrottler::TimeBudgetPool::EnableThrottling
permanently disabling some queues.

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

BUG=639852
==========

[commit-bot: I haz the power, 2016-11-23 17:23:14]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-23 17:25:51]

Description was changed from

==========
[scheduler] Account for disabling throttling in TaskQueueThrottler::IsThrottled

TaskQueueThrottler::IsThrottled didn't respect global on/off throttling
switch. This led to TaskQueueThrottler::TimeBudgetPool::EnableThrottling
permanently disabling some queues.

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

BUG=639852
==========

to

==========
[scheduler] Account for disabling throttling in TaskQueueThrottler::IsThrottled

TaskQueueThrottler::IsThrottled didn't respect global on/off throttling
switch. This led to TaskQueueThrottler::TimeBudgetPool::EnableThrottling
permanently disabling some queues.

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

BUG=639852

Committed: https://crrev.com/8991961c94284b66c45ef222bdd3f787327a3c81
Cr-Commit-Position: refs/heads/master@{#434195}
==========

[commit-bot: I haz the power, 2016-11-23 17:25:52]

Patchset 3 (id:??) landed as
https://crrev.com/8991961c94284b66c45ef222bdd3f787327a3c81
Cr-Commit-Position: refs/heads/master@{#434195}