Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(91)

Issue 2891393002: [scheduler] Discard anomalously long tasks and bump metrics version. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month, 1 week ago by altimin
Modified:
1 month ago
Reviewers:
Ilya Sherman, Sami
CC:
chromium-reviews, blink-reviews, kinuko+watch, scheduler-bugs_chromium.org, asvitkine+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[scheduler] Discard anomalously long tasks and bump metrics version. We assume that very long tasks (> 30 seconds) are a result of measurement errors such as system falling asleep in the middle of a task. R=skyostil@chromium.org,isherman@chromium.org Review-Url: https://codereview.chromium.org/2891393002 Cr-Commit-Position: refs/heads/master@{#473890} Committed: https://chromium.googlesource.com/chromium/src/+/1dc490d564adebf5ed17d275ac5654eb9d63606e

Patch Set 1 #

Total comments: 6

Patch Set 2 : addressed comments from skyostil@ #

Total comments: 8

Patch Set 3 : Addressed comments from isherman@ #

Messages

Total messages: 27 (14 generated)
altimin
PTAL
1 month, 1 week ago (2017-05-19 15:32:11 UTC) #3
Sami
lgtm with two changes. https://codereview.chromium.org/2891393002/diff/1/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2891393002/diff/1/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc#newcode72 third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:72: "RendererScheduler.ForegroundRendererLoad2", load_percentage); I don't think ...
1 month, 1 week ago (2017-05-19 16:11:18 UTC) #4
altimin
https://codereview.chromium.org/2891393002/diff/1/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2891393002/diff/1/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc#newcode72 third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:72: "RendererScheduler.ForegroundRendererLoad2", load_percentage); On 2017/05/19 16:11:18, Sami wrote: > I ...
1 month, 1 week ago (2017-05-19 16:20:45 UTC) #5
Ilya Sherman
https://codereview.chromium.org/2891393002/diff/20001/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2891393002/diff/20001/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc#newcode58 third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:58: // of the task). Hmm, why are you making ...
1 month, 1 week ago (2017-05-19 21:15:11 UTC) #8
altimin
https://codereview.chromium.org/2891393002/diff/20001/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2891393002/diff/20001/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc#newcode58 third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:58: // of the task). On 2017/05/19 21:15:11, Ilya Sherman ...
1 month, 1 week ago (2017-05-19 22:34:20 UTC) #11
Sami
https://codereview.chromium.org/2891393002/diff/20001/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2891393002/diff/20001/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc#newcode58 third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:58: // of the task). FWIW the inactive renderer dialog ...
1 month, 1 week ago (2017-05-22 13:57:39 UTC) #12
Ilya Sherman
https://codereview.chromium.org/2891393002/diff/20001/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2891393002/diff/20001/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc#newcode58 third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:58: // of the task). On 2017/05/19 22:34:20, altimin wrote: ...
1 month, 1 week ago (2017-05-22 20:43:43 UTC) #13
altimin
https://codereview.chromium.org/2891393002/diff/20001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2891393002/diff/20001/tools/metrics/histograms/histograms.xml#oldcode59038 tools/metrics/histograms/histograms.xml:59038: -<histogram name="RendererScheduler.BackgroundRendererMainThreadLoad" units="%"> On 2017/05/22 20:43:43, Ilya Sherman wrote: ...
1 month ago (2017-05-23 12:26:46 UTC) #15
Sami
lgtm
1 month ago (2017-05-23 14:34:25 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2891393002/40001
1 month ago (2017-05-23 14:40:54 UTC) #22
commit-bot: I haz the power
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/1dc490d564adebf5ed17d275ac5654eb9d63606e
1 month ago (2017-05-23 14:44:39 UTC) #25
Z_DONOTUSE
On 2017/05/22 at 20:43:43, isherman wrote: > https://codereview.chromium.org/2891393002/diff/20001/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc > File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): > > https://codereview.chromium.org/2891393002/diff/20001/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc#newcode58 ...
1 month ago (2017-05-23 19:40:10 UTC) #26
altimin
1 month ago (2017-05-25 15:03:22 UTC) #27
Message was sent while issue was closed.
On 2017/05/23 19:40:10, Z_DONOTUSE wrote:
> On 2017/05/22 at 20:43:43, isherman wrote:
> >
>
https://codereview.chromium.org/2891393002/diff/20001/third_party/WebKit/Sour...
> > File
>
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
> (right):
> > 
> >
>
https://codereview.chromium.org/2891393002/diff/20001/third_party/WebKit/Sour...
> >
>
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:58:
> // of the task).
> > On 2017/05/19 22:34:20, altimin wrote:
> > > On 2017/05/19 21:15:11, Ilya Sherman wrote:
> > > > Hmm, why are you making this assumption?  Could you specifically
subtract
> out
> > > > sleep time, rather than just guessing?  I'm concerned that we might be
> > > blinding
> > > > ourselves to real egregious tasks by simply ignoring them.
> > > 
> > > The problem is that we can't reliably detect when system is sleeping (plus
> it's
> > > very platform-specific).
> > > We are already doing something like this for other metrics. 
> > > 
> > > It is a good idea to add one more histogram to track the number (and
> duration)
> > > of discarded tasks? This way we can monitor and act if something goes
wrong.
> > > (The assumption is here that we have a very small number of bad tasks).
> > 
> > Having some visibility into what data is being missed by this metric does
seem
> appropriate.  If there's already coverage for this, then LGTM; otherwise, yes,
> please do add an additional histogram.
> 
> Looks like this feedback didn't get a response. Is there already coverage for
> this? I don't see it in the patch.

I haven't replied to the comment, sorry. In this case I assume that per Sami's
comment we will get a crash dump for a hanged renderer, so we have coverage for
this.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 23e94e589