Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(29)

Issue 2720023003: [scheduler] Add a CHECK for renderer main thread load metrics. (Closed)

Created:
3 years, 9 months ago by altimin
Modified:
3 years, 9 months ago
Reviewers:
Sami
CC:
chromium-reviews, blink-reviews, kinuko+watch, scheduler-bugs_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[scheduler] Add a CHECK for renderer main thread load metrics. Change DCHECK to CHECK in RendererScheduler's Report{Foreground,Background}RendererThreadLoad. This should help pin down strange anomalies from metrics. This patch should be Canary-channel only, and expected to be extremely rare (less than 1 failure/day on Canary). Please revert without asking if something goes wrong. BUG=693542 Review-Url: https://codereview.chromium.org/2720023003 Cr-Commit-Position: refs/heads/master@{#453783} Committed: https://chromium.googlesource.com/chromium/src/+/2100af86f06ded92e913c67723bbff9af08df307

Patch Set 1 #

Total comments: 1

Patch Set 2 : git cl try #

Unified diffs Side-by-side diffs Delta from patch set Stats (+4 lines, -2 lines) Patch
M third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc View 1 2 chunks +4 lines, -2 lines 0 comments Download

Messages

Total messages: 16 (9 generated)
altimin
PTAL
3 years, 9 months ago (2017-02-28 15:55:51 UTC) #2
Sami
lgtm. https://codereview.chromium.org/2720023003/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/2720023003/diff/1/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc#newcode58 third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:58: CHECK_LE(load_percentage, 100); Add a TODO to revert.
3 years, 9 months ago (2017-02-28 16:07:20 UTC) #3
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/2720023003/20001
3 years, 9 months ago (2017-03-01 00:49:33 UTC) #10
commit-bot: I haz the power
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/2100af86f06ded92e913c67723bbff9af08df307
3 years, 9 months ago (2017-03-01 00:55:31 UTC) #13
Sami
Any results? Should we revert this now?
3 years, 9 months ago (2017-03-22 18:36:47 UTC) #14
altimin
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2769133003/ by altimin@chromium.org. ...
3 years, 9 months ago (2017-03-23 13:55:14 UTC) #15
altimin
3 years, 9 months ago (2017-03-23 13:57:04 UTC) #16
Message was sent while issue was closed.
On 2017/03/22 18:36:47, Sami wrote:
> Any results? Should we revert this now?

I see a few crashes, but I'm not sure if they are useful. Regardless of that,
this patch needs to be reverted, thanks for reminding me.

Powered by Google App Engine
This is Rietveld 408576698