Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2696833002:
[scheduler] Do not call PumpThrottledTasks immediately when descheduled (Closed)
Description[scheduler] Do not call PumpThrottledTasks immediately when descheduled
BUG=692085
Review-Url: https://codereview.chromium.org/2696833002
Cr-Commit-Position: refs/heads/master@{#450550}
Committed: https://chromium.googlesource.com/chromium/src/+/0ded2254448f22310ab52645b57a864b281c948d
Patch Set 1 #
Total comments: 2
Patch Set 2 : add dcheck #Messages
Total messages: 22 (14 generated)
Description was changed from ========== [scheduler] Do not call PumpThrottledTasks immediately when system is descheduled BUG=692085 ========== to ========== [scheduler] Do not call PumpThrottledTasks immediately when descheduled BUG=692085 ==========
altimin@chromium.org changed reviewers: + alexclarke@chromium.org, skyostil@chromium.org
PTAL
The CQ bit was checked by altimin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Can we get a test for this?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/14 15:14:53, alex clarke wrote: > Can we get a test for this? I'd prefer to land this speculative fix without a test. A test is very hard to add and we need to expose a lot of internals to test one specific line.
I kinda feel a test would be nice too. That would also be proof that this case can actually be hit. Not sure what needs to be exposed to make that happen though. https://codereview.chromium.org/2696833002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (right): https://codereview.chromium.org/2696833002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:488: AlignedThrottledRunTime(std::max(now, unaligned_runtime)); DCHECK that the time is not in the past?
lgtm to land with the DCHECK, but let's discuss tomorrow if we need to improve the testing around this.
https://codereview.chromium.org/2696833002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (right): https://codereview.chromium.org/2696833002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:488: AlignedThrottledRunTime(std::max(now, unaligned_runtime)); On 2017/02/14 17:54:42, Sami wrote: > DCHECK that the time is not in the past? Done.
The CQ bit was checked by altimin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 altimin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/2696833002/#ps20001 (title: "add dcheck")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487117634077950,
"parent_rev": "1e235b69eb08d4d20006c5be62bc3cca37532801", "commit_rev":
"0ded2254448f22310ab52645b57a864b281c948d"}
Message was sent while issue was closed.
Description was changed from ========== [scheduler] Do not call PumpThrottledTasks immediately when descheduled BUG=692085 ========== to ========== [scheduler] Do not call PumpThrottledTasks immediately when descheduled BUG=692085 Review-Url: https://codereview.chromium.org/2696833002 Cr-Commit-Position: refs/heads/master@{#450550} Committed: https://chromium.googlesource.com/chromium/src/+/0ded2254448f22310ab52645b57a... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/0ded2254448f22310ab52645b57a... |
