Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2591773002:
[cc] BackToBackBFS: Disable time source if all pending observers removed (Closed)
|
Created:
4 years ago by Eric Seckler Modified:
4 years ago Reviewers:
brianderson CC:
chromium-reviews, cc-bugs_chromium.org, scheduler-bugs_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[cc] BackToBackBFS: Disable time source if all pending observers removed
Currently, we activate the time source as soon as one observer becomes
pending. If that observer is removed, the time source may currently do
an unneccessary tick, because we only deactivate the time source if
all observers were removed.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Committed: https://crrev.com/a4a4acddee1984279411cbe73737e849924e9a8b
Cr-Commit-Position: refs/heads/master@{#439841}
Patch Set 1 #
Total comments: 2
Patch Set 2 : add DCHECK #
Messages
Total messages: 21 (15 generated)
Description was changed from ========== [cc] BackToBackBFS: Disable time source if all pending observers removed Currently, we activate the time source as soon as one observer becomes pending. If that observer is removed, the time source may currently do an unneccessary tick, because we only deactivate the time source if all observers were removed. ========== to ========== [cc] BackToBackBFS: Disable time source if all pending observers removed Currently, we activate the time source as soon as one observer becomes pending. If that observer is removed, the time source may currently do an unneccessary tick, because we only deactivate the time source if all observers were removed. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by eseckler@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...
eseckler@chromium.org changed reviewers: + brianderson@chromium.org
Brian, this is the extracted change from https://codereview.chromium.org/2583483002/. It affects sequence numbering in the tests there, so I'd like to land this one first :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2591773002/diff/1/cc/scheduler/begin_frame_so... File cc/scheduler/begin_frame_source.cc (right): https://codereview.chromium.org/2591773002/diff/1/cc/scheduler/begin_frame_so... cc/scheduler/begin_frame_source.cc:96: void BackToBackBeginFrameSource::OnTimerTick() { Would it make sense to DCHECK here that pending_begin_frame_observers_ is empty?
The CQ bit was checked by eseckler@chromium.org to run a CQ dry run
https://codereview.chromium.org/2591773002/diff/1/cc/scheduler/begin_frame_so... File cc/scheduler/begin_frame_source.cc (right): https://codereview.chromium.org/2591773002/diff/1/cc/scheduler/begin_frame_so... cc/scheduler/begin_frame_source.cc:96: void BackToBackBeginFrameSource::OnTimerTick() { On 2016/12/20 16:37:36, brianderson wrote: > Would it make sense to DCHECK here that pending_begin_frame_observers_ is empty? I assume you mean "not empty" :) Added a DCHECK below.
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 eseckler@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brianderson@chromium.org Link to the patchset: https://codereview.chromium.org/2591773002/#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": 1482256590624120,
"parent_rev": "b3dc67ae6d4840ca7ec2e674068f356615d2a793", "commit_rev":
"a9578c46f80fb1226df194215147238e32c5c5ad"}
Message was sent while issue was closed.
Description was changed from ========== [cc] BackToBackBFS: Disable time source if all pending observers removed Currently, we activate the time source as soon as one observer becomes pending. If that observer is removed, the time source may currently do an unneccessary tick, because we only deactivate the time source if all observers were removed. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== [cc] BackToBackBFS: Disable time source if all pending observers removed Currently, we activate the time source as soon as one observer becomes pending. If that observer is removed, the time source may currently do an unneccessary tick, because we only deactivate the time source if all observers were removed. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2591773002 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [cc] BackToBackBFS: Disable time source if all pending observers removed Currently, we activate the time source as soon as one observer becomes pending. If that observer is removed, the time source may currently do an unneccessary tick, because we only deactivate the time source if all observers were removed. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2591773002 ========== to ========== [cc] BackToBackBFS: Disable time source if all pending observers removed Currently, we activate the time source as soon as one observer becomes pending. If that observer is removed, the time source may currently do an unneccessary tick, because we only deactivate the time source if all observers were removed. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Committed: https://crrev.com/a4a4acddee1984279411cbe73737e849924e9a8b Cr-Commit-Position: refs/heads/master@{#439841} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/a4a4acddee1984279411cbe73737e849924e9a8b Cr-Commit-Position: refs/heads/master@{#439841} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
