Remove worker pool names from TaskScheduler.TaskLatency.* histograms.

Remove worker pool names from TaskScheduler.TaskLatency.* histograms.

These histograms:
- TaskScheduler.TaskLatency.BackgroundPool.BackgroundTaskPriority
- TaskScheduler.TaskLatency.BackgroundPool.UserVisibleTaskPriority
- TaskScheduler.TaskLatency.BackgroundPool.UserBlockingTaskPriority
- TaskScheduler.TaskLatency.BackgroundFileIOPool.BackgroundTaskPriority
- TaskScheduler.TaskLatency.BackgroundFileIOPool.UserVisibleTaskPriority
- TaskScheduler.TaskLatency.BackgroundFileIOPool.UserBlockingTaskPriority
- TaskScheduler.TaskLatency.ForegroundPool.BackgroundTaskPriority
- TaskScheduler.TaskLatency.ForegroundPool.UserVisibleTaskPriority
- TaskScheduler.TaskLatency.ForegroundPool.UserBlockingTaskPriority
- TaskScheduler.TaskLatency.ForegroundFileIOPool.BackgroundTaskPriority
- TaskScheduler.TaskLatency.ForegroundFileIOPool.UserVisibleTaskPriority
- TaskScheduler.TaskLatency.ForegroundFileIOPool.UserBlockingTaskPriority
are replaced with:
- TaskScheduler.TaskLatency.BackgroundTaskPriority
- TaskScheduler.TaskLatency.BackgroundTaskPriority.MayBlock
- TaskScheduler.TaskLatency.UserVisibleTaskPriority
- TaskScheduler.TaskLatency.UserVisibleTaskPriority.MayBlock
- TaskScheduler.TaskLatency.UserBlockingTaskPriority
- TaskScheduler.TaskLatency.UserBlockingTaskPriority.MayBlock

Not having a pool name in task latency histogram names will allow
us to keep track of the latency of each type of task when running
experiments that change the way TaskTraits are mapped to pools.

BUG=553459
Review-Url: https://codereview.chromium.org/2600023002
Cr-Commit-Position: refs/heads/master@{#442022}
Committed: https://chromium.googlesource.com/chromium/src/+/9d43cc2631f51968d3790e8709dde20bdedb5a6d

[fdoray, 2016-12-23 19:38:25]

Description was changed from

==========
Remove worker pool name from TaskScheduler.TaskLatency.* histograms.

These histograms:
- TaskScheduler.TaskLatency.BackgroundPool.BackgroundTaskPriority
- TaskScheduler.TaskLatency.BackgroundPool.UserVisibleTaskPriority
- TaskScheduler.TaskLatency.BackgroundPool.UserBlockingTaskPriority
- TaskScheduler.TaskLatency.BackgroundFileIOPool.BackgroundTaskPriority
- TaskScheduler.TaskLatency.BackgroundFileIOPool.UserVisibleTaskPriority
- TaskScheduler.TaskLatency.BackgroundFileIOPool.UserBlockingTaskPriority
- TaskScheduler.TaskLatency.ForegroundPool.BackgroundTaskPriority
- TaskScheduler.TaskLatency.ForegroundPool.UserVisibleTaskPriority
- TaskScheduler.TaskLatency.ForegroundPool.UserBlockingTaskPriority
- TaskScheduler.TaskLatency.ForegroundFileIOPool.BackgroundTaskPriority
- TaskScheduler.TaskLatency.ForegroundFileIOPool.UserVisibleTaskPriority
- TaskScheduler.TaskLatency.ForegroundFileIOPool.UserBlockingTaskPriority
are replaced with:
- TaskScheduler.TaskLatency.BackgroundTaskPriority
- TaskScheduler.TaskLatency.BackgroundTaskPriority.MayBlock
- TaskScheduler.TaskLatency.UserVisibleTaskPriority
- TaskScheduler.TaskLatency.UserVisibleTaskPriority.MayBlock
- TaskScheduler.TaskLatency.UserBlockingTaskPriority
- TaskScheduler.TaskLatency.UserBlockingTaskPriority.MayBlock

Not having a pool name in task latency histogram names will allow
us to keep track of the latency of each type of task when running
experiments that change the way TaskTraits are mapped to pools.

BUG=553459
==========

to

==========
Remove worker pool names from TaskScheduler.TaskLatency.* histograms.

These histograms:
- TaskScheduler.TaskLatency.BackgroundPool.BackgroundTaskPriority
- TaskScheduler.TaskLatency.BackgroundPool.UserVisibleTaskPriority
- TaskScheduler.TaskLatency.BackgroundPool.UserBlockingTaskPriority
- TaskScheduler.TaskLatency.BackgroundFileIOPool.BackgroundTaskPriority
- TaskScheduler.TaskLatency.BackgroundFileIOPool.UserVisibleTaskPriority
- TaskScheduler.TaskLatency.BackgroundFileIOPool.UserBlockingTaskPriority
- TaskScheduler.TaskLatency.ForegroundPool.BackgroundTaskPriority
- TaskScheduler.TaskLatency.ForegroundPool.UserVisibleTaskPriority
- TaskScheduler.TaskLatency.ForegroundPool.UserBlockingTaskPriority
- TaskScheduler.TaskLatency.ForegroundFileIOPool.BackgroundTaskPriority
- TaskScheduler.TaskLatency.ForegroundFileIOPool.UserVisibleTaskPriority
- TaskScheduler.TaskLatency.ForegroundFileIOPool.UserBlockingTaskPriority
are replaced with:
- TaskScheduler.TaskLatency.BackgroundTaskPriority
- TaskScheduler.TaskLatency.BackgroundTaskPriority.MayBlock
- TaskScheduler.TaskLatency.UserVisibleTaskPriority
- TaskScheduler.TaskLatency.UserVisibleTaskPriority.MayBlock
- TaskScheduler.TaskLatency.UserBlockingTaskPriority
- TaskScheduler.TaskLatency.UserBlockingTaskPriority.MayBlock

Not having a pool name in task latency histogram names will allow
us to keep track of the latency of each type of task when running
experiments that change the way TaskTraits are mapped to pools.

BUG=553459
==========

[fdoray, 2016-12-23 19:46:51]

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

[commit-bot: I haz the power, 2016-12-23 19:47:02]

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

[fdoray, 2016-12-23 19:47:05]

fdoray@chromium.org changed reviewers:
+ gab@chromium.org

[fdoray, 2016-12-23 19:47:06]

PTAL

[commit-bot: I haz the power, 2016-12-23 20:54:20]

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

[commit-bot: I haz the power, 2016-12-23 20:54:21]

Dry run: This issue passed the CQ dry run.

[gab, 2017-01-05 19:25:46]

lgtm w/ comments

https://codereview.chromium.org/2600023002/diff/20001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_worker.cc (right):

https://codereview.chromium.org/2600023002/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker.cc:81: std::unique_ptr<Task> task =
sequence->TakeTask();
inline

https://codereview.chromium.org/2600023002/diff/20001/base/task_scheduler/tas...
File base/task_scheduler/task_tracker.cc (right):

https://codereview.chromium.org/2600023002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_tracker.cc:194:
GetTaskLatencyHistogram("UserBlockingTaskPriority.MayBlock")}} {}
// Confirm that all |task_latency_histograms_| have been initialized above.
DCHECK(task_latency_histograms_[static_cast<int>(TaskPriority::HIGHEST) + 1][0]
- 1);

https://codereview.chromium.org/2600023002/diff/20001/base/task_scheduler/tas...
File base/task_scheduler/task_tracker.h (right):

https://codereview.chromium.org/2600023002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_tracker.h:32: // enforces shutdown semantics, records
metrics and takes care of tracing and
, and

https://codereview.chromium.org/2600023002/diff/20001/base/task_scheduler/tas...
File base/task_scheduler/task_tracker_unittest.cc (right):

https://codereview.chromium.org/2600023002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_tracker_unittest.cc:934:
TaskPriority::USER_BLOCKING)][1]}};
Use HistogramTester instead of friending and using the internal value in the
test.

https://codereview.chromium.org/2600023002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_tracker_unittest.cc:936: for (const auto& test : tests)
{
Interesting use of auto! I thought something was wrong with the above struct as
it doesn't declare a type but I guess auto is fine with that (i.e. auto is the
only valid type here!) :-O

https://codereview.chromium.org/2600023002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2600023002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:115548: <affected-histogram
name="TaskScheduler.NumTasksBetweenWaits"/>
These are still suffixed? Or do we intend to change them too?

[fdoray, 2017-01-05 20:13:27]

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

[fdoray, 2017-01-05 20:13:37]

fdoray@chromium.org changed reviewers:
+ rkaplow@chromium.org

[fdoray, 2017-01-05 20:13:38]

rkaplow@: PTAL

https://codereview.chromium.org/2600023002/diff/20001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_worker.cc (right):

https://codereview.chromium.org/2600023002/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker.cc:81: std::unique_ptr<Task> task =
sequence->TakeTask();
On 2017/01/05 19:25:46, gab wrote:
> inline

Done.

https://codereview.chromium.org/2600023002/diff/20001/base/task_scheduler/tas...
File base/task_scheduler/task_tracker.cc (right):

https://codereview.chromium.org/2600023002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_tracker.cc:194:
GetTaskLatencyHistogram("UserBlockingTaskPriority.MayBlock")}} {}
On 2017/01/05 19:25:46, gab wrote:
> // Confirm that all |task_latency_histograms_| have been initialized above.
> DCHECK(task_latency_histograms_[static_cast<int>(TaskPriority::HIGHEST) +
1][0]
> - 1);

Done.

https://codereview.chromium.org/2600023002/diff/20001/base/task_scheduler/tas...
File base/task_scheduler/task_tracker.h (right):

https://codereview.chromium.org/2600023002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_tracker.h:32: // enforces shutdown semantics, records
metrics and takes care of tracing and
On 2017/01/05 19:25:46, gab wrote:
> , and

Done.

https://codereview.chromium.org/2600023002/diff/20001/base/task_scheduler/tas...
File base/task_scheduler/task_tracker_unittest.cc (right):

https://codereview.chromium.org/2600023002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_tracker_unittest.cc:934:
TaskPriority::USER_BLOCKING)][1]}};
On 2017/01/05 19:25:46, gab wrote:
> Use HistogramTester instead of friending and using the internal value in the
> test.

Done.

https://codereview.chromium.org/2600023002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_tracker_unittest.cc:936: for (const auto& test : tests)
{
On 2017/01/05 19:25:46, gab wrote:
> Interesting use of auto! I thought something was wrong with the above struct
as
> it doesn't declare a type but I guess auto is fine with that (i.e. auto is the
> only valid type here!) :-O

Acknowledged.

https://codereview.chromium.org/2600023002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2600023002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:115548: <affected-histogram
name="TaskScheduler.NumTasksBetweenWaits"/>
On 2017/01/05 19:25:46, gab wrote:
> These are still suffixed? Or do we intend to change them too?

I removed pool suffixes from TaskLatency histograms because values are
associated with individual tasks. Not having pool suffixes for TaskLatency allow
us to keep track of latencies with different pool implementations.

I think we should keep pool suffixes for
DetachDuration/NumTasksBeforeDetach/NumTasksBetweenWaits. These histograms are
specific to the current implementation (they don't make sense with a
TaskScheduler backed by the Windows Thread Pool or GCD). The values don't refer
to individual tasks.

[commit-bot: I haz the power, 2017-01-05 20:13:55]

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-05 21:15:02]

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

[commit-bot: I haz the power, 2017-01-05 21:15:03]

Dry run: This issue passed the CQ dry run.

[gab, 2017-01-05 22:16:36]

https://codereview.chromium.org/2600023002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2600023002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:115548: <affected-histogram
name="TaskScheduler.NumTasksBetweenWaits"/>
On 2017/01/05 20:13:38, fdoray wrote:
> On 2017/01/05 19:25:46, gab wrote:
> > These are still suffixed? Or do we intend to change them too?
> 
> I removed pool suffixes from TaskLatency histograms because values are
> associated with individual tasks. Not having pool suffixes for TaskLatency
allow
> us to keep track of latencies with different pool implementations.
> 
> I think we should keep pool suffixes for
> DetachDuration/NumTasksBeforeDetach/NumTasksBetweenWaits. These histograms are
> specific to the current implementation (they don't make sense with a
> TaskScheduler backed by the Windows Thread Pool or GCD). The values don't
refer
> to individual tasks.

Right, sgtm.

https://codereview.chromium.org/2600023002/diff/80001/base/task_scheduler/tas...
File base/task_scheduler/task_tracker.cc (right):

https://codereview.chromium.org/2600023002/diff/80001/base/task_scheduler/tas...
base/task_scheduler/task_tracker.cc:198: 1));
This is git cl format? Looks terrible..!

https://codereview.chromium.org/2600023002/diff/80001/base/task_scheduler/tas...
File base/task_scheduler/task_tracker.h (right):

https://codereview.chromium.org/2600023002/diff/80001/base/task_scheduler/tas...
base/task_scheduler/task_tracker.h:85:
FRIEND_TEST_ALL_PREFIXES(TaskSchedulerTaskTrackerHistogramTest, TaskLatency);
Drop friend?

[fdoray, 2017-01-05 22:31:58]

rkaplow@: PTAL

https://codereview.chromium.org/2600023002/diff/80001/base/task_scheduler/tas...
File base/task_scheduler/task_tracker.cc (right):

https://codereview.chromium.org/2600023002/diff/80001/base/task_scheduler/tas...
base/task_scheduler/task_tracker.cc:198: 1));
On 2017/01/05 22:16:36, gab wrote:
> This is git cl format? Looks terrible..!

Yes. And there is a presubmit check to verify that base/ is git cl formatted, so
we can't change this.

https://codereview.chromium.org/2600023002/diff/80001/base/task_scheduler/tas...
File base/task_scheduler/task_tracker.h (right):

https://codereview.chromium.org/2600023002/diff/80001/base/task_scheduler/tas...
base/task_scheduler/task_tracker.h:85:
FRIEND_TEST_ALL_PREFIXES(TaskSchedulerTaskTrackerHistogramTest, TaskLatency);
On 2017/01/05 22:16:36, gab wrote:
> Drop friend?

Done.

[gab, 2017-01-05 22:49:00]

lgtm++

[rkaplow, 2017-01-06 15:41:13]

lgtm

[fdoray, 2017-01-06 16:18:59]

The CQ bit was checked by fdoray@chromium.org

[commit-bot: I haz the power, 2017-01-06 16:19:32]

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-06 18:46:17]

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

[commit-bot: I haz the power, 2017-01-06 18:46:18]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[fdoray, 2017-01-06 18:53:41]

The CQ bit was checked by fdoray@chromium.org

[commit-bot: I haz the power, 2017-01-06 18:54:18]

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-06 20:12:27]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1483728821555440,
"parent_rev": "253fdc20dcf70575cb682733a33bb69ee099f2e4", "commit_rev":
"9d43cc2631f51968d3790e8709dde20bdedb5a6d"}

[commit-bot: I haz the power, 2017-01-06 20:12:58]

Description was changed from

==========
Remove worker pool names from TaskScheduler.TaskLatency.* histograms.

These histograms:
- TaskScheduler.TaskLatency.BackgroundPool.BackgroundTaskPriority
- TaskScheduler.TaskLatency.BackgroundPool.UserVisibleTaskPriority
- TaskScheduler.TaskLatency.BackgroundPool.UserBlockingTaskPriority
- TaskScheduler.TaskLatency.BackgroundFileIOPool.BackgroundTaskPriority
- TaskScheduler.TaskLatency.BackgroundFileIOPool.UserVisibleTaskPriority
- TaskScheduler.TaskLatency.BackgroundFileIOPool.UserBlockingTaskPriority
- TaskScheduler.TaskLatency.ForegroundPool.BackgroundTaskPriority
- TaskScheduler.TaskLatency.ForegroundPool.UserVisibleTaskPriority
- TaskScheduler.TaskLatency.ForegroundPool.UserBlockingTaskPriority
- TaskScheduler.TaskLatency.ForegroundFileIOPool.BackgroundTaskPriority
- TaskScheduler.TaskLatency.ForegroundFileIOPool.UserVisibleTaskPriority
- TaskScheduler.TaskLatency.ForegroundFileIOPool.UserBlockingTaskPriority
are replaced with:
- TaskScheduler.TaskLatency.BackgroundTaskPriority
- TaskScheduler.TaskLatency.BackgroundTaskPriority.MayBlock
- TaskScheduler.TaskLatency.UserVisibleTaskPriority
- TaskScheduler.TaskLatency.UserVisibleTaskPriority.MayBlock
- TaskScheduler.TaskLatency.UserBlockingTaskPriority
- TaskScheduler.TaskLatency.UserBlockingTaskPriority.MayBlock

Not having a pool name in task latency histogram names will allow
us to keep track of the latency of each type of task when running
experiments that change the way TaskTraits are mapped to pools.

BUG=553459
==========

to

==========
Remove worker pool names from TaskScheduler.TaskLatency.* histograms.

These histograms:
- TaskScheduler.TaskLatency.BackgroundPool.BackgroundTaskPriority
- TaskScheduler.TaskLatency.BackgroundPool.UserVisibleTaskPriority
- TaskScheduler.TaskLatency.BackgroundPool.UserBlockingTaskPriority
- TaskScheduler.TaskLatency.BackgroundFileIOPool.BackgroundTaskPriority
- TaskScheduler.TaskLatency.BackgroundFileIOPool.UserVisibleTaskPriority
- TaskScheduler.TaskLatency.BackgroundFileIOPool.UserBlockingTaskPriority
- TaskScheduler.TaskLatency.ForegroundPool.BackgroundTaskPriority
- TaskScheduler.TaskLatency.ForegroundPool.UserVisibleTaskPriority
- TaskScheduler.TaskLatency.ForegroundPool.UserBlockingTaskPriority
- TaskScheduler.TaskLatency.ForegroundFileIOPool.BackgroundTaskPriority
- TaskScheduler.TaskLatency.ForegroundFileIOPool.UserVisibleTaskPriority
- TaskScheduler.TaskLatency.ForegroundFileIOPool.UserBlockingTaskPriority
are replaced with:
- TaskScheduler.TaskLatency.BackgroundTaskPriority
- TaskScheduler.TaskLatency.BackgroundTaskPriority.MayBlock
- TaskScheduler.TaskLatency.UserVisibleTaskPriority
- TaskScheduler.TaskLatency.UserVisibleTaskPriority.MayBlock
- TaskScheduler.TaskLatency.UserBlockingTaskPriority
- TaskScheduler.TaskLatency.UserBlockingTaskPriority.MayBlock

Not having a pool name in task latency histogram names will allow
us to keep track of the latency of each type of task when running
experiments that change the way TaskTraits are mapped to pools.

BUG=553459

Review-Url: https://codereview.chromium.org/2600023002
Cr-Commit-Position: refs/heads/master@{#442022}
Committed:
https://chromium.googlesource.com/chromium/src/+/9d43cc2631f51968d3790e8709dd...
==========

[commit-bot: I haz the power, 2017-01-06 20:13:00]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/9d43cc2631f51968d3790e8709dd...