Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1344053002:
Add UMA tracing to requestIdleCallback. (Closed)
|
Created:
5 years, 3 months ago by rmcilroy Modified:
5 years, 3 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, Inactive, dglazkov+blink, eae+blinkwatch, rwlbuis, sof, vivekg_samsung, vivekg Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionAdd UMA tracing to requestIdleCallback.
BUG=530780
Committed: https://crrev.com/003ef8b18ab0d0d87799c73c389e00efd16172a8
git-svn-id: svn://svn.chromium.org/blink/trunk@202293 bbb929c8-8fbe-4397-9dbb-9b2b20218538
Patch Set 1 #
Total comments: 8
Patch Set 2 : Fix overrun UMA name #
Depends on Patchset: Messages
Total messages: 21 (5 generated)
rmcilroy@chromium.org changed reviewers: + jochen@chromium.org, skyostil@chromium.org
Corresponding histograms.xml update in https://codereview.chromium.org/1342143002 Sami: could you review to check these are the histograms we want. Jochen: could you review for owners please. Thanks!
https://codereview.chromium.org/1344053002/diff/1/Source/core/dom/ScriptedIdl... File Source/core/dom/ScriptedIdleTaskController.cpp (right): https://codereview.chromium.org/1344053002/diff/1/Source/core/dom/ScriptedIdl... Source/core/dom/ScriptedIdleTaskController.cpp:123: Platform::current()->histogramCustomCounts("WebCore.ScriptedIdleTaskController.IdleCallbackDeadline", allotedTimeMillis, 0, 50, 50); that looks like it's in seconds?
https://codereview.chromium.org/1344053002/diff/1/Source/core/dom/ScriptedIdl... File Source/core/dom/ScriptedIdleTaskController.cpp (right): https://codereview.chromium.org/1344053002/diff/1/Source/core/dom/ScriptedIdl... Source/core/dom/ScriptedIdleTaskController.cpp:123: Platform::current()->histogramCustomCounts("WebCore.ScriptedIdleTaskController.IdleCallbackDeadline", allotedTimeMillis, 0, 50, 50); On 2015/09/15 16:38:46, jochen wrote: > that looks like it's in seconds? I don't think so. The seconds values are multiplied by 1000 above to make ms (the numbers also looked right in local testing).
https://codereview.chromium.org/1344053002/diff/1/Source/core/dom/ScriptedIdl... File Source/core/dom/ScriptedIdleTaskController.cpp (right): https://codereview.chromium.org/1344053002/diff/1/Source/core/dom/ScriptedIdl... Source/core/dom/ScriptedIdleTaskController.cpp:123: Platform::current()->histogramCustomCounts("WebCore.ScriptedIdleTaskController.IdleCallbackDeadline", allotedTimeMillis, 0, 50, 50); On 2015/09/15 at 16:50:46, rmcilroy wrote: > On 2015/09/15 16:38:46, jochen wrote: > > that looks like it's in seconds? > > I don't think so. The seconds values are multiplied by 1000 above to make ms (the numbers also looked right in local testing). hum, ok, so you expect that there will be at most 50ms alloted ever?
https://codereview.chromium.org/1344053002/diff/1/Source/core/dom/ScriptedIdl... File Source/core/dom/ScriptedIdleTaskController.cpp (right): https://codereview.chromium.org/1344053002/diff/1/Source/core/dom/ScriptedIdl... Source/core/dom/ScriptedIdleTaskController.cpp:129: Platform::current()->histogramCustomCounts("WebCore.ScriptedIdleTaskController.idleCallbackOverrunMs", overrunMillis, 0, 10000, 50); "idle" isn't capitalized here but is above -- I guess these are generally always capitalized? This one also has "Ms" in the end but the other doesn't.
https://codereview.chromium.org/1344053002/diff/1/Source/core/dom/ScriptedIdl... File Source/core/dom/ScriptedIdleTaskController.cpp (right): https://codereview.chromium.org/1344053002/diff/1/Source/core/dom/ScriptedIdl... Source/core/dom/ScriptedIdleTaskController.cpp:123: Platform::current()->histogramCustomCounts("WebCore.ScriptedIdleTaskController.IdleCallbackDeadline", allotedTimeMillis, 0, 50, 50); On 2015/09/15 16:53:37, jochen wrote: > On 2015/09/15 at 16:50:46, rmcilroy wrote: > > On 2015/09/15 16:38:46, jochen wrote: > > > that looks like it's in seconds? > > > > I don't think so. The seconds values are multiplied by 1000 above to make ms > (the numbers also looked right in local testing). > > hum, ok, so you expect that there will be at most 50ms alloted ever? Right, that 50ms maximum required by the spec too.
https://codereview.chromium.org/1344053002/diff/1/Source/core/dom/ScriptedIdl... File Source/core/dom/ScriptedIdleTaskController.cpp (right): https://codereview.chromium.org/1344053002/diff/1/Source/core/dom/ScriptedIdl... Source/core/dom/ScriptedIdleTaskController.cpp:123: Platform::current()->histogramCustomCounts("WebCore.ScriptedIdleTaskController.IdleCallbackDeadline", allotedTimeMillis, 0, 50, 50); On 2015/09/15 16:53:37, jochen wrote: > On 2015/09/15 at 16:50:46, rmcilroy wrote: > > On 2015/09/15 16:38:46, jochen wrote: > > > that looks like it's in seconds? > > > > I don't think so. The seconds values are multiplied by 1000 above to make ms > (the numbers also looked right in local testing). > > hum, ok, so you expect that there will be at most 50ms alloted ever? Yup, the spec explicitly limits the maximum deadline to 50ms (to try and ensure user input is always responded to within ~100ms and appear instant). This is also the case for internal idle tasks (e.g. those used by V8's GC). The overrun could be any amount of time, so I've got a 10 second max there, but here 50ms should be the most we ever see.
lgtm
https://codereview.chromium.org/1344053002/diff/1/Source/core/dom/ScriptedIdl... File Source/core/dom/ScriptedIdleTaskController.cpp (right): https://codereview.chromium.org/1344053002/diff/1/Source/core/dom/ScriptedIdl... Source/core/dom/ScriptedIdleTaskController.cpp:129: Platform::current()->histogramCustomCounts("WebCore.ScriptedIdleTaskController.idleCallbackOverrunMs", overrunMillis, 0, 10000, 50); On 2015/09/15 16:58:44, Sami wrote: > "idle" isn't capitalized here but is above -- I guess these are generally always > capitalized? This one also has "Ms" in the end but the other doesn't. Argh, changed this locally but didn't upload. Thanks, I'll fix when back at a computer.
https://codereview.chromium.org/1344053002/diff/1/Source/core/dom/ScriptedIdl... File Source/core/dom/ScriptedIdleTaskController.cpp (right): https://codereview.chromium.org/1344053002/diff/1/Source/core/dom/ScriptedIdl... Source/core/dom/ScriptedIdleTaskController.cpp:129: Platform::current()->histogramCustomCounts("WebCore.ScriptedIdleTaskController.idleCallbackOverrunMs", overrunMillis, 0, 10000, 50); On 2015/09/15 17:05:22, rmcilroy wrote: > On 2015/09/15 16:58:44, Sami wrote: > > "idle" isn't capitalized here but is above -- I guess these are generally > always > > capitalized? This one also has "Ms" in the end but the other doesn't. > > Argh, changed this locally but didn't upload. Thanks, I'll fix when back at a > computer. Done.
The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1344053002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1344053002/20001
Thanks, ++(lgtm)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rmcilroy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org Link to the patchset: https://codereview.chromium.org/1344053002/#ps20001 (title: "Fix overrun UMA name")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1344053002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1344053002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://src.chromium.org/viewvc/blink?view=rev&revision=202293
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/003ef8b18ab0d0d87799c73c389e00efd16172a8 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
