New issue
Advanced search Search tips

Issue 803019 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , iOS , Chrome , Mac , Fuchsia
Pri: 3
Type: Bug



Sign in to add a comment

[Feature] Better per process histograms for TaskScheduler

Project Member Reported by gab@chromium.org, Jan 17 2018

Issue description

* Add a TaskSchedulerName suffix
* Unify existing histograms to use it
* Explicitly name the Browser's TaskScheduler

Let's do this in M65 so that we don't have to worry about maintaining two code paths (given it's branching very soon).
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jan 18 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/19621cd877f25b75777a3ebac12419cecf3bc9bb

commit 19621cd877f25b75777a3ebac12419cecf3bc9bb
Author: Gabriel Charette <gab@chromium.org>
Date: Thu Jan 18 16:48:59 2018

Add TaskScheduler name to TaskLatencyMicroseconds histograms

Also tweak other histograms to use TaskSchedulerName suffix consistently
(instead of mixing it with pool name in some other histograms).

Also add a few Deprecated 4/2017 tags for TaskScheduler.TaskLatency.
which were missed in r468135.

R=fdoray@chromium.org, robliao@chromium.org

Bug:  803019 
Change-Id: I30c3c30016d91d52c9de4d47944d68f2f250be07
Reviewed-on: https://chromium-review.googlesource.com/870116
Reviewed-by: François Doray <fdoray@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530165}
[modify] https://crrev.com/19621cd877f25b75777a3ebac12419cecf3bc9bb/base/task_scheduler/scheduler_worker_pool_impl.cc
[modify] https://crrev.com/19621cd877f25b75777a3ebac12419cecf3bc9bb/base/task_scheduler/task_scheduler_impl.cc
[modify] https://crrev.com/19621cd877f25b75777a3ebac12419cecf3bc9bb/base/task_scheduler/task_scheduler_impl.h
[modify] https://crrev.com/19621cd877f25b75777a3ebac12419cecf3bc9bb/base/task_scheduler/task_tracker.cc
[modify] https://crrev.com/19621cd877f25b75777a3ebac12419cecf3bc9bb/base/task_scheduler/task_tracker.h
[modify] https://crrev.com/19621cd877f25b75777a3ebac12419cecf3bc9bb/base/task_scheduler/task_tracker_posix.cc
[modify] https://crrev.com/19621cd877f25b75777a3ebac12419cecf3bc9bb/base/task_scheduler/task_tracker_posix.h
[modify] https://crrev.com/19621cd877f25b75777a3ebac12419cecf3bc9bb/base/task_scheduler/task_tracker_unittest.cc
[modify] https://crrev.com/19621cd877f25b75777a3ebac12419cecf3bc9bb/tools/metrics/histograms/histograms.xml

Project Member

Comment 2 by bugdroid1@chromium.org, Jan 18 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ed64637586719e017163361adca115173ac8d09f

commit ed64637586719e017163361adca115173ac8d09f
Author: Gabriel Charette <gab@chromium.org>
Date: Thu Jan 18 16:56:31 2018

Remove TaskScheduler name from its worker thread's names

A thread is always in the context of a process and adding the process
name to the thread name is redundant (and the name is already long enough
as-is). This keeps TaskScheduler threads with the same name in the
browser and brings other processes on par (just like we don't name the
IO thread differently in other processes).

This is a prerequisite to always require a TaskScheduler name (to always
suffix histograms). Otherwise we would making the browser's worker thread
names even longer.

Also, renamed a few |name| variables to be more descriptive because I
got confused a few times in what each |name| variable held (e.g. PS1).
Intentionally kept |name| in the public interface. That way we can
change how it's used in the impl but having specific names there helps
identify what it's *currently* used for.

Bug:  803019 
Change-Id: Iaeadce43706f647a7e8de0f843da35303171a0fa
Reviewed-on: https://chromium-review.googlesource.com/873377
Reviewed-by: François Doray <fdoray@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530170}
[modify] https://crrev.com/ed64637586719e017163361adca115173ac8d09f/base/task_scheduler/scheduler_single_thread_task_runner_manager.cc
[modify] https://crrev.com/ed64637586719e017163361adca115173ac8d09f/base/task_scheduler/scheduler_single_thread_task_runner_manager.h
[modify] https://crrev.com/ed64637586719e017163361adca115173ac8d09f/base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc
[modify] https://crrev.com/ed64637586719e017163361adca115173ac8d09f/base/task_scheduler/scheduler_worker_pool_impl.cc
[modify] https://crrev.com/ed64637586719e017163361adca115173ac8d09f/base/task_scheduler/scheduler_worker_pool_impl.h
[modify] https://crrev.com/ed64637586719e017163361adca115173ac8d09f/base/task_scheduler/scheduler_worker_pool_impl_unittest.cc
[modify] https://crrev.com/ed64637586719e017163361adca115173ac8d09f/base/task_scheduler/scheduler_worker_pool_unittest.cc
[modify] https://crrev.com/ed64637586719e017163361adca115173ac8d09f/base/task_scheduler/task_scheduler.h
[modify] https://crrev.com/ed64637586719e017163361adca115173ac8d09f/base/task_scheduler/task_scheduler_impl.cc
[modify] https://crrev.com/ed64637586719e017163361adca115173ac8d09f/base/task_scheduler/task_scheduler_impl.h
[modify] https://crrev.com/ed64637586719e017163361adca115173ac8d09f/base/task_scheduler/task_tracker.cc
[modify] https://crrev.com/ed64637586719e017163361adca115173ac8d09f/base/task_scheduler/task_tracker.h

Project Member

Comment 3 by bugdroid1@chromium.org, Jan 19 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5b74dcb487bffd7777f94cd763aa65b7212486a9

commit 5b74dcb487bffd7777f94cd763aa65b7212486a9
Author: Gabriel Charette <gab@chromium.org>
Date: Fri Jan 19 11:00:53 2018

Add explicit ".Browser" suffix for TaskScheduler histograms from browser process.

This makes it easier to isolate them in UMA tools.

Intentionally do not support a backward compatible path. With M65 going
to beta shortly this avoids an intermediate mess.

R=fdoray@chromium.org, robliao@chromium.org
TBR=sdefresne@chromium.org (ios mirror change)

Bug:  803019 
Change-Id: I10c05b4dc5dfed18a7d26255676af980ce233eaa
Reviewed-on: https://chromium-review.googlesource.com/870390
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Reviewed-by: Robert Liao <robliao@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530481}
[modify] https://crrev.com/5b74dcb487bffd7777f94cd763aa65b7212486a9/base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc
[modify] https://crrev.com/5b74dcb487bffd7777f94cd763aa65b7212486a9/base/task_scheduler/scheduler_worker_pool_impl_unittest.cc
[modify] https://crrev.com/5b74dcb487bffd7777f94cd763aa65b7212486a9/base/task_scheduler/scheduler_worker_pool_unittest.cc
[modify] https://crrev.com/5b74dcb487bffd7777f94cd763aa65b7212486a9/base/task_scheduler/scheduler_worker_stack_unittest.cc
[modify] https://crrev.com/5b74dcb487bffd7777f94cd763aa65b7212486a9/base/task_scheduler/scheduler_worker_unittest.cc
[modify] https://crrev.com/5b74dcb487bffd7777f94cd763aa65b7212486a9/base/task_scheduler/task_scheduler_impl.cc
[modify] https://crrev.com/5b74dcb487bffd7777f94cd763aa65b7212486a9/base/task_scheduler/task_scheduler_impl.h
[modify] https://crrev.com/5b74dcb487bffd7777f94cd763aa65b7212486a9/base/task_scheduler/task_tracker.cc
[modify] https://crrev.com/5b74dcb487bffd7777f94cd763aa65b7212486a9/base/task_scheduler/task_tracker.h
[modify] https://crrev.com/5b74dcb487bffd7777f94cd763aa65b7212486a9/base/task_scheduler/task_tracker_posix.h
[modify] https://crrev.com/5b74dcb487bffd7777f94cd763aa65b7212486a9/base/task_scheduler/task_tracker_posix_unittest.cc
[modify] https://crrev.com/5b74dcb487bffd7777f94cd763aa65b7212486a9/base/task_scheduler/task_tracker_unittest.cc
[modify] https://crrev.com/5b74dcb487bffd7777f94cd763aa65b7212486a9/base/test/scoped_task_environment.cc
[modify] https://crrev.com/5b74dcb487bffd7777f94cd763aa65b7212486a9/content/browser/browser_main_loop.cc
[modify] https://crrev.com/5b74dcb487bffd7777f94cd763aa65b7212486a9/ios/web/public/global_state/ios_global_state.mm
[modify] https://crrev.com/5b74dcb487bffd7777f94cd763aa65b7212486a9/tools/metrics/histograms/histograms.xml

Comment 4 by gab@chromium.org, Jan 22 2018

Labels: Merge-Request-65
Status: Fx (was: Started)
r530481 missed the branch cut. We need it in M65 to avoid having mixed metrics (reported the old way) for a full milestone.

Verified on Canary that chrome://histograms/TaskScheduler has the right output.

Comment 5 by gab@chromium.org, Jan 22 2018

Status: Fixed (was: Fx)

Comment 6 by gov...@chromium.org, Jan 22 2018

Pls apply appropriate OSs label. Thank you.

Comment 7 by gab@chromium.org, Jan 22 2018

Labels: OS-Android OS-Chrome OS-Fuchsia OS-iOS OS-Linux OS-Mac OS-Windows
This is for All, thanks.
Project Member

Comment 8 by sheriffbot@chromium.org, Jan 23 2018

Labels: -Merge-Request-65 Hotlist-Merge-Approved Merge-Approved-65
Your change meets the bar and is auto-approved for M65. Please go ahead and merge the CL to branch 3325 manually. Please contact milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 9 by gov...@chromium.org, Jan 23 2018

Pls merge your change to M65 branch 3325 before 1:00 PM PT today, Tuesday (01/23/18) so we can pick it up for dev release tomorrow. Thank you.
Project Member

Comment 10 by bugdroid1@chromium.org, Jan 23 2018

Labels: -merge-approved-65 merge-merged-3325
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c91c4f71678c7f370b199ddf560333551d588f74

commit c91c4f71678c7f370b199ddf560333551d588f74
Author: Gabriel Charette <gab@chromium.org>
Date: Tue Jan 23 13:26:16 2018

[Merge M65] Add explicit ".Browser" suffix for TaskScheduler histograms from browser process.

This makes it easier to isolate them in UMA tools.

Intentionally do not support a backward compatible path. With M65 going
to beta shortly this avoids an intermediate mess.

TBR=fdoray@chromium.org, robliao@chromium.org, sdefresne@chromium.org, alexmos@chromium.org

(cherry picked from commit 5b74dcb487bffd7777f94cd763aa65b7212486a9)

Bug:  803019 
Change-Id: I10c05b4dc5dfed18a7d26255676af980ce233eaa
Reviewed-on: https://chromium-review.googlesource.com/870390
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Reviewed-by: Robert Liao <robliao@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#530481}
Reviewed-on: https://chromium-review.googlesource.com/880966
Cr-Commit-Position: refs/branch-heads/3325@{#27}
Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369}
[modify] https://crrev.com/c91c4f71678c7f370b199ddf560333551d588f74/base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc
[modify] https://crrev.com/c91c4f71678c7f370b199ddf560333551d588f74/base/task_scheduler/scheduler_worker_pool_impl_unittest.cc
[modify] https://crrev.com/c91c4f71678c7f370b199ddf560333551d588f74/base/task_scheduler/scheduler_worker_pool_unittest.cc
[modify] https://crrev.com/c91c4f71678c7f370b199ddf560333551d588f74/base/task_scheduler/scheduler_worker_stack_unittest.cc
[modify] https://crrev.com/c91c4f71678c7f370b199ddf560333551d588f74/base/task_scheduler/scheduler_worker_unittest.cc
[modify] https://crrev.com/c91c4f71678c7f370b199ddf560333551d588f74/base/task_scheduler/task_scheduler_impl.cc
[modify] https://crrev.com/c91c4f71678c7f370b199ddf560333551d588f74/base/task_scheduler/task_scheduler_impl.h
[modify] https://crrev.com/c91c4f71678c7f370b199ddf560333551d588f74/base/task_scheduler/task_tracker.cc
[modify] https://crrev.com/c91c4f71678c7f370b199ddf560333551d588f74/base/task_scheduler/task_tracker.h
[modify] https://crrev.com/c91c4f71678c7f370b199ddf560333551d588f74/base/task_scheduler/task_tracker_posix.h
[modify] https://crrev.com/c91c4f71678c7f370b199ddf560333551d588f74/base/task_scheduler/task_tracker_posix_unittest.cc
[modify] https://crrev.com/c91c4f71678c7f370b199ddf560333551d588f74/base/task_scheduler/task_tracker_unittest.cc
[modify] https://crrev.com/c91c4f71678c7f370b199ddf560333551d588f74/base/test/scoped_task_environment.cc
[modify] https://crrev.com/c91c4f71678c7f370b199ddf560333551d588f74/content/browser/browser_main_loop.cc
[modify] https://crrev.com/c91c4f71678c7f370b199ddf560333551d588f74/ios/web/public/global_state/ios_global_state.mm
[modify] https://crrev.com/c91c4f71678c7f370b199ddf560333551d588f74/tools/metrics/histograms/histograms.xml

Sign in to add a comment