Record PendingTaskCount when a backgrounded renderer is suspended.

Record PendingTaskCount when a backgrounded renderer is suspended.

To understand PurgeAndSuspend, we need to see how many tasks are still
in task_queues when a backgrounded renderer is suspended.

- intent-to-implement-and-ship of background renderer's purge + suspend is
 https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4
 - one of the documents attached in the above intent is
  https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJvI/edit?usp=sharing
  - The feature is not enabled by default because purge-and-suspend-time is 0.

BUG=635419
Committed: https://crrev.com/a3ed913e99d2d9acf6413ae56e1c8d1da4382522
Cr-Commit-Position: refs/heads/master@{#425627}

[tasak, 2016-10-13 09:00:37]

The CQ bit was checked by tasak@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 09:00:56]

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, 2016-10-13 10:24:40]

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

[commit-bot: I haz the power, 2016-10-13 10:24:41]

Dry run: This issue passed the CQ dry run.

[tasak, 2016-10-13 11:01:56]

The CQ bit was checked by tasak@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 11:02:10]

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

[tasak, 2016-10-13 11:04:14]

tasak@google.com changed reviewers:
+ haraken@chromium.org, skyostil@chromium.org

[tasak, 2016-10-13 11:04:15]

Would you review this CL?

According to the meeting, I created this patch to add UMA (i.e. number of
pending task) to RendererSchedulerImpl::SuspendRenderer.

[commit-bot: I haz the power, 2016-10-13 12:17:28]

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

[commit-bot: I haz the power, 2016-10-13 12:17:29]

Dry run: This issue passed the CQ dry run.

[Sami, 2016-10-14 01:38:18]

Thanks, this will be interesting to know.

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

https://codereview.chromium.org/2416803003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h:124:
unsigned GetNumberOfPendingTasks() const override;
nit: Please use size_t instead of unsigned types.

https://codereview.chromium.org/2416803003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scheduler/child/idle_helper.h (right):

https://codereview.chromium.org/2416803003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/child/idle_helper.h:146: int
GetNumberOfPendingIdleTasks();
size_t

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

https://codereview.chromium.org/2416803003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:503:
int RendererSchedulerImpl::GetNumberOfPendingTasks() {
I would suggest adding this getter into TaskQueueManager instead so it can just
iterate over |queues_| there. Otherwise it is easy to forget to add a queue here
when we introduce new ones.

[tasak, 2016-10-14 05:55:32]

Thank you for review.

So is it better to test GetNumberOfPendingTasks in RendererSchedulerImplTest or
SchedulerHelperTest?

Talking about the newest patch, I tried to added the test into
SchedulerHelperTest.

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

https://codereview.chromium.org/2416803003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h:124:
unsigned GetNumberOfPendingTasks() const override;
On 2016/10/14 01:38:18, Sami (slow -- traveling) wrote:
> nit: Please use size_t instead of unsigned types.

Done.

https://codereview.chromium.org/2416803003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scheduler/child/idle_helper.h (right):

https://codereview.chromium.org/2416803003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/child/idle_helper.h:146: int
GetNumberOfPendingIdleTasks();
On 2016/10/14 01:38:18, Sami (slow -- traveling) wrote:
> size_t

I agree. 
...so I found that we don't need this method. Because I added
GetNumberOfPendingTasks to TaskQueueManager (and also SchedulerHelper).

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

https://codereview.chromium.org/2416803003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:503:
int RendererSchedulerImpl::GetNumberOfPendingTasks() {
On 2016/10/14 01:38:18, Sami (slow -- traveling) wrote:
> I would suggest adding this getter into TaskQueueManager instead so it can
just
> iterate over |queues_| there. Otherwise it is easy to forget to add a queue
here
> when we introduce new ones.

I see. I agree that TaskQueueManager is the best class to add the getter.
Done.

[Sami, 2016-10-14 06:30:05]

On 2016/10/14 05:55:32, tasak wrote:
> Thank you for review.
> 
> So is it better to test GetNumberOfPendingTasks in RendererSchedulerImplTest
or
> SchedulerHelperTest?

I would probably move the test to TaskQueueManagerTest.

With that change this lgtm.

[haraken, 2016-10-14 06:45:33]

LGTM

[tasak, 2016-10-17 05:56:08]

The CQ bit was checked by tasak@google.com

[commit-bot: I haz the power, 2016-10-17 05:56:27]

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-10-17 07:21:25]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-17 07:23:29]

Description was changed from

==========
Record PendingTaskCount when a backgrounded renderer is suspended.

To understand PurgeAndSuspend, we need to see how many tasks are still
in task_queues when a backgrounded renderer is suspended.

- intent-to-implement-and-ship of background renderer's purge + suspend is
 https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4
 - one of the documents attached in the above intent is
 
https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ...
  - The feature is not enabled by default because purge-and-suspend-time is 0.

BUG=635419
==========

to

==========
Record PendingTaskCount when a backgrounded renderer is suspended.

To understand PurgeAndSuspend, we need to see how many tasks are still
in task_queues when a backgrounded renderer is suspended.

- intent-to-implement-and-ship of background renderer's purge + suspend is
 https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4
 - one of the documents attached in the above intent is
 
https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ...
  - The feature is not enabled by default because purge-and-suspend-time is 0.

BUG=635419

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

[commit-bot: I haz the power, 2016-10-17 07:23:31]

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