New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 871204 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 1
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 2
Type: Bug

Blocked on:
issue 874982



Sign in to add a comment

Fix ThreadControllerImpl::DoWork trace event.

Project Member Reported by altimin@chromium.org, Aug 6

Issue description

ThreadControllerImpl::DoWork trace event can intersect with SequenceManager::RunTask trace event.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 7

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

commit d2e82c61666a4be7c5232424d0aab8da01100855
Author: Alexander Timin <altimin@chromium.org>
Date: Tue Aug 07 16:19:33 2018

[scheduler] Fix ThreadControllerImpl::DoWork trace event.

Replace ThreadControllerImpl:DoWork which has strange scope with
ThreadControllerImpl::DoWork with batch-level scope and
ThreadControllerImpl::RunTask with task-level scope.

R=skyostil@chromium.org
BUG= 871204 

Change-Id: Ifdf410b552143b3e0b9c603ed0ded5f621c3ea5f
Reviewed-on: https://chromium-review.googlesource.com/1163611
Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
Commit-Queue: Alexander Timin <altimin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581244}
[modify] https://crrev.com/d2e82c61666a4be7c5232424d0aab8da01100855/base/task/sequence_manager/thread_controller_impl.cc

Labels: Merge-Request-69
We want this in M69 as well. This is trace-only change, so it's safe.
Project Member

Comment 3 by sheriffbot@chromium.org, Aug 8

Labels: -Merge-Request-69 Merge-Review-69 Hotlist-Merge-Review
This bug requires manual review: M69 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Pls apply appropriate OSs label. Thank you.
Labels: OS-Android OS-Chrome OS-Linux OS-Mac OS-Windows
Labels: -Merge-Review-69 Merge-Approved-69
Approving merge to M69 branch 3497 based on comment #2 and per offline chat with altimin@. 
Labels: M-69
Project Member

Comment 8 by sheriffbot@chromium.org, Aug 13

Cc: gov...@chromium.org
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Cc: skyos...@chromium.org
Requesting merge to M69 ASAP. Thank you.
Project Member

Comment 10 by bugdroid1@chromium.org, Aug 14

Labels: -merge-approved-69 merge-merged-3497
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/be51c1084af1b0369e77e5c00a82a5546e70bb7d

commit be51c1084af1b0369e77e5c00a82a5546e70bb7d
Author: Alexander Timin <altimin@chromium.org>
Date: Tue Aug 14 20:27:44 2018

[scheduler] Fix ThreadControllerImpl::DoWork trace event.

Replace ThreadControllerImpl:DoWork which has strange scope with
ThreadControllerImpl::DoWork with batch-level scope and
ThreadControllerImpl::RunTask with task-level scope.

R=skyostil@chromium.org
TBR=altimin@chromium.org
BUG= 871204 

(cherry picked from commit 27620cc5abb803c373ea3fdf27d463971f1509c1)

Change-Id: Ifdf410b552143b3e0b9c603ed0ded5f621c3ea5f
Reviewed-on: https://chromium-review.googlesource.com/1163611
Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
Commit-Queue: Alexander Timin <altimin@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#581244}
Reviewed-on: https://chromium-review.googlesource.com/1174840
Reviewed-by: Alexander Timin <altimin@chromium.org>
Cr-Commit-Position: refs/branch-heads/3497@{#628}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/be51c1084af1b0369e77e5c00a82a5546e70bb7d/base/task/sequence_manager/thread_controller_impl.cc

Cc: bckenny@google.com u...@chromium.org dproy@chromium.org phulce@chromium.org
+lighthouse folks
+traceviewer folks because of TOP_LEVEL_TASK_TITLES in https://cs.chromium.org/chromium/src/third_party/catapult/tracing/tracing/extras/chrome/event_finder_utils.html?type=cs&q=ThreadControllerImpl::DoWork&sq=package:chromium&g=0&l=25-34 


The DoWork trace event is now under the 'sequence_manager' category and no longer has the same meaning.
The new "ThreadControllerImpl::RunTask" event (on 'toplevel') reflects the same uninterruptible task used for our metric purposes.


Also, altimin points out over IM: "The RunTask scope is refined and it should be slightly smaller than of the old DoWork."
FYI we'll have to fix this in Lighthouse, roll it into DevTools audits panel, and also merge back to m69.
Not sure if it's important enough for traceviewer for them to fix and merge, too.
Blockedon: 874982
Status: Fixed (was: Started)
Project Member

Comment 15 by bugdroid1@chromium.org, Oct 15

The following revision refers to this bug:
  https://chromium.googlesource.com/catapult/+/c2f72b8a9299ddd88999b154b797bce10201d853

commit c2f72b8a9299ddd88999b154b797bce10201d853
Author: Deepanjan Roy <dproy@chromium.org>
Date: Mon Oct 15 19:30:00 2018

Fix top level scheduler task title

The top level scheduler task was renamed, so changing all refs
to that name. The old TaskQueueManager::DoWork event now has
category 'sequence_manager', and has different semantics. To
maintain backwards compatibility, we still consider
TaskQueueManager::DoWork events with 'toplevel' category to be
scheduler top level tasks.

This also changes total EQT to use scheduler task. This was a
case that was missed while fixing  crbug.com/832651 .

Bug: chromium:871204 , chromium:832651 

Change-Id: I430ae8553e64ff30a94ab109a045e58849a99390
Reviewed-on: https://chromium-review.googlesource.com/c/1278111
Reviewed-by: oysteine <oysteine@chromium.org>
Reviewed-by: Ben Hayden <benjhayden@chromium.org>
Commit-Queue: Deepanjan Roy <dproy@chromium.org>

[modify] https://crrev.com/c2f72b8a9299ddd88999b154b797bce10201d853/tracing/tracing/extras/chrome/event_finder_utils.html
[modify] https://crrev.com/c2f72b8a9299ddd88999b154b797bce10201d853/tracing/tracing/importer/user_model_builder_test.html
[modify] https://crrev.com/c2f72b8a9299ddd88999b154b797bce10201d853/tracing/tracing/metrics/v8/runtime_stats_metric_test.html
[modify] https://crrev.com/c2f72b8a9299ddd88999b154b797bce10201d853/tracing/tracing/metrics/system_health/power_metric_test.html
[modify] https://crrev.com/c2f72b8a9299ddd88999b154b797bce10201d853/trace_processor/experimental/mappers/long_running_tasks_mapper.html
[modify] https://crrev.com/c2f72b8a9299ddd88999b154b797bce10201d853/tracing/tracing/metrics/system_health/expected_queueing_time_metric_test.html
[modify] https://crrev.com/c2f72b8a9299ddd88999b154b797bce10201d853/tracing/tracing/metrics/system_health/loading_metric_test.html
[modify] https://crrev.com/c2f72b8a9299ddd88999b154b797bce10201d853/tracing/tracing/extras/chrome/estimated_input_latency_test.html
[modify] https://crrev.com/c2f72b8a9299ddd88999b154b797bce10201d853/tracing/tracing/extras/chrome/event_finder_utils_test.html
[modify] https://crrev.com/c2f72b8a9299ddd88999b154b797bce10201d853/tracing/tracing/metrics/system_health/expected_queueing_time_metric.html

Project Member

Comment 16 by bugdroid1@chromium.org, Oct 16

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

commit e4b18306674979a2fffe59ae60e51320aa20ad6e
Author: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Date: Tue Oct 16 05:59:06 2018

Roll src/third_party/catapult 2be20fdd2d70..88afab4ff0f0 (4 commits)

https://chromium.googlesource.com/catapult.git/+log/2be20fdd2d70..88afab4ff0f0


git log 2be20fdd2d70..88afab4ff0f0 --date=short --no-merges --format='%ad %ae %s'
2018-10-15 benjhayden@chromium.org Add ReportCacheRequest for v2spa service worker
2018-10-15 pasko@chromium.org event_finder_util: skip renderer events when no mainThread
2018-10-15 chiniforooshan@chromium.org Telemetry: fix a bug in percentage_smooth
2018-10-15 dproy@chromium.org Fix top level scheduler task title


Created with:
  gclient setdep -r src/third_party/catapult@88afab4ff0f0

The AutoRoll server is located here: https://autoroll.skia.org/r/catapult-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.

CQ_INCLUDE_TRYBOTS=luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel

BUG=chromium:891797, chromium:895096 , chromium:871204 , chromium:832651 
TBR=sullivan@chromium.org

Change-Id: I2a5a1b05066c3c5f10bbec768c0bd430c80e5fa4
Reviewed-on: https://chromium-review.googlesource.com/c/1282342
Reviewed-by: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Commit-Queue: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#599878}
[modify] https://crrev.com/e4b18306674979a2fffe59ae60e51320aa20ad6e/DEPS

Sign in to add a comment