Scheduler UKMs don't report walltime / cputime correctly |
|||||||||||||
Issue descriptionWe currently override the TaskDuration field with thread_time, which is 0 in 99% of cases. There are two issues here: 1) (fix ASAP) Record thread_time into the correct field so that we get useful wall_time stats. 2) (fix later) Make sure that the tasks we capture UKMs for have thread_time recorded somehow.
,
May 11 2018
Requesting merge into M67 for patch in #1, as this will be needed to collect sensible UKM statistics from stable. The patch is very low-risk.
,
May 11 2018
Pls apply appropriate OSs label. Thank you.
,
May 11 2018
,
May 11 2018
This bug requires manual review: M67 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
May 11 2018
Pls update the bug with canary result on Monday.
,
May 14 2018
The NextAction date has arrived: 2018-05-14
,
May 14 2018
The patch didn't make it into Friday's canary build. Will wait until Wednesday for some initial results.
,
May 14 2018
Re #8: Patch should be in latest canary version #68.0.3430.0 which is branched at chromium revision #558173 (Cl listed #1 Cr-Commit-Position: refs/heads/master@{#557860}). Could you pls double check?
,
May 14 2018
Right, thanks! I was looking at Android only. The patch is in canary since 68.0.3428.0 on Windows/MacOS and UKM data we received since then looks good. I don't see any other regression that's attributed to the change. We would like to integrate this patch into M67 as it is a crucial bugfix to the UKM collection of blink scheduler task statistics, which we would like to use to verify the impact of performance experiments we are running. It's a very low impact change, with 1 LOC delta, and only affecting UKM collection specific to the blink scheduler. Thus, it's very limited in risk.
,
May 14 2018
Approving merge to M67 branch 3396 based on comment #10. Pls merge. Thank you.
,
May 14 2018
,
May 14 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b4ff00451e8f9b887d5d10eb8b5a23764ff1b188 commit b4ff00451e8f9b887d5d10eb8b5a23764ff1b188 Author: Eric Seckler <eseckler@chromium.org> Date: Mon May 14 15:57:56 2018 scheduler: Collect a task's thread_time into correct UKM field thread_time was previously overriding the wall time, but should actually be reported in a separate UKM field. Bug: 842112 Change-Id: If772b5276c0629a4184006bb5382939d9281d3d7 Reviewed-on: https://chromium-review.googlesource.com/1054011 Reviewed-by: Sami Kyöstilä <skyostil@chromium.org> Commit-Queue: Eric Seckler <eseckler@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#557860}(cherry picked from commit 2794e1b22391a55780ef2c6d1734ef970fc7acc8) Reviewed-on: https://chromium-review.googlesource.com/1057248 Reviewed-by: Eric Seckler <eseckler@chromium.org> Cr-Commit-Position: refs/branch-heads/3396@{#586} Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428} [modify] https://crrev.com/b4ff00451e8f9b887d5d10eb8b5a23764ff1b188/third_party/blink/renderer/platform/scheduler/main_thread/main_thread_scheduler_impl.cc
,
May 15 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d6e1319d13772df9141d7d0e2456328341ec953b commit d6e1319d13772df9141d7d0e2456328341ec953b Author: Eric Seckler <eseckler@chromium.org> Date: Tue May 15 12:55:07 2018 scheduler: Align UKM sampling with CPU time sampling. When TaskQueueManagerImpl is sampling CPU time, align the sampling of tasks for UKM reporting with the CPU time sampling, so that UKM-sampled tasks always report CPU time. Bug: 842112 Change-Id: I56ebc8c1f8a82165e5cb71cc4ea8bbf4c161ba43 Reviewed-on: https://chromium-review.googlesource.com/1057167 Commit-Queue: Eric Seckler <eseckler@chromium.org> Reviewed-by: Alexander Timin <altimin@chromium.org> Reviewed-by: Sami Kyöstilä <skyostil@chromium.org> Cr-Commit-Position: refs/heads/master@{#558675} [modify] https://crrev.com/d6e1319d13772df9141d7d0e2456328341ec953b/third_party/blink/renderer/platform/scheduler/base/task_queue_manager.h [modify] https://crrev.com/d6e1319d13772df9141d7d0e2456328341ec953b/third_party/blink/renderer/platform/scheduler/base/task_queue_manager_impl.cc [modify] https://crrev.com/d6e1319d13772df9141d7d0e2456328341ec953b/third_party/blink/renderer/platform/scheduler/base/task_queue_manager_impl.h [modify] https://crrev.com/d6e1319d13772df9141d7d0e2456328341ec953b/third_party/blink/renderer/platform/scheduler/common/scheduler_helper.cc [modify] https://crrev.com/d6e1319d13772df9141d7d0e2456328341ec953b/third_party/blink/renderer/platform/scheduler/common/scheduler_helper.h [modify] https://crrev.com/d6e1319d13772df9141d7d0e2456328341ec953b/third_party/blink/renderer/platform/scheduler/main_thread/main_thread_scheduler_impl.cc [modify] https://crrev.com/d6e1319d13772df9141d7d0e2456328341ec953b/third_party/blink/renderer/platform/scheduler/main_thread/main_thread_scheduler_impl.h [modify] https://crrev.com/d6e1319d13772df9141d7d0e2456328341ec953b/third_party/blink/renderer/platform/scheduler/main_thread/main_thread_scheduler_impl_unittest.cc
,
May 15 2018
govind@, would you consider the patch in #14 for merge into M67 as well? It addresses the second problem described in #0 and would also be very useful for the reporting of the same UKM metric, but is a little bit more involved than the earlier patch in #1. If so, I'd report back once it has been in canary for a day or two. Sorry about the late request! Thanks!
,
May 15 2018
This bug requires manual review: We are only 13 days from stable. Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
May 15 2018
CL listed at #14 is not yet baked in canary so we can't take it in for this week Beta release tomorrow. And we only have next week beta left before stable promotion. As per #15, CL listed at #14 is little more involved and per comment #0, (2) fix later, then it would be great if this can wait until M68. Pls let me know ASAP if there is any concern here.
,
May 16 2018
I think we can make do with this in M68.
,
May 16 2018
Thank you eseckler@. Rejecting merge to M67 based on comment #18.
,
May 30 2018
|
|||||||||||||
►
Sign in to add a comment |
|||||||||||||
Comment 1 by bugdroid1@chromium.org
, May 11 2018